Convert greybus/uart.c from IDR to XArray. The abstract data type XArray
is more memory-efficient, parallelisable, and cache friendly. It takes
advantage of RCU to perform lookups without locking. Furthermore, IDR is
deprecated because XArray has a better (cleaner and more consistent) API.
Signed-off-by: Fabio M. De Francesco <fmdefrancesco(a)gmail.com>
---
v3->v4:
Remove mutex_lock/unlock around xa_load(). These locks seem to
be unnecessary because there is a 1:1 correspondence between
a specific minor and its gb_tty and there is no reference
counting. I think that the RCU locks used inside xa_load()
are sufficient to protect this API from returning an invalid
gb_tty in case of concurrent access. Some more considerations
on this topic are in the following message to linux-kernel list:
https://lore.kernel.org/lkml/3554184.2JXonMZcNW@localhost.localdomain/
v2->v3:
Fix some issues according to a review by Alex Elder <elder(a)ieee.org>
v1->v2:
Fix an issue found by the kernel test robot. It is due to
passing to xa_*lock() the same old mutex that IDR used with
the previous version of the code.
drivers/staging/greybus/uart.c | 32 ++++++++++++++------------------
1 file changed, 14 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c
index 73f01ed1e5b7..f66983adb51b 100644
--- a/drivers/staging/greybus/uart.c
+++ b/drivers/staging/greybus/uart.c
@@ -22,7 +22,7 @@
#include <linux/serial.h>
#include <linux/tty_driver.h>
#include <linux/tty_flip.h>
-#include <linux/idr.h>
+#include <linux/xarray.h>
#include <linux/fs.h>
#include <linux/kdev_t.h>
#include <linux/kfifo.h>
@@ -32,8 +32,9 @@
#include "gbphy.h"
-#define GB_NUM_MINORS 16 /* 16 is more than enough */
-#define GB_NAME "ttyGB"
+#define GB_NUM_MINORS 16 /* 16 is more than enough */
+#define GB_RANGE_MINORS XA_LIMIT(0, GB_NUM_MINORS)
+#define GB_NAME "ttyGB"
#define GB_UART_WRITE_FIFO_SIZE PAGE_SIZE
#define GB_UART_WRITE_ROOM_MARGIN 1 /* leave some space in fifo */
@@ -67,8 +68,7 @@ struct gb_tty {
};
static struct tty_driver *gb_tty_driver;
-static DEFINE_IDR(tty_minors);
-static DEFINE_MUTEX(table_lock);
+static DEFINE_XARRAY(tty_minors);
static int gb_uart_receive_data_handler(struct gb_operation *op)
{
@@ -341,8 +341,7 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor)
{
struct gb_tty *gb_tty;
- mutex_lock(&table_lock);
- gb_tty = idr_find(&tty_minors, minor);
+ gb_tty = xa_load(&tty_minors, minor);
if (gb_tty) {
mutex_lock(&gb_tty->mutex);
if (gb_tty->disconnected) {
@@ -353,19 +352,18 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor)
mutex_unlock(&gb_tty->mutex);
}
}
- mutex_unlock(&table_lock);
return gb_tty;
}
static int alloc_minor(struct gb_tty *gb_tty)
{
int minor;
+ int ret;
- mutex_lock(&table_lock);
- minor = idr_alloc(&tty_minors, gb_tty, 0, GB_NUM_MINORS, GFP_KERNEL);
- mutex_unlock(&table_lock);
- if (minor >= 0)
- gb_tty->minor = minor;
+ ret = xa_alloc(&tty_minors, &minor, gb_tty, GB_RANGE_MINORS, GFP_KERNEL);
+ if (ret)
+ return ret;
+ gb_tty->minor = minor;
return minor;
}
@@ -374,9 +372,7 @@ static void release_minor(struct gb_tty *gb_tty)
int minor = gb_tty->minor;
gb_tty->minor = 0; /* Maybe should use an invalid value instead */
- mutex_lock(&table_lock);
- idr_remove(&tty_minors, minor);
- mutex_unlock(&table_lock);
+ xa_erase(&tty_minors, minor);
}
static int gb_tty_install(struct tty_driver *driver, struct tty_struct *tty)
@@ -837,7 +833,7 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
minor = alloc_minor(gb_tty);
if (minor < 0) {
- if (minor == -ENOSPC) {
+ if (minor == -EBUSY) {
dev_err(&gbphy_dev->dev,
"no more free minor numbers\n");
retval = -ENODEV;
@@ -982,7 +978,7 @@ static void gb_tty_exit(void)
{
tty_unregister_driver(gb_tty_driver);
put_tty_driver(gb_tty_driver);
- idr_destroy(&tty_minors);
+ xa_destroy(&tty_minors);
}
static const struct gbphy_device_id gb_uart_id_table[] = {
--
2.32.0
Hi
I was looking at the Zephyr for Greybus
<https://github.com/cfriedt/greybus-for-zephyr/> work, and realized that
the actual spec for greybus doesn't actually have support for ADC. My
usecase is to use the ADCs on an attached microcontroller as if they are
native ADCs on a Linux machine.
I understand this is more complicated than it looks - because unlike a
GPIO, or I2C, there's no such thing as a native ADC for the Linux side of
things. Have I understood that correctly?
I stumbled into the IIO subsystem for such requirements where there's some
analog sensor that would need to be read - but that would not work through
a microcontroller like my use case.
If one were to look at adding such support, what would they need? Is this
something that's been discussed before? Would love to understand this a bit
more, and potentially contribute.