| Message ID | 20231212054253.50094-1-warthog618@gmail.com |
|---|---|
| Headers | show |
| Series | gpiolib: cdev: relocate debounce_period_us | expand |
On Tue, Dec 12, 2023 at 6:43 AM Kent Gibson <warthog618@gmail.com> wrote: > > This series contains minor improvements to gpiolib-cdev. > > The banner change is relocating the debounce_period_us from gpiolib's > struct gpio_desc to cdev's struct line. The first patch stores the > field locally in cdev. The second removes the now unused field from > gpiolib. > > The third patch is somewhat related and removes a FIXME from > gpio_desc_to_lineinfo(). The FIXME relates to a race condition in > the calculation of the used flag, but I would assert that from > the userspace perspective the read operation itself is inherently racy. > The line being reported as unused in the info provides no guarantee - > it just an indicator that requesting the line is likely to succeed - > assuming the line is not otherwise requested in the meantime. > Give the overall operation is racy, trying to stamp out an unlikely > race within the operation is pointless. Accept it as a possibility > that has negligible side-effects and reduce the number of locks held > simultaneously and the duration that the gpio_lock is held. > > The fourth patch is unrelated to debounce or info, but addresses Andy's > recent assertion that the linereq get/set values functions are confusing > and under documented. Figured I may as well add that while I was in > there. > > Kent Gibson (4): > gpiolib: cdev: relocate debounce_period_us from struct gpio_desc > gpiolib: remove debounce_period_us from struct gpio_desc > gpiolib: cdev: reduce locking in gpio_desc_to_lineinfo() > gpiolib: cdev: improve documentation of get/set values > > drivers/gpio/gpiolib-cdev.c | 257 ++++++++++++++++++++++++++++-------- > drivers/gpio/gpiolib.c | 3 - > drivers/gpio/gpiolib.h | 5 - > 3 files changed, 201 insertions(+), 64 deletions(-) > > -- > 2.39.2 > Patches 2-4 look fine, I was about to review patch 1 in detail but I thought I'd just throw this one in here before we commit to a specific solution. For some reason I thought this would not work but I'm now considering it as an alternative approach: is there anything wrong with adding struct kref to struct line, allocating it separately per-line when gpio_chardev_data is created, referencing it from struct linereq when the line is being requested, and dropping the reference from gpio_chardev_data and linereq when either is being removed? Other than the increased number of allocations? Bartosz
On Tue, Dec 12, 2023 at 06:09:00PM +0100, Bartosz Golaszewski wrote: > On Tue, Dec 12, 2023 at 6:43 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > This series contains minor improvements to gpiolib-cdev. > > > > The banner change is relocating the debounce_period_us from gpiolib's > > struct gpio_desc to cdev's struct line. The first patch stores the > > field locally in cdev. The second removes the now unused field from > > gpiolib. > > > > The third patch is somewhat related and removes a FIXME from > > gpio_desc_to_lineinfo(). The FIXME relates to a race condition in > > the calculation of the used flag, but I would assert that from > > the userspace perspective the read operation itself is inherently racy. > > The line being reported as unused in the info provides no guarantee - > > it just an indicator that requesting the line is likely to succeed - > > assuming the line is not otherwise requested in the meantime. > > Give the overall operation is racy, trying to stamp out an unlikely > > race within the operation is pointless. Accept it as a possibility > > that has negligible side-effects and reduce the number of locks held > > simultaneously and the duration that the gpio_lock is held. > > > > The fourth patch is unrelated to debounce or info, but addresses Andy's > > recent assertion that the linereq get/set values functions are confusing > > and under documented. Figured I may as well add that while I was in > > there. > > > > Kent Gibson (4): > > gpiolib: cdev: relocate debounce_period_us from struct gpio_desc > > gpiolib: remove debounce_period_us from struct gpio_desc > > gpiolib: cdev: reduce locking in gpio_desc_to_lineinfo() > > gpiolib: cdev: improve documentation of get/set values > > > > drivers/gpio/gpiolib-cdev.c | 257 ++++++++++++++++++++++++++++-------- > > drivers/gpio/gpiolib.c | 3 - > > drivers/gpio/gpiolib.h | 5 - > > 3 files changed, 201 insertions(+), 64 deletions(-) > > > > -- > > 2.39.2 > > > > Patches 2-4 look fine, I was about to review patch 1 in detail but I > thought I'd just throw this one in here before we commit to a specific > solution. > > For some reason I thought this would not work but I'm now considering > it as an alternative approach: is there anything wrong with adding > struct kref to struct line, allocating it separately per-line when > gpio_chardev_data is created, referencing it from struct linereq when > the line is being requested, and dropping the reference from > gpio_chardev_data and linereq when either is being removed? Other than > the increased number of allocations? > The collection of struct line always has to be global, right, as both gpio_chardev_data and linereq are ephemeral. e.g. if one process requests a line and another checks the lineinfo, those will have distinct gpio_chardev_data. But the key issue is that the linereq and struct line lifetimes are strictly tied - a struct line does not live beyond the containing linereq. Leaving the struct line alive after the linereq is released is just wrong. The line has been released back to gpiolib so there can be no supplemental info left. If you want any such info to persist beyond the line release then it should be located in gpiolib itself, not cdev. Cheers, Kent.
On Wed, Dec 13, 2023 at 12:58 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Tue, Dec 12, 2023 at 06:09:00PM +0100, Bartosz Golaszewski wrote: [snip] > > > > Patches 2-4 look fine, I was about to review patch 1 in detail but I > > thought I'd just throw this one in here before we commit to a specific > > solution. > > > > For some reason I thought this would not work but I'm now considering > > it as an alternative approach: is there anything wrong with adding > > struct kref to struct line, allocating it separately per-line when > > gpio_chardev_data is created, referencing it from struct linereq when > > the line is being requested, and dropping the reference from > > gpio_chardev_data and linereq when either is being removed? Other than > > the increased number of allocations? > > > > The collection of struct line always has to be global, right, as both > gpio_chardev_data and linereq are ephemeral. e.g. if one process requests > a line and another checks the lineinfo, those will have distinct > gpio_chardev_data. > Strictly speaking at least the supplemental info has to be globally accessible. But I get your point. > But the key issue is that the linereq and struct line lifetimes are > strictly tied - a struct line does not live beyond the containing linereq. I was thinking about decoupling one from the other actually. > Leaving the struct line alive after the linereq is released is just wrong. > The line has been released back to gpiolib so there can be no > supplemental info left. Indeed. > If you want any such info to persist beyond the line release then it > should be located in gpiolib itself, not cdev. > Right, we even zero debounce_period_us anyway on line release - just as if we released struct line. Bart
On Wed, Dec 13, 2023 at 11:03:40AM +0100, Bartosz Golaszewski wrote: > On Wed, Dec 13, 2023 at 12:58 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Tue, Dec 12, 2023 at 06:09:00PM +0100, Bartosz Golaszewski wrote: > > [snip] > > > > > > > Patches 2-4 look fine, I was about to review patch 1 in detail but I > > > thought I'd just throw this one in here before we commit to a specific > > > solution. > > > > > > For some reason I thought this would not work but I'm now considering > > > it as an alternative approach: is there anything wrong with adding > > > struct kref to struct line, allocating it separately per-line when > > > gpio_chardev_data is created, referencing it from struct linereq when > > > the line is being requested, and dropping the reference from > > > gpio_chardev_data and linereq when either is being removed? Other than > > > the increased number of allocations? > > > > > > > The collection of struct line always has to be global, right, as both > > gpio_chardev_data and linereq are ephemeral. e.g. if one process requests > > a line and another checks the lineinfo, those will have distinct > > gpio_chardev_data. > > > > Strictly speaking at least the supplemental info has to be globally > accessible. But I get your point. > > > But the key issue is that the linereq and struct line lifetimes are > > strictly tied - a struct line does not live beyond the containing linereq. > > I was thinking about decoupling one from the other actually. > I was also headed down that path - making the supplemental info for each line distinct from the struct line. But then I realised that the lifetime is strictly tied to the linereq, as per the struct line, and there was no benefit in a separate object - just more overhead. Cheers, Kent.