gpio: consumer: new virtual driver
RFC PATCH 1/1 gpio: consumer: new virtual driver
From: Bartosz Golaszewski <[email protected]>
The GPIO subsystem has 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).
The configfs interface allows users to create dynamic GPIO lookup tables that are registered with the GPIO subsystem. Every config group represents a consumer device. Every sub-group represents a single GPIO lookup. The device can work in three modes: just keeping the line active, toggling it every second or requesting its interrupt and reporting edges. Every lookup allows to specify the key, offset and flags as per the lookup struct defined in linux/gpio/machine.h.
The module together with gpio-sim allows to easily trigger kernel hot-unplug errors. A simple use-case is to create a simulated chip, setup the consumer to lookup one of its lines in 'monitor' mode, unbind the simulator, unbind the consumer and observe the fireworks in dmesg.
This driver is aimed as a helper in tackling the hot-unplug problem in GPIO as well as basis for future regression testing once the fixes are upstream.
--- drivers/gpio/Kconfig | 7 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-consumer.c | 1004 ++++++++++++++++++++++++++++++++++ 3 files changed, 1012 insertions(+) create mode 100644 drivers/gpio/gpio-consumer.c
drivers/gpio/Kconfig +7 -0
@@ -1796,6 +1796,13 @@ config GPIO_SIM This enables the GPIO simulator - a configfs-based GPIO testing driver. +config GPIO_CONSUMER + tristate "GPIO Consumer Testing Module" + select CONFIGFS_FS + help + This enables the configurable, configfs-based virtual GPIO consumer + testing driver. + endmenu endif
drivers/gpio/Makefile +1 -0
@@ -46,6 +46,7 @@ obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o obj-$(CONFIG_GPIO_CADENCE) += gpio-cadence.o obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o obj-$(CONFIG_GPIO_SNPS_CREG) += gpio-creg-snps.o +obj-$(CONFIG_GPIO_CONSUMER) += gpio-consumer.o obj-$(CONFIG_GPIO_CRYSTAL_COVE) += gpio-crystalcove.o obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o
drivers/gpio/gpio-consumer.c new file +1004 -0
1,004 lines not on this page · raw message
Signature · 1 line
2.39.2
Quoted 31 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
The GPIO subsystem has 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).
The configfs interface allows users to create dynamic GPIO lookup tables that are registered with the GPIO subsystem. Every config group represents a consumer device. Every sub-group represents a single GPIO lookup. The device can work in three modes: just keeping the line active, toggling it every second or requesting its interrupt and reporting edges. Every lookup allows to specify the key, offset and flags as per the lookup struct defined in
linux/gpio/machine.h.The module together with gpio-sim allows to easily trigger kernel hot-unplug errors. A simple use-case is to create a simulated chip, setup the consumer to lookup one of its lines in 'monitor' mode, unbind the simulator, unbind the consumer and observe the fireworks in dmesg.
Quoted 5 lines
This driver is aimed as a helper in tackling the hot-unplug problem in GPIO as well as basis for future regression testing once the fixes are upstream.
Signed-off-by Bartosz Golaszewski <[email protected]>
Anyone can easily see how this will benefit testing, so:
drivers/gpio/Kconfig config GPIO_SIM · line 1804
Quoted 6 lines
drivers/gpio/Kconfig +6
-0+config GPIO_CONSUMER + tristate "GPIO Consumer Testing Module" + select CONFIGFS_FS + help + This enables the configurable, configfs-based virtual GPIO consumer + testing driver.
Since this is only for debugging, should it depend on DEBUG?
Yours,
Linus Walleij
Quoted 34 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
The GPIO subsystem has 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).
The configfs interface allows users to create dynamic GPIO lookup tables that are registered with the GPIO subsystem. Every config group represents a consumer device. Every sub-group represents a single GPIO lookup. The device can work in three modes: just keeping the line active, toggling it every second or requesting its interrupt and reporting edges. Every lookup allows to specify the key, offset and flags as per the lookup struct defined in
linux/gpio/machine.h.The module together with gpio-sim allows to easily trigger kernel hot-unplug errors. A simple use-case is to create a simulated chip, setup the consumer to lookup one of its lines in 'monitor' mode, unbind the simulator, unbind the consumer and observe the fireworks in dmesg.
This driver is aimed as a helper in tackling the hot-unplug problem in GPIO as well as basis for future regression testing once the fixes are upstream.
...
drivers/gpio/Kconfig config GPIO_SIM · line 1804
Quoted 10 lines
drivers/gpio/Kconfig +6
-0@@ -1796,6 +1796,13 @@ config GPIO_SIM This enables the GPIO simulator - a configfs-based GPIO testing driver. +config GPIO_CONSUMER + tristate "GPIO Consumer Testing Module" + select CONFIGFS_FS + help + This enables the configurable, configfs-based virtual GPIO consumer + testing driver.
I believe the agreement (as mentioned in this file) is to keep sorted by config names, so CONSUMER is definitely should be earlier than SIM.
...
Quoted 6 lines
drivers/gpio/gpio-consumer.c new file +6
-0+#include <linux/completion.h> +#include <linux/configfs.h> +#include <linux/device.h> +#include <linux/gpio/consumer.h> +#include <linux/gpio/machine.h> +#include <linux/idr.h>
drivers/gpio/gpio-consumer.c line 24
Quoted 10 lines
drivers/gpio/gpio-consumer.c new file +9
-0+#include <linux/interrupt.h> +#include <linux/jiffies.h> +#include <linux/kernel.h> +#include <linux/limits.h> +#include <linux/list.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/notifier.h> +#include <linux/of_platform.h>
Wrong header. Use mod_devicetable.h.
drivers/gpio/gpio-consumer.c line 30
Quoted 6 lines
drivers/gpio/gpio-consumer.c new file +6
-0+#include <linux/platform_device.h> +#include <linux/printk.h> +#include <linux/property.h> +#include <linux/slab.h> +#include <linux/string.h> +#include <linux/timer.h>
And general recommendation is to revisit this block and refine it accordingly.
...
drivers/gpio/gpio-consumer.c line 37
Quoted 3 lines
drivers/gpio/gpio-consumer.c new file +2
-0+enum gpio_consumer_function { +GPIO_CONSUMER_FUNCTION_ACTIVE =0 ,
= 0 is guaranteed by C standard, why do you need it be explicit?
drivers/gpio/gpio-consumer.c line 40
Quoted 3 lines
drivers/gpio/gpio-consumer.c new file +3
-0+GPIO_CONSUMER_FUNCTION_TOGGLE , +GPIO_CONSUMER_FUNCTION_MONITOR , + __GPIO_CONSUMER_FUNCTION_LAST,
No comma in terminator line.
Quoted 1 line
+};
...
drivers/gpio/gpio-consumer.c line 92
Quoted 5 lines
drivers/gpio/gpio-consumer.c new file +4
-0+static void gpio_consumer_on_timer (struct timer_list *timer) +{ +struct gpio_consumer_timer_data *timer_data =to_timer_data (timer); + timer_data->val = timer_data->val ==0 ?1 :0 ;
timer_data->val = timer_data->val ? 0 : 1;
is shorter, but what is showing better the intention is
timer_data->val ^= 1;
Quoted 3 lines
drivers/gpio/gpio-consumer.c new file +3
-0+gpiod_set_value_cansleep (timer_data->desc, timer_data->val); +mod_timer (&timer_data->timer, jiffies +msecs_to_jiffies (1000 )); +}
...
drivers/gpio/gpio-consumer.c line 123
Quoted 3 lines
drivers/gpio/gpio-consumer.c new file +3
-0+ ret =match_string (gpio_consumer_function_strings,-1 , function_prop); +if (ret <0 ) + return dev_err_probe(dev, -EINVAL,
Why not
return dev_err_probe(dev, ret,
?
Quoted 2 lines
drivers/gpio/gpio-consumer.c new file +2
-0+"Invalid consumer function: '%s'\n" , + function_prop);
...
drivers/gpio/gpio-consumer.c line 152
Quoted 8 lines
drivers/gpio/gpio-consumer.c new file +8
-0+ flags = function ==GPIO_CONSUMER_FUNCTION_MONITOR ? +GPIOD_IN :GPIOD_OUT_HIGH ; +for (i =0 ; i < num_lines; i++) { + desc =devm_gpiod_get (dev, lines[i], flags); +if (IS_ERR (desc)) +return dev_err_probe (dev,PTR_ERR (desc), +"Failed to get GPIO '%s'\n" , + lines[i]);
Would it make sense to request GPIOs via devm_gpiod_get_array() and then try the rest on them in a loop?
Quoted 1 line
(quoted diff) +1
-0+ }
...
drivers/gpio/gpio-consumer.c line 240
Quoted 12 lines
drivers/gpio/gpio-consumer.c new file +11
-0+static int gpio_consumer_bus_notifier_call (struct notifier_block *nb, +unsigned long action,void *data) +{ +struct gpio_consumer_device *consumer; +struct device *dev = data; +char devname[32 ]; + + consumer = container_of(nb, struct gpio_consumer_device, bus_notifier); +snprintf (devname,sizeof (devname),"gpio-virtual-consumer.%d" , + consumer->id); +if (strcmp (dev_name (dev), devname) ==0 ) {
if (strcmp(dev_name(dev), devname)) return NOTIFY_DONE;
?
Quoted 17 lines
drivers/gpio/gpio-consumer.c new file +17
-0+switch (action) { +case BUS_NOTIFY_BOUND_DRIVER : + consumer->driver_bound = true; +break ; +case BUS_NOTIFY_DRIVER_NOT_BOUND : + consumer->driver_bound = false; +break ; +default : +return NOTIFY_DONE ; + } + +complete (&consumer->probe_completion); +return NOTIFY_OK ; + } + +return NOTIFY_DONE ; +}
...
Quoted 5 lines
drivers/gpio/gpio-consumer.c new file +5
-0+static ssize_t +gpio_consumer_lookup_config_offset_store (struct config_item *item, +const char *page,size_t count) +{ +struct gpio_consumer_lookup *lookup =to_gpio_consumer_lookup (item);
drivers/gpio/gpio-consumer.c line 370
Quoted 10 lines
drivers/gpio/gpio-consumer.c new file +10
-0+struct gpio_consumer_device *dev = lookup->parent; +int offset, ret; + + ret =kstrtoint (page,0 , &offset); +if (ret) +return ret; + +/* Use -1 to indicate lookup by name. */ +if (offset > (U16_MAX -1 )) +return -EINVAL ;
So, offset here may be negative. Is it okay?
Quoted 13 lines
drivers/gpio/gpio-consumer.c new file +13
-0+mutex_lock (&dev->lock); + +if (gpio_consumer_device_is_live_unlocked (dev)) { +mutex_unlock (&dev->lock); +return -EBUSY ; + } + + lookup->offset = offset; + +mutex_unlock (&dev->lock); + +return count; +}
...
drivers/gpio/gpio-consumer.c line 413
Quoted 4 lines
drivers/gpio/gpio-consumer.c new file +4
-0+if (flags &GPIO_OPEN_DRAIN ) + repr ="open-drain" ; +else if (flags &GPIO_OPEN_SOURCE ) + repr ="open-source" ;
Can it be both flags set?
Quoted 2 lines
drivers/gpio/gpio-consumer.c new file +2
-0+else + repr ="push-pull" ;
...
drivers/gpio/gpio-consumer.c line 444
Quoted 11 lines
drivers/gpio/gpio-consumer.c new file +11
-0+if (sysfs_streq (page,"push-pull" )) { + lookup->flags &= ~(GPIO_OPEN_DRAIN |GPIO_OPEN_SOURCE ); + }else if (sysfs_streq (page,"open-drain" )) { + lookup->flags &= ~GPIO_OPEN_SOURCE ; + lookup->flags |=GPIO_OPEN_DRAIN ; + }else if (sysfs_streq (page,"open-source" )) { + lookup->flags &= ~GPIO_OPEN_DRAIN ; + lookup->flags |=GPIO_OPEN_SOURCE ; + }else { + count = -EINVAL ; + }
I prefer to see some kind of the array of constant string literals and do sysfs_match_string() here
lookup->flags &= ~(GPIO_OPEN_DRAIN | GPIO_OPEN_SOURCE); flag = sysfs_match_string(...); if (flag < 0) count = flag else lookup->flags |= flag;
(or something similar). And respectively indexed access above.
...
Quoted 10 lines
drivers/gpio/gpio-consumer.c new file +10
-0+ flags =gpio_consumer_lookup_get_flags (item); + +if (flags &GPIO_PULL_UP ) + repr ="pull-up" ; +else if (flags &GPIO_PULL_DOWN ) + repr ="pull-down" ; +else if (flags &GPIO_PULL_DISABLE ) + repr ="pull-disabled" ; +else + repr ="as-is" ;
...
Quoted 5 lines
drivers/gpio/gpio-consumer.c new file +5
-0+if (sysfs_streq (page,"pull-up" )) { + lookup->flags &= ~(GPIO_PULL_DOWN |GPIO_PULL_DISABLE ); + lookup->flags |=GPIO_PULL_UP ; + }else if (sysfs_streq (page,"pull-down" )) { + lookup->flags &= ~(GPIO_PULL_UP |GPIO_PULL_DISABLE );
drivers/gpio/gpio-consumer.c line 501
Quoted 10 lines
drivers/gpio/gpio-consumer.c new file +10
-0+ lookup->flags |=GPIO_PULL_DOWN ; + }else if (sysfs_streq (page,"pull-disabled" )) { + lookup->flags &= ~(GPIO_PULL_UP |GPIO_PULL_DOWN ); + lookup->flags |=GPIO_PULL_DISABLE ; + }else if (sysfs_streq (page,"as-is" )) { + lookup->flags &= ~(GPIO_PULL_UP |GPIO_PULL_DOWN | +GPIO_PULL_DISABLE ); + }else { + count = -EINVAL ; + }
The above is more tricky, but still consider similar approach.
...
drivers/gpio/gpio-consumer.c line 629
Quoted 2 lines
drivers/gpio/gpio-consumer.c new file +2
-0+ num_entries =list_count_nodes (&dev->lookup_list); + table = kzalloc(sizeof(*table) + num_entries * sizeof(*table->table),
struct_size() ?
Quoted 3 lines
drivers/gpio/gpio-consumer.c new file +3
-0+ GFP_KERNEL); +if (!table) +return -ENOMEM ;
...
drivers/gpio/gpio-consumer.c line 698
Quoted 2 lines
drivers/gpio/gpio-consumer.c new file +2
-0+if (list_empty (&dev->lookup_list)) +return -ENODATA ;
Instead you may count nodes here and if 0, return an error, otherwise pass it to the callee.
Quoted 3 lines
drivers/gpio/gpio-consumer.c new file +3
-0+ swnode =gpio_consumer_make_device_swnode (dev); +if (IS_ERR (swnode)) +return PTR_ERR (swnode);
...
Quoted 6 lines
drivers/gpio/gpio-consumer.c new file +6
-0+static ssize_t +gpio_consumer_device_config_live_store (struct config_item *item, +const char *page,size_t count) +{ +struct gpio_consumer_device *dev =to_gpio_consumer_device (item); +bool live;
drivers/gpio/gpio-consumer.c line 767
Quoted 10 lines
drivers/gpio/gpio-consumer.c new file +10
-0+int ret; + + ret =kstrtobool (page, &live); +if (ret) +return ret; + +mutex_lock (&dev->lock); + +if ((!live && !gpio_consumer_device_is_live_unlocked (dev)) || + (live &&gpio_consumer_device_is_live_unlocked (dev)))
if (live ^ gpio_consumer_device_is_live_unlocked(dev))
?
Quoted 10 lines
drivers/gpio/gpio-consumer.c new file +10
-0+ ret = -EPERM ; + else if (live) + ret =gpio_consumer_device_activate_unlocked (dev); + else +gpio_consumer_device_deactivate_unlocked (dev ); + +mutex_unlock (&dev->lock); + +return ret ?: count; +}
...
drivers/gpio/gpio-consumer.c line 973
Quoted 7 lines
drivers/gpio/gpio-consumer.c new file +7
-0+static int __init gpio_consumer_init (void ) +{ +int ret; + + ret =platform_driver_register (&gpio_consumer_driver); +if (ret) { + pr_err("Failed to register the gpio-consumer platform driver: %d\n",
Isn't name will be duplicated via pr_fmt()?
Review
Quoted 1 line
(quoted diff) +1
-0+ ret);
If yes, can be shortened to
pr_err("Failed to register the driver: %d\n", ret);Quoted 17 lines
drivers/gpio/gpio-consumer.c new file +17
-0+return ret; + } + +config_group_init (&gpio_consumer_config_subsys.su_group); +mutex_init (&gpio_consumer_config_subsys.su_mutex); + ret =configfs_register_subsystem (&gpio_consumer_config_subsys); +if (ret) { +pr_err ("Failed to register the '%s' configfs subsystem: %d\n" , + gpio_consumer_config_subsys.su_group.cg_item.ci_namebuf, + ret); +mutex_destroy (&gpio_consumer_config_subsys.su_mutex); +platform_driver_unregister (&gpio_consumer_driver); +return ret; + } + +return 0 ; +}
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 1 line · Andy Shevchenko
[snip]
Quoted 5 lines
Quoted 2 lines
+#include <
linux/of_platform.h>Wrong header. Use
mod_devicetable.h.
Quoted 9 lines
drivers/gpio/gpio-consumer.c line 30
Quoted 6 lines
drivers/gpio/gpio-consumer.c new file +6
-0+#include <linux/platform_device.h> +#include <linux/printk.h> +#include <linux/property.h> +#include <linux/slab.h> +#include <linux/string.h> +#include <linux/timer.h> And general recommendation is to revisit this block and refine it accordingly.
I kept track of the interfaces I used for most part, so it should be mostly fine.
[snip]
Quoted 11 lines
...
drivers/gpio/gpio-consumer.c line 152
Quoted 8 lines
drivers/gpio/gpio-consumer.c new file +8
-0+ flags = function ==GPIO_CONSUMER_FUNCTION_MONITOR ? +GPIOD_IN :GPIOD_OUT_HIGH ; +for (i =0 ; i < num_lines; i++) { + desc =devm_gpiod_get (dev, lines[i], flags); +if (IS_ERR (desc)) +return dev_err_probe (dev,PTR_ERR (desc), +"Failed to get GPIO '%s'\n" , + lines[i]);
Quoted 3 lines
Would it make sense to request GPIOs via
devm_gpiod_get_array()and then try the rest on them in a loop?
No it would not. gpiod_get_array() works for properties represented in DT as:
foo-gpios = <&chip ...>, <&chip ...>, <&chip ...>;
while what we have here is:
foo-gpios = <&chip ...>;
bar-gpios = <&chip ...>;Which makes me think that I need to add proper documentation for this module.
[snip]
Quoted 10 lines
Quoted 10 lines
drivers/gpio/gpio-consumer.c new file +9
-0+static ssize_t +gpio_consumer_lookup_config_offset_store (struct config_item *item, +const char *page,size_t count) +{ +struct gpio_consumer_lookup *lookup =to_gpio_consumer_lookup (item); +struct gpio_consumer_device *dev = lookup->parent; +int offset, ret; + + ret =kstrtoint (page,0 , &offset);
Quoted 9 lines
drivers/gpio/gpio-consumer.c line 370
Quoted 6 lines
drivers/gpio/gpio-consumer.c new file +6
-0+if (ret) +return ret; + +/* Use -1 to indicate lookup by name. */ +if (offset > (U16_MAX -1 )) +return -EINVAL ;So, offset here may be negative. Is it okay?
Yes. If negative - lookup line by name, if positive, by chip and offset. I will document this properly for v2.
Quoted 16 lines
Quoted 13 lines
drivers/gpio/gpio-consumer.c new file +13
-0+mutex_lock (&dev->lock); + +if (gpio_consumer_device_is_live_unlocked (dev)) { +mutex_unlock (&dev->lock); +return -EBUSY ; + } + + lookup->offset = offset; + +mutex_unlock (&dev->lock); + +return count; +}...
Quoted 7 lines
drivers/gpio/gpio-consumer.c line 413
Quoted 4 lines
drivers/gpio/gpio-consumer.c new file +4
-0+if (flags &GPIO_OPEN_DRAIN ) + repr ="open-drain" ; +else if (flags &GPIO_OPEN_SOURCE ) + repr ="open-source" ;Can it be both flags set?
No!
Quoted 17 lines
Quoted 2 lines
drivers/gpio/gpio-consumer.c new file +2
-0+else + repr ="push-pull" ;...
drivers/gpio/gpio-consumer.c line 444
Quoted 11 lines
drivers/gpio/gpio-consumer.c new file +11
-0+if (sysfs_streq (page,"push-pull" )) { + lookup->flags &= ~(GPIO_OPEN_DRAIN |GPIO_OPEN_SOURCE ); + }else if (sysfs_streq (page,"open-drain" )) { + lookup->flags &= ~GPIO_OPEN_SOURCE ; + lookup->flags |=GPIO_OPEN_DRAIN ; + }else if (sysfs_streq (page,"open-source" )) { + lookup->flags &= ~GPIO_OPEN_DRAIN ; + lookup->flags |=GPIO_OPEN_SOURCE ; + }else { + count = -EINVAL ; + }
Quoted 3 lines
I prefer to see some kind of the array of constant string literals and do
sysfs_match_string()here
I would generally agree but if the flag values ever change to ones that make the resulting string array have holes in it, match_string() will suddenly stop working. I think that with bit flags defined elsewhere it's safer and more readable to do the above.
Quoted 12 lines
lookup->flags &= ~(GPIO_OPEN_DRAIN | GPIO_OPEN_SOURCE); flag = sysfs_match_string(...); if (flag < 0) count = flag else lookup->flags |= flag;(or something similar). And respectively indexed access above.
...
Quoted 7 lines
...
drivers/gpio/gpio-consumer.c line 698
Quoted 2 lines
drivers/gpio/gpio-consumer.c new file +2
-0+if (list_empty (&dev->lookup_list)) +return -ENODATA ;Instead you may count nodes here and if 0, return an error, otherwise pass it to the callee.
I'm not following, please rephrase.
Quoted 26 lines
Quoted 4 lines
drivers/gpio/gpio-consumer.c new file +3
-0+ swnode =gpio_consumer_make_device_swnode (dev); +if (IS_ERR (swnode)) +return PTR_ERR (swnode);...
Quoted 6 lines
drivers/gpio/gpio-consumer.c new file +6
-0+static ssize_t +gpio_consumer_device_config_live_store (struct config_item *item, +const char *page,size_t count) +{ +struct gpio_consumer_device *dev =to_gpio_consumer_device (item); +bool live;drivers/gpio/gpio-consumer.c line 767
Quoted 10 lines
drivers/gpio/gpio-consumer.c new file +10
-0+int ret; + + ret =kstrtobool (page, &live); +if (ret) +return ret; + +mutex_lock (&dev->lock); + +if ((!live && !gpio_consumer_device_is_live_unlocked (dev)) || + (live &&gpio_consumer_device_is_live_unlocked (dev)))if (live ^
gpio_consumer_device_is_live_unlocked(dev))
Quoted 1 line
?
Nah, let's not use bitwise operators for boolean logic.
[snip]
I commented on the ones that needed it, for others, I'll fix them for v2.
Bart
Quoted 2 lines · Bartosz Golaszewski
On Thu, Aug 3, 2023 at 1:39 PM Andy Shevchenko
<[email protected]> wrote:
...
Quoted 7 lines
Quoted 5 lines
drivers/gpio/gpio-consumer.c line 698
Quoted 2 lines
drivers/gpio/gpio-consumer.c new file +2
-0+if (list_empty (&dev->lookup_list)) +return -ENODATA ;Instead you may count nodes here and if 0, return an error, otherwise pass it to the callee.
I'm not following, please rephrase.
The below call calls list_count_nodes(), you may simply join these two checks by calling list_count_nodes() and return -ENODATA in case it's 0.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/gpio/gpio-consumer.c new file +3
-0+ swnode =gpio_consumer_make_device_swnode (dev); +if (IS_ERR (swnode)) +return PTR_ERR (swnode);
...
Quoted 8 lines
Quoted 6 lines
drivers/gpio/gpio-consumer.c line 767
Quoted 2 lines
drivers/gpio/gpio-consumer.c new file +2
-0+if ((!live && !gpio_consumer_device_is_live_unlocked (dev)) || + (live &&gpio_consumer_device_is_live_unlocked (dev)))if (live ^
gpio_consumer_device_is_live_unlocked(dev))?
Nah, let's not use bitwise operators for boolean logic.
Then it's even simpler:
if (live == gpio_consumer_device_is_live_unlocked(dev))
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 10 lines · Andy Shevchenko
Quoted 2 lines · Bartosz Golaszewski
On Thu, Aug 3, 2023 at 1:39 PM Andy Shevchenko
<[email protected]> wrote:...
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
drivers/gpio/gpio-consumer.c new file +2
-0+if (list_empty (&dev->lookup_list)) +return -ENODATA ;
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Instead you may count nodes here and if 0, return an error, otherwise pass it to the callee.
I'm not following, please rephrase.
The below call calls
list_count_nodes(), you may simply join these two checks by callinglist_count_nodes()and return-ENODATAin case it's 0.
We don't gain anything by doing that: list_empty() doesn't count nodes, it just checks the head pointer. We would decrease readability (more arguments!) for no reason.
Bart
Quoted 24 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/gpio/gpio-consumer.c new file +3
-0+ swnode =gpio_consumer_make_device_swnode (dev); +if (IS_ERR (swnode)) +return PTR_ERR (swnode);...
Quoted 8 lines
Quoted 6 lines
drivers/gpio/gpio-consumer.c line 767
Quoted 2 lines
drivers/gpio/gpio-consumer.c new file +2
-0+if ((!live && !gpio_consumer_device_is_live_unlocked (dev)) || + (live &&gpio_consumer_device_is_live_unlocked (dev)))if (live ^
gpio_consumer_device_is_live_unlocked(dev))?
Nah, let's not use bitwise operators for boolean logic.
Then it's even simpler:
if (live == gpio_consumer_device_is_live_unlocked(dev))--
With Best Regards,
Andy Shevchenko