gpio: virtuser: new virtual driver

From: Bartosz Golaszewski <[email protected]>

The GPIO subsystem used to have a serious problem with undefined behavior and use-after-free bugs on hot-unplug of GPIO chips. This can be considered a corner-case by some as most GPIO controllers are enabled early in the boot process and live until the system goes down but most GPIO drivers do allow unbind over sysfs, many are loadable modules that can be (force) unloaded and there are also GPIO devices that can be dynamically detached, for instance CP2112 which is a USB GPIO expender.

Bugs can be triggered both from user-space as well as by in-kernel users. We have the means of testing it from user-space via the character device but the issues manifest themselves differently in the kernel.

This is a proposition of adding a new virtual driver - a configurable GPIO consumer that can be configured over configfs (similarly to gpio-sim) or described on the device-tree.

This driver is aimed as a helper in spotting any regressions in hot-unplug handling in GPIOLIB.

User must pass exactly the number of values that the array contains

drivers/gpio/gpio-virtuser.c new file +7 -0

+#include <linux/atomic.h>
+#include <linux/bitmap.h>
+#include <linux/cleanup.h>
+#include <linux/completion.h>
+#include <linux/configfs.h>
+#include <linux/device.h>
+#include <linux/err.h>

drivers/gpio/gpio-virtuser.c new file +5 -0

+#include <linux/limits.h>
+#include <linux/list.h>
+#include <linux/lockdep.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>

drivers/gpio/gpio-virtuser.c new file +3 -0

+#include <linux/string_helpers.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>

drivers/gpio/gpio-virtuser.c new file +6 -0

+struct gpio_virtuser_line_array_data {
+	struct gpio_descs *descs;
+	struct kobject *kobj;
+	struct attribute_group *attr_group;
+};
+

+}

(quoted diff) +2 -0

+	return 0;
+}

+}

drivers/gpio/gpio-virtuser.c new file +5 -0

+static int gpio_virtuser_parse_direction(const char *buf, int *dir, int *val)
+{
+	if (sysfs_streq(buf, "input")) {
+		*dir = GPIO_LINE_DIRECTION_IN;
+		return 0;

(quoted diff) +2 -0

+	return 0;
+}

drivers/gpio/gpio-virtuser.c new file +4 -0

+	}
+
+	return value;
+}

(quoted diff) +2 -0

+	if (ret)
+		return ret;

drivers/gpio/gpio-virtuser.c new file +10 -0

+	guard(mutex)(&data->consumer_lock);
+
+	ret = gpiod_set_consumer_name(data->desc, buf);
+	if (ret)
+		return ret;
+
+	sprintf(data->consumer, buf);
+
+	return len;
+}

(quoted diff) +2 -0

+	if (!data->attr_group->name)
+		return -ENOMEM;

(quoted diff) +1 -0

+		return ret;

+}

+/*
+ * If this is an OF-based system, then we iterate over properties and consider
+ * all whose names end in "-gpios". For configfs we expect an additional string
+ * array property - "gpio-virtuser,ids" - containing the list of all GPIO IDs
+ * to request.

drivers/gpio/gpio-virtuser.c new file +5 -0

+		}
 
+		return ret;
+	}
 
+	return device_property_string_array_count(dev, "gpio-virtuser,ids");
+}

drivers/gpio/gpio-virtuser.c new file +13 -0

+			if (!tmp)
+				return -ENOMEM;
+
+			tmp[diff] = '\0';
+			ids[pos++] = tmp;
+		}
+
+		return 0;
+	}
+
+	return device_property_read_string_array(dev, "gpio-virtuser,ids",
+						 ids, num_ids);
+}

drivers/gpio/gpio-virtuser.c new file +6 -0

+static int gpio_virtuser_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct gpio_descs *descs;
+	int ret, num_ids = 0, i;
+	const char **ids;

drivers/gpio/gpio-virtuser.c new file +38 -0

+	}
+
+	ids = devm_kcalloc(dev, num_ids, sizeof(*ids), GFP_KERNEL);
+	if (!ids)
+		return -ENOMEM;
+
+	ret = gpio_virtuser_get_ids(dev, ids, num_ids);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Failed to get the IDs of GPIOs to request\n");
+
+	for (i = 0; i < num_ids; i++) {
+		descs = devm_gpiod_get_array(dev, ids[i], GPIOD_ASIS);
+		if (IS_ERR(descs))
+			return dev_err_probe(dev, PTR_ERR(descs),
+					     "Failed to request the '%s' GPIOs\n",
+					     ids[i]);
+
+		ret = gpio_virtuser_sysfs_init_line_array_attrs(dev, descs,
+								ids[i]);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to setup the sysfs array interface for the '%s' GPIOs\n",
+					     ids[i]);
+
+		for (j = 0; j < descs->ndescs; j++) {
+			ret = gpio_virtuser_sysfs_init_line_attrs(dev,
+							descs->desc[j],
+							ids[i], j);
+			if (ret)
+				return dev_err_probe(dev, ret,
+						     "Failed to setup the sysfs line interface for the '%s' GPIOs\n",
+						     ids[i]);
+		}
+	}
+
+	return 0;
+}

drivers/gpio/gpio-virtuser.c new file +16 -0

+		return NOTIFY_DONE;
+
+	switch (action) {
+	case BUS_NOTIFY_BOUND_DRIVER:
+		vdev->driver_bound = true;
+		break;
+	case BUS_NOTIFY_DRIVER_NOT_BOUND:
+		vdev->driver_bound = false;
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	complete(&vdev->probe_completion);
+	return NOTIFY_OK;
+}

drivers/gpio/gpio-virtuser.c new file +13 -0

+	if (!key)
+		return -ENOMEM;
 
+	strim(key);
 
+	guard(mutex)(&dev->lock);
+
+	if (gpio_virtuser_device_is_live(dev))
+		return -EBUSY;
+
+	kfree(entry->key);
+	entry->key = no_free_ptr(key);
+
+	return count;
+}

drivers/gpio/gpio-virtuser.c new file +4 -0

+	if (sysfs_streq(page, "pull-up")) {
+		entry->flags &= ~(GPIO_PULL_DOWN | GPIO_PULL_DISABLE);
+		entry->flags |= GPIO_PULL_UP;
+	} else if (sysfs_streq(page, "pull-down")) {

drivers/gpio/gpio-virtuser.c new file +3 -0

+	}
+
+	return count;

drivers/gpio/gpio-virtuser.c new file +2 -0

+	if (!table->dev_id)
+		return -ENOMEM;

drivers/gpio/gpio-virtuser.c new file +8 -0

+	list_for_each_entry(lookup, &dev->lookup_list, siblings)
+		ids[i++] = lookup->con_id;
+
+	properties[0] = PROPERTY_ENTRY_STRING_ARRAY_LEN("gpio-virtuser,ids",
+							ids, num_ids);
+
+	return fwnode_create_software_node(properties, NULL);
+}

drivers/gpio/gpio-virtuser.c new file +2 -0

+	if (!dev)
+		return ERR_PTR(-ENOMEM);

(quoted diff) +2 -0

+		return ret;
+	}

Mon, May 27, 2024 at 04:40:54PM +0200, Bartosz Golaszewski kirjoitti:

Quoted 14 lines

From: Bartosz Golaszewski <[email protected]>

The GPIO subsystem used to have a serious problem with undefined behavior and use-after-free bugs on hot-unplug of GPIO chips. This can be considered a corner-case by some as most GPIO controllers are enabled early in the boot process and live until the system goes down but most GPIO drivers do allow unbind over sysfs, many are loadable modules that can be (force) unloaded and there are also GPIO devices that can be dynamically detached, for instance CP2112 which is a USB GPIO expender.

Bugs can be triggered both from user-space as well as by in-kernel users. We have the means of testing it from user-space via the character device but the issues manifest themselves differently in the kernel.

Quoted 6 lines

This is a proposition of adding a new virtual driver - a configurable GPIO consumer that can be configured over configfs (similarly to gpio-sim) or described on the device-tree.

This driver is aimed as a helper in spotting any regressions in hot-unplug handling in GPIOLIB.

...

Quoted 1 line

User must pass exactly the number of values that the array contains

Can't we assume non-active values for the rest if less than needed were provided? For more than that, why do we care?

...

Quoted 10 lines

drivers/gpio/gpio-virtuser.c new file +10 -0

+#include <linux/atomic.h>
+#include <linux/bitmap.h>
+#include <linux/cleanup.h>
+#include <linux/completion.h>
+#include <linux/configfs.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/machine.h>

drivers/gpio/gpio-virtuser.c line 23

Quoted 6 lines

drivers/gpio/gpio-virtuser.c new file +4 -0

+#include <linux/idr.h>
 
+#include <linux/interrupt.h>
+#include <linux/irq_work.h>
 
+#include <linux/kernel.h>

Do you need this?

Quoted 14 lines

drivers/gpio/gpio-virtuser.c new file +13 -0

+#include <linux/limits.h>
+#include <linux/list.h>
+#include <linux/lockdep.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/of.h>
+#include <linux/overflow.h>
+#include <linux/platform_device.h>
+#include <linux/printk.h>
+#include <linux/property.h>
+#include <linux/slab.h>

Quoted 1 line

+#include <linux/string.h>

Implied by string_helpers.h

Quoted 3 lines

drivers/gpio/gpio-virtuser.c new file +3 -0

+#include <linux/string_helpers.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>

...

Quoted 6 lines

drivers/gpio/gpio-virtuser.c new file +6 -0

+struct gpio_virtuser_line_array_data {
+     struct gpio_descs *descs;
+     struct kobject *kobj;
+     struct attribute_group *attr_group;
+};
+

drivers/gpio/gpio-virtuser.c line 78

Quoted 10 lines

drivers/gpio/gpio-virtuser.c new file +10 -0

+struct gpio_virtuser_line_data {
+     struct gpio_desc *desc;
+     struct kobject *kobj;
+     struct attribute_group *attr_group;
+     char consumer[GPIO_CONSUMER_NAME_MAX_LEN];
+     struct mutex consumer_lock;
+     unsigned int debounce;
+     atomic_t irq;
+     atomic_t irq_count;
+};

Maybe

struct gpio_virtuser_sysfs_data {
        union {
                struct gpio_desc *desc;
                struct gpio_descs *descs;
        };
        struct kobject *kobj;
        struct attribute_group *attr_group;
};

struct gpio_virtuser_line_array_data {
        struct gpio_virtuser_sysfs_data sd;
};

struct gpio_virtuser_line_data {
        struct gpio_virtuser_sysfs_data sd;
        char consumer[GPIO_CONSUMER_NAME_MAX_LEN];
        struct mutex consumer_lock;
        unsigned int debounce;
        atomic_t irq;
        atomic_t irq_count;
};

?

...

drivers/gpio/gpio-virtuser.c line 83

Quoted 4 lines

drivers/gpio/gpio-virtuser.c new file +4 -0

+struct gpio_virtuser_attr_ctx {
+     struct device_attribute dev_attr;
+     void *data;
+};

struct dev_ext_attribute ?

...

drivers/gpio/gpio-virtuser.c line 103

Quoted 6 lines

drivers/gpio/gpio-virtuser.c new file +6 -0

+struct gpio_virtuser_attr_descr {
+     const char *name;
+     ssize_t (*show)(struct device *, struct device_attribute *, char *);
+     ssize_t (*store)(struct device *, struct device_attribute *,
+                      const char *, size_t);
+};

struct device_attribute ? (Yes, I know that that one is a bit bigger but benefit is that we have some code that you may reuse)

...

drivers/gpio/gpio-virtuser.c line 150

Quoted 11 lines

drivers/gpio/gpio-virtuser.c new file +11 -0

+static ssize_t gpio_virtuser_sysfs_emit_value_array(char *buf,
+                                                 unsigned long *values,
+                                                 size_t num_values)
+{
+     ssize_t len = 0;
+     size_t i;
+
+     for (i = 0; i < num_values; i++)
+             len += sysfs_emit_at(buf, len, "%d",
+                                  test_bit(i, values) ? 1 : 0);
+     return len + sysfs_emit_at(buf, len, "\n");

Why not use %pb?

Quoted 1 line

+}

...

drivers/gpio/gpio-virtuser.c line 158

Quoted 6 lines

drivers/gpio/gpio-virtuser.c new file +6 -0

+static int gpio_virtuser_sysfs_parse_value_array(const char *buf, size_t len,
+                                              unsigned long *values)
+{
+     size_t i;
+
+     for (i = 0; i < len; i++) {

Perhaps

                bool val;
                int ret;

                ret = kstrtobool(...);

                if (ret)
                        return ret;

                assign_bit(...); // btw, why atomic?

Quoted 6 lines

drivers/gpio/gpio-virtuser.c new file +6 -0

+             if (buf[i] == '0')
+                     clear_bit(i, values);
+             else if (buf[i] == '1')
+                     set_bit(i, values);
+             else
+                     return -EINVAL;

Quoted 1 line

+ }

BUT, why not bitmap_parse()?

Quoted 2 lines

(quoted diff) +2 -0

+     return 0;
+}

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+     unsigned long *values __free(bitmap) = bitmap_alloc(descs->ndescs,
+                                                         GFP_KERNEL);

Perhaps

unsigned long *values __free(bitmap) =
bitmap_alloc(descs->ndescs, GFP_KERNEL);

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+     unsigned long *values __free(bitmap) = bitmap_zalloc(descs->ndescs,
+                                                          GFP_KERNEL);

In the similar way?

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+     unsigned long *values __free(bitmap) = bitmap_zalloc(descs->ndescs,
+                                                          GFP_KERNEL);

Ditto.

...

drivers/gpio/gpio-virtuser.c line 304

Quoted 3 lines

drivers/gpio/gpio-virtuser.c new file +3 -0

+{
+     return sysfs_emit(buf, "%s\n",
+                       dir == GPIO_LINE_DIRECTION_IN ? "input" : "output");

I think this maybe transformed to something like str_input_output() in string_choices.h (and you don't even need to include that as it's implied by string_helpers.h)

Quoted 1 line

+}

...

Quoted 5 lines

drivers/gpio/gpio-virtuser.c new file +5 -0

+static int gpio_virtuser_parse_direction(const char *buf, int *dir, int *val)
+{
+     if (sysfs_streq(buf, "input")) {
+             *dir = GPIO_LINE_DIRECTION_IN;
+             return 0;

drivers/gpio/gpio-virtuser.c line 329

Quoted 10 lines

drivers/gpio/gpio-virtuser.c new file +10 -0

+     }
+
+     if (sysfs_streq(buf, "output-high"))
+             *val = 1;
+     else if (sysfs_streq(buf, "output-low"))
+             *val = 0;
+     else
+             return -EINVAL;
+
+     *dir = GPIO_LINE_DIRECTION_OUT;

This can be transformed to use sysfs_match_string() with

static const char * const dirs[] = { "output-low", "output-high", "input" };

        int ret;

        ret = sysfs_match_string(...);
        if (ret < 0)
                return ret;

        *val = ret;
        *dir = ret == 2 ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;

And with this approach it even not clear why do you need dir and val to be separated here (esp. if we add a enum like

GPIO_VIRTUSER_OUT_LOW,
GPIO_VIRTUSER_OUT_HIGH,
GPIO_VIRTUSER_IN,

(with it the string array can also be indexed).

Quoted 2 lines

(quoted diff) +2 -0

+     return 0;
+}

...

Quoted 10 lines

drivers/gpio/gpio-virtuser.c new file +10 -0

+static int gpio_virtuser_parse_value(const char *buf)
+{
+     int value, ret;
+
+     value = sysfs_match_string(gpio_virtuser_sysfs_value_strings, buf);
+     if (value < 0) {
+             /* Can be 0 or 1 too. */
+             ret = kstrtoint(buf, 0, &value);
+             if (ret)
+                     return ret;

drivers/gpio/gpio-virtuser.c line 448

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+             if (value != 0 && value != 1)
+                     return -EINVAL;

Why not kstrtobool()?

Quoted 4 lines

drivers/gpio/gpio-virtuser.c new file +4 -0

+     }
+
+     return value;
+}

...

Quoted 1 line

+ ret = kstrtouint(buf, 10, &debounce);

Why restrict to decimal?

Quoted 2 lines

(quoted diff) +2 -0

+     if (ret)
+             return ret;

...

Quoted 7 lines

drivers/gpio/gpio-virtuser.c new file +7 -0

+static ssize_t
+gpio_virtuser_sysfs_consumer_store(struct device *dev,
+                                struct device_attribute *attr,
+                                const char *buf, size_t len)
+{
+     struct gpio_virtuser_line_data *data = to_gpio_virtuser_data(attr);
+     int ret;

drivers/gpio/gpio-virtuser.c line 594

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+     if (strlen(buf) > GPIO_CONSUMER_NAME_MAX_LEN)
+             return -EINVAL;

You don't need this if you use strscpy() below and check its returned value.

Quoted 10 lines

drivers/gpio/gpio-virtuser.c new file +10 -0

+     guard(mutex)(&data->consumer_lock);
+
+     ret = gpiod_set_consumer_name(data->desc, buf);
+     if (ret)
+             return ret;
+
+     sprintf(data->consumer, buf);
+
+     return len;
+}

...

drivers/gpio/gpio-virtuser.c line 769

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+     data->attr_group->name = devm_kasprintf(dev, GFP_KERNEL,
+                                             "gpiod:%s", id);

Why two lines?

Quoted 2 lines

(quoted diff) +2 -0

+     if (!data->attr_group->name)
+             return -ENOMEM;

...

drivers/gpio/gpio-virtuser.c line 824

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+     ret = devm_add_action_or_reset(dev, gpio_virtuser_mutex_destroy,
+                                    &data->consumer_lock);

Don't we have devm_mutex_init() (git tag --contains shows v6.10-rc1 to me)

Quoted 1 line

+ return ret;

...

drivers/gpio/gpio-virtuser.c line 861

Quoted 3 lines

drivers/gpio/gpio-virtuser.c new file +3 -0

+static int gpio_virtuser_prop_is_gpio(struct property *prop)
+{
+     char *dash = strpbrk(prop->name, "-");

Why not strrchr() ?

Quoted 1 line

+ return dash && strcmp(dash, "-gpios") == 0;

Can't we reuse the suffix from the array from the gpiolib internal header? Also I don't like the form of '-' in the line. "gpios" is good and chance that linker deduplicates the same string if it occurs somewhere else in the binary (in case this goes with =y in .config).

Quoted 1 line

+}

...

Quoted 5 lines

+/*
+ * If this is an OF-based system, then we iterate over properties and consider
+ * all whose names end in "-gpios". For configfs we expect an additional string
+ * array property - "gpio-virtuser,ids" - containing the list of all GPIO IDs
+ * to request.

Why not any other system? What's wrong for having this available for ACPI, for example? Okay, I see that this is probably due to absence of API.

OTOH the last call in the function assumes non-OF cases. Why can't we have the same approach in both?

drivers/gpio/gpio-virtuser.c line 874

Quoted 4 lines

+ */

drivers/gpio/gpio-virtuser.c new file +3 -0

+static int gpio_virtuser_count_ids(struct device *dev)
+{
+     struct fwnode_handle *fwnode = dev_fwnode(dev);

Why? This function is mostly OF one, make it simpler.

        struct device_node *np = dev_of_node(dev);

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+     struct property *prop;
+     int ret = 0;

Quoted 1 line

+ if (is_of_node(fwnode)) {

Instead of this check...

        if (np) {

...can be used.

Quoted 2 lines

+ for_each_property_of_node(to_of_node(fwnode), prop) {

        for_each_property_of_node(np, prop) {

drivers/gpio/gpio-virtuser.c line 881

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+                     if (gpio_virtuser_prop_is_gpio(prop))
+                             ++ret;

Why pre-increment?

Quoted 8 lines

drivers/gpio/gpio-virtuser.c new file +5 -0

+             }
 
+             return ret;
+     }
 
+     return device_property_string_array_count(dev, "gpio-virtuser,ids");
+}

...

drivers/gpio/gpio-virtuser.c line 899

Quoted 10 lines

drivers/gpio/gpio-virtuser.c new file +10 -0

+static int gpio_virtuser_get_ids(struct device *dev, const char **ids,
+                              int num_ids)
+{
+     struct fwnode_handle *fwnode = dev_fwnode(dev);
+     struct property *prop;
+     size_t pos = 0, diff;
+     char *dash, *tmp;
+
+     if (is_of_node(fwnode)) {
+             for_each_property_of_node(to_of_node(fwnode), prop) {

As per above function.

drivers/gpio/gpio-virtuser.c line 907

Quoted 8 lines

drivers/gpio/gpio-virtuser.c new file +8 -0

+                     if (!gpio_virtuser_prop_is_gpio(prop))
+                             continue;
+
+                     dash = strpbrk(prop->name, "-");
+                     diff = dash - prop->name;
+
+                     tmp = devm_kmemdup(dev, prop->name, diff + 1,
+                                        GFP_KERNEL);

devm_kstrndup() is not okay? Okay, we don't have it (yet?), but at least I would rather expect wrapped kstrndup() than this.

Quoted 13 lines

drivers/gpio/gpio-virtuser.c new file +13 -0

+                     if (!tmp)
+                             return -ENOMEM;
+
+                     tmp[diff] = '\0';
+                     ids[pos++] = tmp;
+             }
+
+             return 0;
+     }
+
+     return device_property_read_string_array(dev, "gpio-virtuser,ids",
+                                              ids, num_ids);
+}

...

Quoted 6 lines

drivers/gpio/gpio-virtuser.c new file +6 -0

+static int gpio_virtuser_probe(struct platform_device *pdev)
+{
+     struct device *dev = &pdev->dev;
+     struct gpio_descs *descs;
+     int ret, num_ids = 0, i;
+     const char **ids;

drivers/gpio/gpio-virtuser.c line 937

Quoted 10 lines

drivers/gpio/gpio-virtuser.c new file +10 -0

+     unsigned int j;
+
+     num_ids = gpio_virtuser_count_ids(dev);
+     if (num_ids < 0)
+             return dev_err_probe(dev, num_ids,
+                                  "Failed to get the number of GPIOs to request\n");
+
+     if (num_ids == 0) {
+             dev_err(dev, "No GPIO IDs specified\n");
+             return -EINVAL;

It's okay to

                return dev_err_probe(...);

with know error code.

Quoted 38 lines

drivers/gpio/gpio-virtuser.c new file +38 -0

+     }
+
+     ids = devm_kcalloc(dev, num_ids, sizeof(*ids), GFP_KERNEL);
+     if (!ids)
+             return -ENOMEM;
+
+     ret = gpio_virtuser_get_ids(dev, ids, num_ids);
+     if (ret < 0)
+             return dev_err_probe(dev, ret,
+                                  "Failed to get the IDs of GPIOs to request\n");
+
+     for (i = 0; i < num_ids; i++) {
+             descs = devm_gpiod_get_array(dev, ids[i], GPIOD_ASIS);
+             if (IS_ERR(descs))
+                     return dev_err_probe(dev, PTR_ERR(descs),
+                                          "Failed to request the '%s' GPIOs\n",
+                                          ids[i]);
+
+             ret = gpio_virtuser_sysfs_init_line_array_attrs(dev, descs,
+                                                             ids[i]);
+             if (ret)
+                     return dev_err_probe(dev, ret,
+                                          "Failed to setup the sysfs array interface for the '%s' GPIOs\n",
+                                          ids[i]);
+
+             for (j = 0; j < descs->ndescs; j++) {
+                     ret = gpio_virtuser_sysfs_init_line_attrs(dev,
+                                                     descs->desc[j],
+                                                     ids[i], j);
+                     if (ret)
+                             return dev_err_probe(dev, ret,
+                                                  "Failed to setup the sysfs line interface for the '%s' GPIOs\n",
+                                                  ids[i]);
+             }
+     }
+
+     return 0;
+}

...

drivers/gpio/gpio-virtuser.c line 1017

Quoted 11 lines

drivers/gpio/gpio-virtuser.c new file +11 -0

+static int gpio_virtuser_bus_notifier_call(struct notifier_block *nb,
+                                        unsigned long action, void *data)
+{
+     struct gpio_virtuser_device *vdev;
+     struct device *dev = data;
+     char devname[32];
+
+     vdev = container_of(nb, struct gpio_virtuser_device, bus_notifier);
+     snprintf(devname, sizeof(devname), "gpio-virtuser.%d", vdev->id);
+
+     if (strcmp(dev_name(dev), devname))

        if (!device_match_name(...))

Quoted 16 lines

drivers/gpio/gpio-virtuser.c new file +16 -0

+             return NOTIFY_DONE;
+
+     switch (action) {
+     case BUS_NOTIFY_BOUND_DRIVER:
+             vdev->driver_bound = true;
+             break;
+     case BUS_NOTIFY_DRIVER_NOT_BOUND:
+             vdev->driver_bound = false;
+             break;
+     default:
+             return NOTIFY_DONE;
+     }
+
+     complete(&vdev->probe_completion);
+     return NOTIFY_OK;
+}

...

drivers/gpio/gpio-virtuser.c line 1110

Quoted 9 lines

drivers/gpio/gpio-virtuser.c new file +9 -0

+static ssize_t
+gpio_virtuser_lookup_entry_config_key_store(struct config_item *item,
+                                         const char *page, size_t count)
+{
+     struct gpio_virtuser_lookup_entry *entry =
+                                     to_gpio_virtuser_lookup_entry(item);
+     struct gpio_virtuser_device *dev = entry->parent->parent;
+
+     char *key = kstrndup(skip_spaces(page), count, GFP_KERNEL);

Missing __free() ?

Quoted 15 lines

drivers/gpio/gpio-virtuser.c new file +13 -0

+     if (!key)
+             return -ENOMEM;
 
+     strim(key);
 
+     guard(mutex)(&dev->lock);
+
+     if (gpio_virtuser_device_is_live(dev))
+             return -EBUSY;
+
+     kfree(entry->key);
+     entry->key = no_free_ptr(key);
+
+     return count;
+}

...

Quoted 4 lines

drivers/gpio/gpio-virtuser.c new file +4 -0

+     if (sysfs_streq(page, "pull-up")) {
+             entry->flags &= ~(GPIO_PULL_DOWN | GPIO_PULL_DISABLE);
+             entry->flags |= GPIO_PULL_UP;
+     } else if (sysfs_streq(page, "pull-down")) {

drivers/gpio/gpio-virtuser.c line 1282

Quoted 10 lines

drivers/gpio/gpio-virtuser.c new file +10 -0

+             entry->flags &= ~(GPIO_PULL_UP | GPIO_PULL_DISABLE);
+             entry->flags |= GPIO_PULL_DOWN;
+     } else if (sysfs_streq(page, "pull-disabled")) {
+             entry->flags &= ~(GPIO_PULL_UP | GPIO_PULL_DOWN);
+             entry->flags |= GPIO_PULL_DISABLE;
+     } else if (sysfs_streq(page, "as-is")) {
+             entry->flags &= ~(GPIO_PULL_UP | GPIO_PULL_DOWN |
+                               GPIO_PULL_DISABLE);
+     } else {
+             count = -EINVAL;

return -EINVAL won't (ab)use count semantics.

drivers/gpio/gpio-virtuser.c line 1124

Quoted 3 lines

drivers/gpio/gpio-virtuser.c new file +3 -0

+     }
+
+     return count;

...

Quoted 1 line

+ return sprintf(page, "%s\n", flags & GPIO_ACTIVE_LOW ? "1" : "0");

Somewhere above you used %d for very similar situation, why %s here? Or why "5d" there?

...

Quoted 1 line

+ return sprintf(page, "%s\n", flags & GPIO_TRANSITORY ? "1" : "0");

Ditto.

...

Quoted 1 line

+ return sprintf(page, "%c\n", live ? '1' : '0');

Wow! Third type of the same.

...

Quoted 4 lines

drivers/gpio/gpio-virtuser.c new file +4 -0

+     struct gpiod_lookup_table *table __free(kfree) =
+             kzalloc(struct_size(table, table, num_entries + 1), GFP_KERNEL);
+     if (!table)
+             return -ENOMEM;

drivers/gpio/gpio-virtuser.c line 1439

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+     table->dev_id = kasprintf(GFP_KERNEL, "gpio-virtuser.%d",
+                               dev->id);

Perfectly one line in comparison with the few lines above).

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+     if (!table->dev_id)
+             return -ENOMEM;

...

drivers/gpio/gpio-virtuser.c line 1451

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+                     curr->chip_hwnum = entry->offset < 0
+                                             ? U16_MAX : entry->offset;

Can we leave ? on the previous line?

...

Quoted 1 line

+ ++i;

Why pre-increment?

...

drivers/gpio/gpio-virtuser.c line 1469

Quoted 7 lines

drivers/gpio/gpio-virtuser.c new file +7 -0

+static struct fwnode_handle *
+gpio_virtuser_make_device_swnode(struct gpio_virtuser_device *dev)
+{
+     struct property_entry properties[2];
+     struct gpio_virtuser_lookup *lookup;
+     size_t num_ids;
+     int i = 0;

Why signed? And in all this kind of case, I would split assignment...

drivers/gpio/gpio-virtuser.c line 1478

Quoted 8 lines

drivers/gpio/gpio-virtuser.c new file +8 -0

+     memset(properties, 0, sizeof(properties));
+
+     num_ids = list_count_nodes(&dev->lookup_list);
+     char **ids __free(kfree) = kcalloc(num_ids + 1, sizeof(*ids),
+                                        GFP_KERNEL);
+     if (!ids)
+             return ERR_PTR(-ENOMEM);
+

To be here, that the reader will see immediately (close enough) what is the initial values. Moreover this code will be robuse against changes in between (if i become reusable).

Quoted 8 lines

drivers/gpio/gpio-virtuser.c new file +8 -0

+     list_for_each_entry(lookup, &dev->lookup_list, siblings)
+             ids[i++] = lookup->con_id;
+
+     properties[0] = PROPERTY_ENTRY_STRING_ARRAY_LEN("gpio-virtuser,ids",
+                                                     ids, num_ids);
+
+     return fwnode_create_software_node(properties, NULL);
+}

...

drivers/gpio/gpio-virtuser.c line 1571

Quoted 4 lines

drivers/gpio/gpio-virtuser.c new file +4 -0

+     guard(mutex)(&dev->lock);
+
+     if (live == gpio_virtuser_device_is_live(dev))
+             ret = -EPERM;

With guard in place, just return directly, ...

Quoted 1 line

+ else if (live)

...drop 'else'...

drivers/gpio/gpio-virtuser.c line 1574

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+             ret = gpio_virtuser_device_activate(dev);
+     else

...ditto...

drivers/gpio/gpio-virtuser.c line 1577

Quoted 3 lines

drivers/gpio/gpio-virtuser.c new file +3 -0

+             gpio_virtuser_device_deactivate(dev);
+
+     return ret ?: count;

...and simply return count here.

...

drivers/gpio/gpio-virtuser.c line 1736

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+     struct gpio_virtuser_device *dev __free(kfree) = kzalloc(sizeof(*dev),
+                                                              GFP_KERNEL);

struct gpio_virtuser_device *dev __free(kfree) =
kzalloc(sizeof(*dev), GFP_KERNEL);

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+     if (!dev)
+             return ERR_PTR(-ENOMEM);

...

drivers/gpio/gpio-virtuser.c line 1779

Quoted 5 lines

drivers/gpio/gpio-virtuser.c new file +4 -0

+     ret = platform_driver_register(&gpio_virtuser_driver);
+     if (ret) {
 
+             pr_err("Failed to register the platform driver: %d\n",
+                    ret);

I would keep one line.

Review

Quoted 2 lines

(quoted diff) +2 -0

+             return ret;
+     }

--
With Best Regards,
Andy Shevchenko

Quoted 1 line · Andy Shevchenko

Mon, May 27, 2024 at 04:40:54PM +0200, Bartosz Golaszewski kirjoitti:

Quoted 4 lines

Quoted 1 line

User must pass exactly the number of values that the array contains

Can't we assume non-active values for the rest if less than needed were provided? For more than that, why do we care?

Honestly, what good would it do? It would just be more confusing IMO.

Quoted 16 lines

Quoted 15 lines

drivers/gpio/gpio-virtuser.c new file +13 -0

+#include <linux/atomic.h>
+#include <linux/bitmap.h>
+#include <linux/cleanup.h>
+#include <linux/completion.h>
+#include <linux/configfs.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/machine.h>
 
+#include <linux/idr.h>
 
+#include <linux/interrupt.h>
+#include <linux/irq_work.h>

Quoted 3 lines

Quoted 1 line

+#include <linux/kernel.h>

Do you need this?

ARRAY_SIZE() used to live here when I first wrote this but it was since moved. I'll drop this.

Quoted 14 lines

Quoted 13 lines

drivers/gpio/gpio-virtuser.c new file +13 -0

+#include <linux/limits.h>
+#include <linux/list.h>
+#include <linux/lockdep.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/of.h>
+#include <linux/overflow.h>
+#include <linux/platform_device.h>
+#include <linux/printk.h>
+#include <linux/property.h>
+#include <linux/slab.h>

Quoted 3 lines

Quoted 1 line

+#include <linux/string.h>

Implied by string_helpers.h

Yeah, but we still use symbols directly from string.h, we shouldn't depend on implicit includes.

Quoted 3 lines

Quoted 3 lines

drivers/gpio/gpio-virtuser.c new file +3 -0

+#include <linux/string_helpers.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>

Quoted 9 lines

drivers/gpio/gpio-virtuser.c line 103

Quoted 6 lines

drivers/gpio/gpio-virtuser.c new file +6 -0

+struct gpio_virtuser_attr_descr {
+     const char *name;
+     ssize_t (*show)(struct device *, struct device_attribute *, char *);
+     ssize_t (*store)(struct device *, struct device_attribute *,
+                      const char *, size_t);
+};

struct device_attribute ? (Yes, I know that that one is a bit bigger but benefit is that we have some code that you may reuse)

Not sure what you mean here, these are callbacks for sysfs.

Quoted 5 lines

Quoted 5 lines

drivers/gpio/gpio-virtuser.c new file +5 -0

+static ssize_t gpio_virtuser_sysfs_emit_value_array(char *buf,
+                                                 unsigned long *values,
+                                                 size_t num_values)
+{
+     ssize_t len = 0;

Quoted 8 lines

drivers/gpio/gpio-virtuser.c line 150

Quoted 6 lines

drivers/gpio/gpio-virtuser.c new file +6 -0

+     size_t i;
+
+     for (i = 0; i < num_values; i++)
+             len += sysfs_emit_at(buf, len, "%d",
+                                  test_bit(i, values) ? 1 : 0);
+     return len + sysfs_emit_at(buf, len, "\n");

Why not use %pb?

Because it outputs hex? I want to output binary, can I do it?

Quoted 1 line

Quoted 1 line

+}

Quoted 9 lines

drivers/gpio/gpio-virtuser.c line 158

Quoted 6 lines

drivers/gpio/gpio-virtuser.c new file +6 -0

+static int gpio_virtuser_sysfs_parse_value_array(const char *buf, size_t len,
+                                              unsigned long *values)
+{
+     size_t i;
+
+     for (i = 0; i < len; i++) {

Perhaps

Quoted 4 lines

                bool val;
                int ret;

                ret = kstrtobool(...);

kstrtobool() accepts values we don't want here like [Tt]rue and [Ff]alse.

Quoted 12 lines

                if (ret)
                        return ret;

                assign_bit(...); // btw, why atomic?

Quoted 6 lines

drivers/gpio/gpio-virtuser.c new file +6 -0

+             if (buf[i] == '0')
+                     clear_bit(i, values);
+             else if (buf[i] == '1')
+                     set_bit(i, values);
+             else
+                     return -EINVAL;

Quoted 3 lines

Quoted 1 line

+ }

BUT, why not bitmap_parse()?

Because it parses hex, not binary.

Quoted 2 lines

Quoted 2 lines

(quoted diff) +2 -0

+     return 0;
+}

Quoted 6 lines

drivers/gpio/gpio-virtuser.c line 304

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+     return sysfs_emit(buf, "%s\n",
+                       dir == GPIO_LINE_DIRECTION_IN ? "input" : "output");

I think this maybe transformed to something like str_input_output() in string_choices.h (and you don't even need to include that as it's implied by string_helpers.h)

These helpers take bool as argument. Hard to tell whether input or output should correspond to true. I'd leave it as is.

Quoted 29 lines

Quoted 5 lines

drivers/gpio/gpio-virtuser.c new file +5 -0

+static int gpio_virtuser_parse_direction(const char *buf, int *dir, int *val)
+{
+     if (sysfs_streq(buf, "input")) {
+             *dir = GPIO_LINE_DIRECTION_IN;
+             return 0;

drivers/gpio/gpio-virtuser.c line 329

Quoted 10 lines

drivers/gpio/gpio-virtuser.c new file +10 -0

+     }
+
+     if (sysfs_streq(buf, "output-high"))
+             *val = 1;
+     else if (sysfs_streq(buf, "output-low"))
+             *val = 0;
+     else
+             return -EINVAL;
+
+     *dir = GPIO_LINE_DIRECTION_OUT;

This can be transformed to use sysfs_match_string() with

static const char * const dirs[] = { "output-low", "output-high", "input" };

        int ret;

        ret = sysfs_match_string(...);
        if (ret < 0)
                return ret;

        *val = ret;
        *dir = ret == 2 ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;

Quoted 2 lines

And with this approach it even not clear why do you need dir and val to be separated here (esp. if we add a enum like

We do want them to be separated not for better UX but to be able to test all kernel APIs (gpiod_direction_input|output() and gpiod_set_value()).

Quoted 8 lines

GPIO_VIRTUSER_OUT_LOW,
GPIO_VIRTUSER_OUT_HIGH,
GPIO_VIRTUSER_IN,

(with it the string array can also be indexed).

Quoted 2 lines

(quoted diff) +2 -0

+     return 0;
+}

Quoted 11 lines

Quoted 10 lines

drivers/gpio/gpio-virtuser.c new file +10 -0

+static int gpio_virtuser_parse_value(const char *buf)
+{
+     int value, ret;
+
+     value = sysfs_match_string(gpio_virtuser_sysfs_value_strings, buf);
+     if (value < 0) {
+             /* Can be 0 or 1 too. */
+             ret = kstrtoint(buf, 0, &value);
+             if (ret)
+                     return ret;

Quoted 4 lines

drivers/gpio/gpio-virtuser.c line 448

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+             if (value != 0 && value != 1)
+                     return -EINVAL;

Why not kstrtobool()?

I don't want to accept all the other strings kstrtobool() is fine with.

Quoted 4 lines

Quoted 4 lines

drivers/gpio/gpio-virtuser.c new file +4 -0

+     }
+
+     return value;
+}

Quoted 3 lines

Quoted 1 line

+ ret = kstrtouint(buf, 10, &debounce);

Why restrict to decimal?

Not sure what you gain from passing a period in hex?

Quoted 2 lines

Quoted 2 lines

(quoted diff) +2 -0

+     if (ret)
+             return ret;

Quoted 6 lines

Quoted 1 line

+ return dash && strcmp(dash, "-gpios") == 0;

Can't we reuse the suffix from the array from the gpiolib internal header? Also I don't like the form of '-' in the line. "gpios" is good and chance that linker deduplicates the same string if it occurs somewhere else in the binary (in case this goes with =y in .config).

I'm not sure I follow what you're saying here. Please rephrase.

Quoted 11 lines

Quoted 5 lines

+/*
+ * If this is an OF-based system, then we iterate over properties and consider
+ * all whose names end in "-gpios". For configfs we expect an additional string
+ * array property - "gpio-virtuser,ids" - containing the list of all GPIO IDs
+ * to request.

Why not any other system? What's wrong for having this available for ACPI, for example? Okay, I see that this is probably due to absence of API.

OTOH the last call in the function assumes non-OF cases. Why can't we have the same approach in both?

Again: I have no idea what you mean. We support device-tree and configfs as sources of configuration for these virtual consumers. If you want to add something more, be my guest once it's upstream.

The reason to use a different approach is to not require the "gpio-virtuser,ids" property in device-tree.

Quoted 1 line

Quoted 1 line

+ */

Quoted 4 lines

drivers/gpio/gpio-virtuser.c line 881

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+                     if (gpio_virtuser_prop_is_gpio(prop))
+                             ++ret;

Why pre-increment?

Why not?

Quoted 1 line

Quoted 1 line

+ dash = strpbrk(prop->name, "-");

Quoted 5 lines

Quoted 4 lines

drivers/gpio/gpio-virtuser.c new file +4 -0

+                     diff = dash - prop->name;
+
+                     tmp = devm_kmemdup(dev, prop->name, diff + 1,
+                                        GFP_KERNEL);

Quoted 2 lines

devm_kstrndup() is not okay? Okay, we don't have it (yet?), but at least I would rather expect wrapped kstrndup() than this.

Meh, this logic is fine as we know the range exactly. IMO kstrndup() here would be overkill. I'd leave it for now.

Quoted 4 lines

Quoted 4 lines

drivers/gpio/gpio-virtuser.c new file +3 -0

+                     if (!tmp)
+                             return -ENOMEM;
 
+                     tmp[diff] = '\0';

Quoted 3 lines

Quoted 1 line

+ int i = 0;

Why signed? And in all this kind of case, I would split assignment...

Quoted 4 lines

Quoted 4 lines

drivers/gpio/gpio-virtuser.c new file +4 -0

+     memset(properties, 0, sizeof(properties));
+
+     num_ids = list_count_nodes(&dev->lookup_list);
+     char **ids __free(kfree) = kcalloc(num_ids + 1, sizeof(*ids),

Quoted 8 lines

drivers/gpio/gpio-virtuser.c line 1478

Quoted 4 lines

drivers/gpio/gpio-virtuser.c new file +4 -0

+                                        GFP_KERNEL);
+     if (!ids)
+             return ERR_PTR(-ENOMEM);
+

To be here, that the reader will see immediately (close enough) what is the initial values. Moreover this code will be robuse against changes in between (if i become reusable).

Sorry, I can't parse it.

Quoted 2 lines

Quoted 2 lines

(quoted diff) +2 -0

+     list_for_each_entry(lookup, &dev->lookup_list, siblings)
+             ids[i++] = lookup->con_id;

Mon, Jun 10, 2024 at 03:22:32PM +0200, Bartosz Golaszewski kirjoitti:

Quoted 3 lines

Quoted 1 line · Andy Shevchenko

Mon, May 27, 2024 at 04:40:54PM +0200, Bartosz Golaszewski kirjoitti:

...

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

User must pass exactly the number of values that the array contains

Quoted 4 lines

Quoted 2 lines

Can't we assume non-active values for the rest if less than needed were provided? For more than that, why do we care?

Honestly, what good would it do? It would just be more confusing IMO.

Let's say you can leave documentation as is, but relax the code. That's the benefit, less complex checks in the code.

...

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

drivers/gpio/gpio-virtuser.c new file +13 -0

+#include <linux/atomic.h>
+#include <linux/bitmap.h>
+#include <linux/cleanup.h>
+#include <linux/completion.h>
+#include <linux/configfs.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/machine.h>
 
+#include <linux/idr.h>
 
+#include <linux/interrupt.h>
+#include <linux/irq_work.h>

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

+#include <linux/kernel.h>

Do you need this?

ARRAY_SIZE() used to live here when I first wrote this but it was since moved. I'll drop this.

Rather replace with array_size.h.

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

drivers/gpio/gpio-virtuser.c new file +14 -0

+#include <linux/limits.h>
+#include <linux/list.h>
+#include <linux/lockdep.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/of.h>
+#include <linux/overflow.h>
+#include <linux/platform_device.h>
+#include <linux/printk.h>
+#include <linux/property.h>
+#include <linux/slab.h>
 
+#include <linux/string.h>

Quoted 4 lines

Quoted 1 line

Implied by string_helpers.h

Yeah, but we still use symbols directly from string.h, we shouldn't depend on implicit includes.

string_helpers.h is and will continue guranteening inclusion of string.h. It's the same as we drop bits.h when we include, for instance, bitmap.h.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

drivers/gpio/gpio-virtuser.c new file +3 -0

+#include <linux/string_helpers.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>

...

Quoted 10 lines

Quoted 9 lines

drivers/gpio/gpio-virtuser.c line 103

Quoted 6 lines

drivers/gpio/gpio-virtuser.c new file +6 -0

+struct gpio_virtuser_attr_descr {
+     const char *name;
+     ssize_t (*show)(struct device *, struct device_attribute *, char *);
+     ssize_t (*store)(struct device *, struct device_attribute *,
+                      const char *, size_t);
+};

struct device_attribute ? (Yes, I know that that one is a bit bigger but benefit is that we have some code that you may reuse)

Quoted 1 line

Not sure what you mean here, these are callbacks for sysfs.

I mean to replace your custom data type with the existing device_attribute.

...

Quoted 14 lines

Quoted 13 lines

drivers/gpio/gpio-virtuser.c line 150

Quoted 11 lines

drivers/gpio/gpio-virtuser.c new file +11 -0

+static ssize_t gpio_virtuser_sysfs_emit_value_array(char *buf,
+                                                 unsigned long *values,
+                                                 size_t num_values)
+{
+     ssize_t len = 0;
+     size_t i;
+
+     for (i = 0; i < num_values; i++)
+             len += sysfs_emit_at(buf, len, "%d",
+                                  test_bit(i, values) ? 1 : 0);
+     return len + sysfs_emit_at(buf, len, "\n");

Why not use %pb?

Quoted 1 line

Because it outputs hex? I want to output binary, can I do it?

But why do you need that? You can also print a list of numbers of bits that set (%pbl).

We have a few ABIs in the kernel that works nice and people are familiar with (CPU sets, IRQ affinity masks, etc). Why to reinvent the wheel?

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

+}

...

Quoted 9 lines

Quoted 9 lines

drivers/gpio/gpio-virtuser.c line 158

Quoted 6 lines

drivers/gpio/gpio-virtuser.c new file +6 -0

+static int gpio_virtuser_sysfs_parse_value_array(const char *buf, size_t len,
+                                              unsigned long *values)
+{
+     size_t i;
+
+     for (i = 0; i < len; i++) {

Perhaps

Quoted 6 lines

Quoted 4 lines

                bool val;
                int ret;

                ret = kstrtobool(...);

kstrtobool() accepts values we don't want here like [Tt]rue and [Ff]alse.

Yes, see below.

Quoted 12 lines

Quoted 12 lines

                if (ret)
                        return ret;

                assign_bit(...); // btw, why atomic?

Quoted 6 lines

drivers/gpio/gpio-virtuser.c new file +6 -0

+             if (buf[i] == '0')
+                     clear_bit(i, values);
+             else if (buf[i] == '1')
+                     set_bit(i, values);
+             else
+                     return -EINVAL;

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+ }

BUT, why not bitmap_parse()?

Because it parses hex, not binary.

So, why do we reinvent a wheel? Wouldn't be better that users may apply the knowledge they familiar with (and I believe the group of the users who know about bitmaps is much bigger than those who will use this driver).

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

(quoted diff) +2 -0

+     return 0;
+}

...

Quoted 9 lines

Quoted 6 lines

drivers/gpio/gpio-virtuser.c line 304

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+     return sysfs_emit(buf, "%s\n",
+                       dir == GPIO_LINE_DIRECTION_IN ? "input" : "output");

I think this maybe transformed to something like str_input_output() in string_choices.h (and you don't even need to include that as it's implied by string_helpers.h)

These helpers take bool as argument. Hard to tell whether input or output should correspond to true. I'd leave it as is.

There is a convention: str_TRUE_FALSE().

...

Quoted 29 lines

Quoted 29 lines

Quoted 5 lines

drivers/gpio/gpio-virtuser.c new file +5 -0

+static int gpio_virtuser_parse_direction(const char *buf, int *dir, int *val)
+{
+     if (sysfs_streq(buf, "input")) {
+             *dir = GPIO_LINE_DIRECTION_IN;
+             return 0;

drivers/gpio/gpio-virtuser.c line 329

Quoted 10 lines

drivers/gpio/gpio-virtuser.c new file +10 -0

+     }
+
+     if (sysfs_streq(buf, "output-high"))
+             *val = 1;
+     else if (sysfs_streq(buf, "output-low"))
+             *val = 0;
+     else
+             return -EINVAL;
+
+     *dir = GPIO_LINE_DIRECTION_OUT;

This can be transformed to use sysfs_match_string() with

static const char * const dirs[] = { "output-low", "output-high", "input" };

        int ret;

        ret = sysfs_match_string(...);
        if (ret < 0)
                return ret;

        *val = ret;
        *dir = ret == 2 ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;

Quoted 6 lines

Quoted 2 lines

And with this approach it even not clear why do you need dir and val to be separated here (esp. if we add a enum like

We do want them to be separated not for better UX but to be able to test all kernel APIs (gpiod_direction_input|output() and gpiod_set_value()).

Still you can do some optimisations I proposed above.

Quoted 8 lines

Quoted 8 lines

GPIO_VIRTUSER_OUT_LOW,
GPIO_VIRTUSER_OUT_HIGH,
GPIO_VIRTUSER_IN,

(with it the string array can also be indexed).

Quoted 2 lines

(quoted diff) +2 -0

+     return 0;
+}

...

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

drivers/gpio/gpio-virtuser.c new file +10 -0

+static int gpio_virtuser_parse_value(const char *buf)
+{
+     int value, ret;
+
+     value = sysfs_match_string(gpio_virtuser_sysfs_value_strings, buf);
+     if (value < 0) {
+             /* Can be 0 or 1 too. */
+             ret = kstrtoint(buf, 0, &value);
+             if (ret)
+                     return ret;

Quoted 6 lines

Quoted 4 lines

drivers/gpio/gpio-virtuser.c line 448

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+             if (value != 0 && value != 1)
+                     return -EINVAL;

Why not kstrtobool()?

I don't want to accept all the other strings kstrtobool() is fine with.

What's wrong with other strings?

At bare minumum you can reduce the range by using kstrtou8().

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/gpio/gpio-virtuser.c new file +4 -0

+     }
+
+     return value;
+}

...

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

+ ret = kstrtouint(buf, 10, &debounce);

Quoted 3 lines

Quoted 1 line

Why restrict to decimal?

Not sure what you gain from passing a period in hex?

For example, if I compare this to the real HW, I might be able to do something like 0x1234 (let's say it's debounce step) and shifting it by 4 bits will give me something I want. But despite that quite unlikely case the restriction here doesn't bring us much.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

(quoted diff) +2 -0

+     if (ret)
+             return ret;

...

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

+ return dash && strcmp(dash, "-gpios") == 0;

Quoted 6 lines

Quoted 4 lines

Can't we reuse the suffix from the array from the gpiolib internal header? Also I don't like the form of '-' in the line. "gpios" is good and chance that linker deduplicates the same string if it occurs somewhere else in the binary (in case this goes with =y in .config).

I'm not sure I follow what you're saying here. Please rephrase.

Do strcmp() against one from the gpio_suffixes array.

...

Quoted 12 lines

Quoted 11 lines

Quoted 5 lines

+/*
+ * If this is an OF-based system, then we iterate over properties and consider
+ * all whose names end in "-gpios". For configfs we expect an additional string
+ * array property - "gpio-virtuser,ids" - containing the list of all GPIO IDs
+ * to request.

Why not any other system? What's wrong for having this available for ACPI, for example? Okay, I see that this is probably due to absence of API.

OTOH the last call in the function assumes non-OF cases. Why can't we have the same approach in both?

Quoted 6 lines

Again: I have no idea what you mean. We support device-tree and configfs as sources of configuration for these virtual consumers. If you want to add something more, be my guest once it's upstream.

The reason to use a different approach is to not require the "gpio-virtuser,ids" property in device-tree.

Yes, and I'm asking why can't we unify and require it there as well? But okay, I might give up on the trying of the DT/ACPI property unification.

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ */

...

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

drivers/gpio/gpio-virtuser.c new file +2 -0

+                     if (gpio_virtuser_prop_is_gpio(prop))
+                             ++ret;

Quoted 3 lines

Quoted 1 line

Why pre-increment?

Why not?

Because we have a pattern. Pre-increment adds into additional questioning "why?". I.e. What does make this case special? When I read such a code I need more brain power to parse it.

...

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ dash = strpbrk(prop->name, "-");

Btw, don't you want strrchr() here? (Note 'r' variant).

Quoted 12 lines

Quoted 8 lines

drivers/gpio/gpio-virtuser.c line 907

Quoted 5 lines

drivers/gpio/gpio-virtuser.c new file +4 -0

+                     diff = dash - prop->name;
+
+                     tmp = devm_kmemdup(dev, prop->name, diff + 1,
+                                        GFP_KERNEL);

devm_kstrndup() is not okay? Okay, we don't have it (yet?), but at least I would rather expect wrapped kstrndup() than this.

Meh, this logic is fine as we know the range exactly. IMO kstrndup() here would be overkill. I'd leave it for now.

Quoted 4 lines

Quoted 4 lines

drivers/gpio/gpio-virtuser.c line 911

Quoted 4 lines

drivers/gpio/gpio-virtuser.c new file +3 -0

+                     if (!tmp)
+                             return -ENOMEM;
 
+                     tmp[diff] = '\0';

This line will gone with kstrndup(). I think we will benefit from it.

...

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

+ int i = 0;

Why signed? And in all this kind of case, I would split assignment...

(1)

Quoted 13 lines

Quoted 12 lines

drivers/gpio/gpio-virtuser.c line 1478

Quoted 8 lines

drivers/gpio/gpio-virtuser.c new file +8 -0

+     memset(properties, 0, sizeof(properties));
+
+     num_ids = list_count_nodes(&dev->lookup_list);
+     char **ids __free(kfree) = kcalloc(num_ids + 1, sizeof(*ids),
+                                        GFP_KERNEL);
+     if (!ids)
+             return ERR_PTR(-ENOMEM);
+

To be here, that the reader will see immediately (close enough) what is the initial values. Moreover this code will be robuse against changes in between (if i become reusable).

Quoted 1 line

Sorry, I can't parse it.

I meant to see here

        i = 0;

instead of the above (1).

Quoted 2 lines

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+     list_for_each_entry(lookup, &dev->lookup_list, siblings)
+             ids[i++] = lookup->con_id;

--
With Best Regards,
Andy Shevchenko

Quoted 4 lines · Andy Shevchenko

Mon, Jun 10, 2024 at 03:22:32PM +0200, Bartosz Golaszewski kirjoitti:

Quoted 3 lines

Quoted 1 line · Andy Shevchenko

Mon, May 27, 2024 at 04:40:54PM +0200, Bartosz Golaszewski kirjoitti:

Quoted 16 lines

Quoted 15 lines

Quoted 5 lines

Quoted 5 lines

drivers/gpio/gpio-virtuser.c new file +5 -0

+static ssize_t gpio_virtuser_sysfs_emit_value_array(char *buf,
+                                                 unsigned long *values,
+                                                 size_t num_values)
+{
+     ssize_t len = 0;

Quoted 8 lines

drivers/gpio/gpio-virtuser.c line 150

Quoted 6 lines

drivers/gpio/gpio-virtuser.c new file +6 -0

+     size_t i;
+
+     for (i = 0; i < num_values; i++)
+             len += sysfs_emit_at(buf, len, "%d",
+                                  test_bit(i, values) ? 1 : 0);
+     return len + sysfs_emit_at(buf, len, "\n");

Why not use %pb?

Because it outputs hex? I want to output binary, can I do it?

Quoted 5 lines

But why do you need that? You can also print a list of numbers of bits that set (%pbl).

We have a few ABIs in the kernel that works nice and people are familiar with (CPU sets, IRQ affinity masks, etc). Why to reinvent the wheel?

If I see "11001011" as output, I can immediately convert that to pins in my head. If I see 0xcb, I need to use a calculator.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

+}

Quoted 32 lines

Quoted 9 lines

Quoted 9 lines

drivers/gpio/gpio-virtuser.c line 158

Quoted 6 lines

drivers/gpio/gpio-virtuser.c new file +6 -0

+static int gpio_virtuser_sysfs_parse_value_array(const char *buf, size_t len,
+                                              unsigned long *values)
+{
+     size_t i;
+
+     for (i = 0; i < len; i++) {

Perhaps

Quoted 6 lines

Quoted 4 lines

                bool val;
                int ret;

                ret = kstrtobool(...);

kstrtobool() accepts values we don't want here like [Tt]rue and [Ff]alse.

Yes, see below.

Quoted 14 lines

Quoted 14 lines

                if (ret)
                        return ret;

                assign_bit(...); // btw, why atomic?

Quoted 8 lines

drivers/gpio/gpio-virtuser.c new file +7 -0

+             if (buf[i] == '0')
+                     clear_bit(i, values);
+             else if (buf[i] == '1')
+                     set_bit(i, values);
+             else
+                     return -EINVAL;
 
+     }

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

BUT, why not bitmap_parse()?

Because it parses hex, not binary.

So, why do we reinvent a wheel? Wouldn't be better that users may apply the knowledge they are familiar with (and I believe the group of the users who know about bitmaps is much bigger than those who will use this driver).

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

(quoted diff) +2 -0

+     return 0;
+}

Quoted 1 line

At bare minumum you can reduce the range by using kstrtou8().

As opposed to just checking '0' and '1'? Meh...

Quoted 21 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

+ int i = 0;

Why signed? And in all this kind of case, I would split assignment...

(1)

Quoted 14 lines

Quoted 4 lines

Quoted 4 lines

drivers/gpio/gpio-virtuser.c new file +4 -0

+     memset(properties, 0, sizeof(properties));
+
+     num_ids = list_count_nodes(&dev->lookup_list);
+     char **ids __free(kfree) = kcalloc(num_ids + 1, sizeof(*ids),

Quoted 8 lines

drivers/gpio/gpio-virtuser.c line 1478

Quoted 4 lines

drivers/gpio/gpio-virtuser.c new file +4 -0

+                                        GFP_KERNEL);
+     if (!ids)
+             return ERR_PTR(-ENOMEM);
+

To be here, that the reader will see immediately (close enough) what is the initial values. Moreover this code will be robuse against changes in between (if i become reusable).

Sorry, I can't parse it.

Quoted 5 lines

I meant to see here

        i = 0;

instead of the above (1).

Why? I see no good reason.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

(quoted diff) +2 -0

+     list_for_each_entry(lookup, &dev->lookup_list, siblings)
+             ids[i++] = lookup->con_id;