| Message ID | 20241025-gpio-notify-sysfs-v2-0-5bd1b1b0b3e6@linaro.org |
|---|---|
| Headers | show |
| Series | gpio: sysfs: send character device notifications for sysfs class events | expand |
On Fri, Oct 25, 2024 at 3:24 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Fri, Oct 25, 2024 at 02:18:51PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Shrink the code and drop some goto labels by using lock guards around > > gpiod_data::mutex. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- > > drivers/gpio/gpiolib-sysfs.c | 81 ++++++++++++++++---------------------------- > > 1 file changed, 29 insertions(+), 52 deletions(-) > > > > @@ -139,19 +132,17 @@ static ssize_t value_store(struct device *dev, > > long value; > > > > status = kstrtol(buf, 0, &value); > > + if (status) > > + return status; > > > > - mutex_lock(&data->mutex); > > + guard(mutex)(&data->mutex); > > > > - if (!test_bit(FLAG_IS_OUT, &desc->flags)) { > > - status = -EPERM; > > - } else if (status == 0) { > > - gpiod_set_value_cansleep(desc, value); > > - status = size; > > - } > > + if (!test_bit(FLAG_IS_OUT, &desc->flags)) > > + return -EPERM; > > > > - mutex_unlock(&data->mutex); > > + gpiod_set_value_cansleep(desc, value); > > > > - return status; > > + return size; > > } > > This is a behavioural change as you've moved the decode check before the > permission check. Not sure if that is significant or not, so in my > suggestion I retained the old order. > > Cheers, > Kent. Yeah, I don't know why it was done. Typically you want to sanitize the input before anything else and this is what's done almost everywhere else. I'd keep it like that. Bart
On Fri, Oct 25, 2024 at 5:34 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > Yeah, I don't know why it was done. Typically you want to sanitize the > > input before anything else and this is what's done almost everywhere > > else. I'd keep it like that. > > Not knowing why it was done was precisely the reason I thought it > should be left as is. The fact that the checks are performed in the > other order elsewhere makes me think this one was done intentionally. > Conceivably it could be used by userspace to test if a line is output when > the direction is fixed (so /sys/class/gpio/gpioN/direction does not exist). > So write a non-integer to the value and see if it returns -EPERM rather > than -EINVAL. > > Admittedly I'm speculating, but I can't rule it out, so I wouldn't > change the behaviour just because it is more aesthetically pleasing. > And if you insist on tidying the behaviour then it should be in a separate > patch rather than piggy-backing onto the guard change. > > Anyway, that is my 2c. > Ok, I'll restore the order in v3. Bartosz
This may be a total corner-case but for consistency and completeness I think it makes sense to also send out line state change events on actions triggered from the GPIO sysfs class. The first two patches use cleanup helpers in sysfs code. The next three change the code to emit notifications on line export (unexport is already handled) and active_low & edge changes. One last thing I considered was also notifying user-space whenever gpiochip_un/lock_as_irq() is called but that doesn't make much sense as it's largely independent from the GPIO core and can be called for both requested and available lines whenever someone requests an interrupt from a GPIO controller. Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> --- Changes in v2: - Streamline the code even more by dropping unnecessary return code assignments - use normal guards where scoped ones are overkill - Link to v1: https://lore.kernel.org/r/20241024-gpio-notify-sysfs-v1-0-981f2773e785@linaro.org --- Bartosz Golaszewski (5): gpio: sysfs: use cleanup guards for gpiod_data::mutex gpio: sysfs: use cleanup guards for the sysfs_lock mutex gpio: sysfs: emit chardev line-state events on GPIO export gpio: sysfs: emit chardev line-state events on active-low changes gpio: sysfs: emit chardev line-state events on edge store drivers/gpio/gpiolib-sysfs.c | 169 +++++++++++++++++++------------------------ 1 file changed, 75 insertions(+), 94 deletions(-) --- base-commit: a39230ecf6b3057f5897bc4744a790070cfbe7a8 change-id: 20241022-gpio-notify-sysfs-3bddf9ecca9f Best regards,