| Message ID | 20231212054253.50094-2-warthog618@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: cdev: relocate debounce_period_us | expand |
On Tue, Dec 12, 2023 at 01:42:50PM +0800, Kent Gibson wrote: > Store the debounce period for a requested line locally, rather than in > the debounce_period_us field in the gpiolib struct gpio_desc. > > Add a global tree of lines containing supplemental line information > to make the debounce period available to be reported by the > GPIO_V2_GET_LINEINFO_IOCTL and the line change notifier. ... > struct line { > struct gpio_desc *desc; > + struct rb_node node; If you swap them, would it benefit in a code generation (bloat-o-meter)? > }; ... > +struct supinfo { > + spinlock_t lock; > + struct rb_root tree; > +}; Same Q. ... > +static struct supinfo supinfo; Why supinfo should be a struct to begin with? Seems to me as an unneeded complication. ... > + pr_warn("%s: duplicate line inserted\n", __func__); I hope at bare minimum we have pr_fmt(), but even though this is poor message that might require some information about exact duplication (GPIO chip label / name, line number, etc). Generally speaking the __func__ in non-debug messages _usually_ is a symptom of poorly written message. ... > +out_unlock: > + spin_unlock(&supinfo.lock); No use of cleanup.h? ... > +static inline bool line_is_supplemental(struct line *line) > +{ > + return READ_ONCE(line->debounce_period_us) != 0; " != 0" is redundant. > +} ... > for (i = 0; i < lr->num_lines; i++) { > - if (lr->lines[i].desc) { > - edge_detector_stop(&lr->lines[i]); > - gpiod_free(lr->lines[i].desc); > + line = &lr->lines[i]; > + if (line->desc) { Perhaps if (!line->desc) continue; ? > + edge_detector_stop(line); > + if (line_is_supplemental(line)) > + supinfo_erase(line); > + gpiod_free(line->desc); > } > } ... > +static int __init gpiolib_cdev_init(void) > +{ > + supinfo_init(); > + return 0; > +} It's a good practice to explain initcalls (different to the default ones), can you add a comment on top to explain the choice of this initcall, please? > +postcore_initcall(gpiolib_cdev_init);
On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote: > On Tue, Dec 12, 2023 at 01:42:50PM +0800, Kent Gibson wrote: > > Store the debounce period for a requested line locally, rather than in > > the debounce_period_us field in the gpiolib struct gpio_desc. > > > > Add a global tree of lines containing supplemental line information > > to make the debounce period available to be reported by the > > GPIO_V2_GET_LINEINFO_IOCTL and the line change notifier. > > ... > > > struct line { > > struct gpio_desc *desc; > > + struct rb_node node; > > If you swap them, would it benefit in a code generation (bloat-o-meter)? > Didn't consider that placement within the scruct could impact code generation. Having the rb_nodes at the beginning of struct is preferable? > > }; > > ... > > > +struct supinfo { > > + spinlock_t lock; > > + struct rb_root tree; > > +}; > > Same Q. > Same - I tend to put locks before the field(s) they cover. But if the node being first results in nicer code then happy to swap. > ... > > > +static struct supinfo supinfo; > > Why supinfo should be a struct to begin with? Seems to me as an unneeded > complication. > Yeah, that is a hangover from an earlier iteration where supinfo was contained in other object rather than being a global. Could merge the struct definition into the variable now. > ... > > > + pr_warn("%s: duplicate line inserted\n", __func__); > > I hope at bare minimum we have pr_fmt(), but even though this is poor message > that might require some information about exact duplication (GPIO chip label / > name, line number, etc). Generally speaking the __func__ in non-debug messages > _usually_ is a symptom of poorly written message. > > ... Yeah, I wasn't sure about the best way to log here. The details of chip or line etc don't add anything - seeing this error means there is a logic error in the code - we have inserted a line without erasing it. Knowing which chip or line it happened to occur on wont help debug it. It should never happen, but you can't just leave it unhandled, so I went with a basic log. > > > +out_unlock: > > + spin_unlock(&supinfo.lock); > > No use of cleanup.h? > Again, that is new to me, so no not yet. > ... > > > +static inline bool line_is_supplemental(struct line *line) > > +{ > > + return READ_ONCE(line->debounce_period_us) != 0; > > " != 0" is redundant. > I prefer conversion from int to bool to be explicit, but if you insist... > > +} > > ... > > > for (i = 0; i < lr->num_lines; i++) { > > - if (lr->lines[i].desc) { > > - edge_detector_stop(&lr->lines[i]); > > - gpiod_free(lr->lines[i].desc); > > + line = &lr->lines[i]; > > + if (line->desc) { > > Perhaps > > if (!line->desc) > continue; > > ? Seems reasonable - I was just going with what was already there. > > > + edge_detector_stop(line); > > + if (line_is_supplemental(line)) > > + supinfo_erase(line); > > + gpiod_free(line->desc); > > } > > } > > ... > > > +static int __init gpiolib_cdev_init(void) > > +{ > > + supinfo_init(); > > + return 0; > > +} > > It's a good practice to explain initcalls (different to the default ones), > can you add a comment on top to explain the choice of this initcall, please? > Not sure what you mean. This section used gpiolib-sysfs as a template, and that has no documentation. > > +postcore_initcall(gpiolib_cdev_init); > Thanks for the review - always instructive. Cheers, Kent.
On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote: > > On Tue, Dec 12, 2023 at 01:42:50PM +0800, Kent Gibson wrote: > > > Store the debounce period for a requested line locally, rather than in > > > the debounce_period_us field in the gpiolib struct gpio_desc. > > > > > > Add a global tree of lines containing supplemental line information > > > to make the debounce period available to be reported by the > > > GPIO_V2_GET_LINEINFO_IOCTL and the line change notifier. > > > > ... > > > > > struct line { > > > struct gpio_desc *desc; > > > + struct rb_node node; > > > > If you swap them, would it benefit in a code generation (bloat-o-meter)? > > > > Didn't consider that placement within the scruct could impact code > generation. > Having the rb_nodes at the beginning of struct is preferable? > I suppose it has something to do with 0 offset when using container_of(). Not sure if that really matters though. > > > }; > > > > ... > > > > > +struct supinfo { > > > + spinlock_t lock; > > > + struct rb_root tree; > > > +}; > > > > Same Q. > > > > Same - I tend to put locks before the field(s) they cover. > But if the node being first results in nicer code then happy to swap. > > > ... > > > > > +static struct supinfo supinfo; > > > > Why supinfo should be a struct to begin with? Seems to me as an unneeded > > complication. > > I think we should keep it as a struct but defined the following way: struct { spinlock_t lock; struct rb_root tree; } supinfo; > > Yeah, that is a hangover from an earlier iteration where supinfo was > contained in other object rather than being a global. > Could merge the struct definition into the variable now. > > > ... > > > > > + pr_warn("%s: duplicate line inserted\n", __func__); > > > > I hope at bare minimum we have pr_fmt(), but even though this is poor message > > that might require some information about exact duplication (GPIO chip label / > > name, line number, etc). Generally speaking the __func__ in non-debug messages > > _usually_ is a symptom of poorly written message. > > > > ... > > Yeah, I wasn't sure about the best way to log here. > > The details of chip or line etc don't add anything - seeing this error > means there is a logic error in the code - we have inserted a line > without erasing it. Knowing which chip or line it happened to occur on > wont help debug it. It should never happen, but you can't just leave it > unhandled, so I went with a basic log. > We should yell loudly in that case - use one of the WARN() variants that'll print a stack trace too and point you to the relevant line in the code. > > > > > +out_unlock: > > > + spin_unlock(&supinfo.lock); > > > > No use of cleanup.h? > > > > Again, that is new to me, so no not yet. > Yep, please use a guard, they're awesome. :) > > ... > > > > > +static inline bool line_is_supplemental(struct line *line) > > > +{ > > > + return READ_ONCE(line->debounce_period_us) != 0; > > > > " != 0" is redundant. > > > > I prefer conversion from int to bool to be explicit, but if you > insist... > > > > +} > > > > ... > > > > > for (i = 0; i < lr->num_lines; i++) { > > > - if (lr->lines[i].desc) { > > > - edge_detector_stop(&lr->lines[i]); > > > - gpiod_free(lr->lines[i].desc); > > > + line = &lr->lines[i]; > > > + if (line->desc) { > > > > Perhaps > > > > if (!line->desc) > > continue; > > > > ? > > Seems reasonable - I was just going with what was already there. > > > > > > + edge_detector_stop(line); > > > + if (line_is_supplemental(line)) > > > + supinfo_erase(line); > > > + gpiod_free(line->desc); > > > } > > > } > > > > ... > > > > > +static int __init gpiolib_cdev_init(void) > > > +{ > > > + supinfo_init(); > > > + return 0; > > > +} > > > > It's a good practice to explain initcalls (different to the default ones), > > can you add a comment on top to explain the choice of this initcall, please? > > > > Not sure what you mean. This section used gpiolib-sysfs as a template, > and that has no documentation. > > > > +postcore_initcall(gpiolib_cdev_init); > > > > Thanks for the review - always instructive. > > Cheers, > Kent. Bart
On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote: > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote: > > > On Tue, Dec 12, 2023 at 01:42:50PM +0800, Kent Gibson wrote: > > > > Store the debounce period for a requested line locally, rather than in > > > > the debounce_period_us field in the gpiolib struct gpio_desc. > > > > > > > > Add a global tree of lines containing supplemental line information > > > > to make the debounce period available to be reported by the > > > > GPIO_V2_GET_LINEINFO_IOCTL and the line change notifier. > > > > > > ... > > > > > > > struct line { > > > > struct gpio_desc *desc; > > > > + struct rb_node node; > > > > > > If you swap them, would it benefit in a code generation (bloat-o-meter)? > > > > > > > Didn't consider that placement within the scruct could impact code > > generation. > > Having the rb_nodes at the beginning of struct is preferable? > > > > I suppose it has something to do with 0 offset when using > container_of(). Not sure if that really matters though. > There are other fields that get the container_of() treatment, but node does look to be the one most used, so probably makes sense to put it first. > > > > }; > > > > > > ... > > > > > > > +struct supinfo { > > > > + spinlock_t lock; > > > > + struct rb_root tree; > > > > +}; > > > > > > Same Q. > > > > > > > Same - I tend to put locks before the field(s) they cover. > > But if the node being first results in nicer code then happy to swap. > > > > > ... > > > > > > > +static struct supinfo supinfo; > > > > > > Why supinfo should be a struct to begin with? Seems to me as an unneeded > > > complication. > > > > > I think we should keep it as a struct but defined the following way: > > struct { > spinlock_t lock; > struct rb_root tree; > } supinfo; That is what I meant be merging the struct definition with the variable definition. Or is there some other way to completely do away with the struct that I'm missing? > > > > Yeah, that is a hangover from an earlier iteration where supinfo was > > contained in other object rather than being a global. > > Could merge the struct definition into the variable now. > > > > > ... > > > > > > > + pr_warn("%s: duplicate line inserted\n", __func__); > > > > > > I hope at bare minimum we have pr_fmt(), but even though this is poor message > > > that might require some information about exact duplication (GPIO chip label / > > > name, line number, etc). Generally speaking the __func__ in non-debug messages > > > _usually_ is a symptom of poorly written message. > > > > > > ... > > > > Yeah, I wasn't sure about the best way to log here. > > > > The details of chip or line etc don't add anything - seeing this error > > means there is a logic error in the code - we have inserted a line > > without erasing it. Knowing which chip or line it happened to occur on > > wont help debug it. It should never happen, but you can't just leave it > > unhandled, so I went with a basic log. > > > > We should yell loudly in that case - use one of the WARN() variants > that'll print a stack trace too and point you to the relevant line in > the code. > Ok, so any suggestion as to which WARN() variant would make the most sense? > > > > > > > +out_unlock: > > > > + spin_unlock(&supinfo.lock); > > > > > > No use of cleanup.h? > > > > > > > Again, that is new to me, so no not yet. > > > > Yep, please use a guard, they're awesome. :) > Will do. Thanks, Kent.
On Wed, Dec 13, 2023 at 11:59:26PM +0800, Kent Gibson wrote: > On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote: > > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <warthog618@gmail.com> wrote: > > > On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote: > > > > On Tue, Dec 12, 2023 at 01:42:50PM +0800, Kent Gibson wrote: ... > > > > > +static struct supinfo supinfo; > > > > > > > > Why supinfo should be a struct to begin with? Seems to me as an unneeded > > > > complication. > > > > I think we should keep it as a struct but defined the following way: > > > > struct { > > spinlock_t lock; > > struct rb_root tree; > > } supinfo; > > That is what I meant be merging the struct definition with the variable > definition. Or is there some other way to completely do away with the > struct that I'm missing? Look at the top of gpiolib.c: static DEFINE_MUTEX(gpio_lookup_lock); static LIST_HEAD(gpio_lookup_list); In the similar way you can simply do static DEFINE_SPINLOCK(gpio_sup_lock); static struct rb_root gpio_sup_tree; > > > Yeah, that is a hangover from an earlier iteration where supinfo was > > > contained in other object rather than being a global. > > > Could merge the struct definition into the variable now.
On Wed, Dec 13, 2023 at 4:59 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote: > > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote: > > > > On Tue, Dec 12, 2023 at 01:42:50PM +0800, Kent Gibson wrote: > > > > > Store the debounce period for a requested line locally, rather than in > > > > > the debounce_period_us field in the gpiolib struct gpio_desc. > > > > > > > > > > Add a global tree of lines containing supplemental line information > > > > > to make the debounce period available to be reported by the > > > > > GPIO_V2_GET_LINEINFO_IOCTL and the line change notifier. > > > > > > > > ... > > > > > > > > > struct line { > > > > > struct gpio_desc *desc; > > > > > + struct rb_node node; > > > > > > > > If you swap them, would it benefit in a code generation (bloat-o-meter)? > > > > > > > > > > Didn't consider that placement within the scruct could impact code > > > generation. > > > Having the rb_nodes at the beginning of struct is preferable? > > > > > > > I suppose it has something to do with 0 offset when using > > container_of(). Not sure if that really matters though. > > > > There are other fields that get the container_of() treatment, but node > does look to be the one most used, so probably makes sense to put it > first. > > > > > > }; > > > > > > > > ... > > > > > > > > > +struct supinfo { > > > > > + spinlock_t lock; > > > > > + struct rb_root tree; > > > > > +}; > > > > > > > > Same Q. > > > > > > > > > > Same - I tend to put locks before the field(s) they cover. > > > But if the node being first results in nicer code then happy to swap. > > > > > > > ... > > > > > > > > > +static struct supinfo supinfo; > > > > > > > > Why supinfo should be a struct to begin with? Seems to me as an unneeded > > > > complication. > > > > > > > > I think we should keep it as a struct but defined the following way: > > > > struct { > > spinlock_t lock; > > struct rb_root tree; > > } supinfo; > > That is what I meant be merging the struct definition with the variable > definition. Or is there some other way to completely do away with the > struct that I'm missing? No, I also meant that. > > > > > > > Yeah, that is a hangover from an earlier iteration where supinfo was > > > contained in other object rather than being a global. > > > Could merge the struct definition into the variable now. > > > > > > > ... > > > > > > > > > + pr_warn("%s: duplicate line inserted\n", __func__); > > > > > > > > I hope at bare minimum we have pr_fmt(), but even though this is poor message > > > > that might require some information about exact duplication (GPIO chip label / > > > > name, line number, etc). Generally speaking the __func__ in non-debug messages > > > > _usually_ is a symptom of poorly written message. > > > > > > > > ... > > > > > > Yeah, I wasn't sure about the best way to log here. > > > > > > The details of chip or line etc don't add anything - seeing this error > > > means there is a logic error in the code - we have inserted a line > > > without erasing it. Knowing which chip or line it happened to occur on > > > wont help debug it. It should never happen, but you can't just leave it > > > unhandled, so I went with a basic log. > > > > > > > We should yell loudly in that case - use one of the WARN() variants > > that'll print a stack trace too and point you to the relevant line in > > the code. > > > > Ok, so any suggestion as to which WARN() variant would make the most sense? > Just a regular WARN(1, "msg ..."); > > > > > > > > > +out_unlock: > > > > > + spin_unlock(&supinfo.lock); > > > > > > > > No use of cleanup.h? > > > > > > > > > > Again, that is new to me, so no not yet. > > > > > > > Yep, please use a guard, they're awesome. :) > > > > Will do. > > Thanks, > Kent. Bart
On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote: > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > +out_unlock: > > > > + spin_unlock(&supinfo.lock); > > > > > > No use of cleanup.h? > > > > > > > Again, that is new to me, so no not yet. > > > > Yep, please use a guard, they're awesome. :) > Before I go nuts and switch everything over, is it ok to put a return within the scoped_guard - it will automatically unlock on the way out? Cheers, Kent.
On Wed, Dec 13, 2023 at 5:12 PM Andy Shevchenko <andy@kernel.org> wrote: > > On Wed, Dec 13, 2023 at 11:59:26PM +0800, Kent Gibson wrote: > > On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote: > > > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote: > > > > > On Tue, Dec 12, 2023 at 01:42:50PM +0800, Kent Gibson wrote: > > ... > > > > > > > +static struct supinfo supinfo; > > > > > > > > > > Why supinfo should be a struct to begin with? Seems to me as an unneeded > > > > > complication. > > > > > > I think we should keep it as a struct but defined the following way: > > > > > > struct { > > > spinlock_t lock; > > > struct rb_root tree; > > > } supinfo; > > > > That is what I meant be merging the struct definition with the variable > > definition. Or is there some other way to completely do away with the > > struct that I'm missing? > > Look at the top of gpiolib.c: > > static DEFINE_MUTEX(gpio_lookup_lock); > static LIST_HEAD(gpio_lookup_list); > > In the similar way you can simply do > > static DEFINE_SPINLOCK(gpio_sup_lock); > static struct rb_root gpio_sup_tree; > The fact that this has been like this, doesn't mean it's the only right way. IMO putting these into the same structure makes logical sense. Bart > > > > Yeah, that is a hangover from an earlier iteration where supinfo was > > > > contained in other object rather than being a global. > > > > Could merge the struct definition into the variable now. > > -- > With Best Regards, > Andy Shevchenko > > >
On Wed, Dec 13, 2023 at 5:15 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote: > > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > > > > > +out_unlock: > > > > > + spin_unlock(&supinfo.lock); > > > > > > > > No use of cleanup.h? > > > > > > > > > > Again, that is new to me, so no not yet. > > > > > > > Yep, please use a guard, they're awesome. :) > > > > Before I go nuts and switch everything over, is it ok to put a return > within the scoped_guard - it will automatically unlock on the way out? > Yes! This is precisely why they're so great. Bart > Cheers, > Kent.
On Thu, Dec 14, 2023 at 12:15:10AM +0800, Kent Gibson wrote: > On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote: > > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > +out_unlock: > > > > > + spin_unlock(&supinfo.lock); > > > > > > > > No use of cleanup.h? > > > > > > Again, that is new to me, so no not yet. > > > > Yep, please use a guard, they're awesome. :) > > Before I go nuts and switch everything over, is it ok to put a return > within the scoped_guard - it will automatically unlock on the way out? Yes, should do that.
On Wed, Dec 13, 2023 at 05:15:38PM +0100, Bartosz Golaszewski wrote: > On Wed, Dec 13, 2023 at 5:12 PM Andy Shevchenko <andy@kernel.org> wrote: > > On Wed, Dec 13, 2023 at 11:59:26PM +0800, Kent Gibson wrote: > > > On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote: > > > > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote: > > > > > > On Tue, Dec 12, 2023 at 01:42:50PM +0800, Kent Gibson wrote: ... > > > > > > > +static struct supinfo supinfo; > > > > > > > > > > > > Why supinfo should be a struct to begin with? Seems to me as an unneeded > > > > > > complication. > > > > > > > > I think we should keep it as a struct but defined the following way: > > > > > > > > struct { > > > > spinlock_t lock; > > > > struct rb_root tree; > > > > } supinfo; > > > > > > That is what I meant be merging the struct definition with the variable > > > definition. Or is there some other way to completely do away with the > > > struct that I'm missing? > > > > Look at the top of gpiolib.c: > > > > static DEFINE_MUTEX(gpio_lookup_lock); > > static LIST_HEAD(gpio_lookup_list); > > > > In the similar way you can simply do > > > > static DEFINE_SPINLOCK(gpio_sup_lock); > > static struct rb_root gpio_sup_tree; > > The fact that this has been like this, doesn't mean it's the only > right way. IMO putting these into the same structure makes logical > sense. I disagree on the struct like this on the grounds: - it's global - it's one time use - it adds complications for no benefit - it makes code uglier and longer What we probably want to have is a singleton objects in C language or at least inside Linux kernel project. But I'm not sure it's feasible. > > > > > Yeah, that is a hangover from an earlier iteration where supinfo was > > > > > contained in other object rather than being a global. > > > > > Could merge the struct definition into the variable now.
On Wed, Dec 13, 2023 at 5:29 PM Andy Shevchenko <andy@kernel.org> wrote: > > On Wed, Dec 13, 2023 at 05:15:38PM +0100, Bartosz Golaszewski wrote: > > On Wed, Dec 13, 2023 at 5:12 PM Andy Shevchenko <andy@kernel.org> wrote: > > > On Wed, Dec 13, 2023 at 11:59:26PM +0800, Kent Gibson wrote: > > > > On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote: > > > > > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote: > > > > > > > On Tue, Dec 12, 2023 at 01:42:50PM +0800, Kent Gibson wrote: > > ... > > > > > > > > > +static struct supinfo supinfo; > > > > > > > > > > > > > > Why supinfo should be a struct to begin with? Seems to me as an unneeded > > > > > > > complication. > > > > > > > > > > I think we should keep it as a struct but defined the following way: > > > > > > > > > > struct { > > > > > spinlock_t lock; > > > > > struct rb_root tree; > > > > > } supinfo; > > > > > > > > That is what I meant be merging the struct definition with the variable > > > > definition. Or is there some other way to completely do away with the > > > > struct that I'm missing? > > > > > > Look at the top of gpiolib.c: > > > > > > static DEFINE_MUTEX(gpio_lookup_lock); > > > static LIST_HEAD(gpio_lookup_list); > > > > > > In the similar way you can simply do > > > > > > static DEFINE_SPINLOCK(gpio_sup_lock); > > > static struct rb_root gpio_sup_tree; > > > > The fact that this has been like this, doesn't mean it's the only > > right way. IMO putting these into the same structure makes logical > > sense. > > I disagree on the struct like this on the grounds: > - it's global > - it's one time use > - it adds complications for no benefit > - it makes code uglier and longer > It boils down to supinfo.lock vs supinfo_lock. I do prefer the former but it's a weak opinion, I won't die on that hill. Bart > What we probably want to have is a singleton objects in C language or at least > inside Linux kernel project. But I'm not sure it's feasible. > > > > > > > Yeah, that is a hangover from an earlier iteration where supinfo was > > > > > > contained in other object rather than being a global. > > > > > > Could merge the struct definition into the variable now. > > -- > With Best Regards, > Andy Shevchenko > >
On Wed, Dec 13, 2023 at 08:03:44PM +0100, Bartosz Golaszewski wrote: > On Wed, Dec 13, 2023 at 5:29 PM Andy Shevchenko <andy@kernel.org> wrote: > > On Wed, Dec 13, 2023 at 05:15:38PM +0100, Bartosz Golaszewski wrote: > > > On Wed, Dec 13, 2023 at 5:12 PM Andy Shevchenko <andy@kernel.org> wrote: > > > > On Wed, Dec 13, 2023 at 11:59:26PM +0800, Kent Gibson wrote: > > > > > On Wed, Dec 13, 2023 at 04:40:12PM +0100, Bartosz Golaszewski wrote: > > > > > > On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote: > > > > > > > > On Tue, Dec 12, 2023 at 01:42:50PM +0800, Kent Gibson wrote: ... > > > > > > > > > +static struct supinfo supinfo; > > > > > > > > > > > > > > > > Why supinfo should be a struct to begin with? Seems to me as an unneeded > > > > > > > > complication. > > > > > > > > > > > > I think we should keep it as a struct but defined the following way: > > > > > > > > > > > > struct { > > > > > > spinlock_t lock; > > > > > > struct rb_root tree; > > > > > > } supinfo; > > > > > > > > > > That is what I meant be merging the struct definition with the variable > > > > > definition. Or is there some other way to completely do away with the > > > > > struct that I'm missing? > > > > > > > > Look at the top of gpiolib.c: > > > > > > > > static DEFINE_MUTEX(gpio_lookup_lock); > > > > static LIST_HEAD(gpio_lookup_list); > > > > > > > > In the similar way you can simply do > > > > > > > > static DEFINE_SPINLOCK(gpio_sup_lock); > > > > static struct rb_root gpio_sup_tree; > > > > > > The fact that this has been like this, doesn't mean it's the only > > > right way. IMO putting these into the same structure makes logical > > > sense. > > > > I disagree on the struct like this on the grounds: > > - it's global > > - it's one time use > > - it adds complications for no benefit > > - it makes code uglier and longer > > > > It boils down to supinfo.lock vs supinfo_lock. I do prefer the former > but it's a weak opinion, I won't die on that hill. Me neither, just showing rationale from my side. > > What we probably want to have is a singleton objects in C language or at least > > inside Linux kernel project. But I'm not sure it's feasible. > > > > > > > > > Yeah, that is a hangover from an earlier iteration where supinfo was > > > > > > > contained in other object rather than being a global. > > > > > > > Could merge the struct definition into the variable now.
On Wed, Dec 13, 2023 at 10:07:12PM +0200, Andy Shevchenko wrote: > On Wed, Dec 13, 2023 at 08:03:44PM +0100, Bartosz Golaszewski wrote: > > ... > > > > > > > > > > > +static struct supinfo supinfo; > > > > > > > > > > > > > > > > > > Why supinfo should be a struct to begin with? Seems to me as an unneeded > > > > > > > > > complication. > > > > > > > > > > > > > > I think we should keep it as a struct but defined the following way: > > > > > > > > > > > > > > struct { > > > > > > > spinlock_t lock; > > > > > > > struct rb_root tree; > > > > > > > } supinfo; > > > > > > > > > > > > That is what I meant be merging the struct definition with the variable > > > > > > definition. Or is there some other way to completely do away with the > > > > > > struct that I'm missing? > > > > > > > > > > Look at the top of gpiolib.c: > > > > > > > > > > static DEFINE_MUTEX(gpio_lookup_lock); > > > > > static LIST_HEAD(gpio_lookup_list); > > > > > > > > > > In the similar way you can simply do > > > > > > > > > > static DEFINE_SPINLOCK(gpio_sup_lock); > > > > > static struct rb_root gpio_sup_tree; > > > > > > > > The fact that this has been like this, doesn't mean it's the only > > > > right way. IMO putting these into the same structure makes logical > > > > sense. > > > > > > I disagree on the struct like this on the grounds: > > > - it's global I dislike having the global at all - and now you want two :-(. > > > - it's one time use Its not about how many times it is instanciated, it is about maintainability. > > > - it adds complications for no benefit It provides a placeholder for collective documentation and clarifies scope for the reader. How is it more complicated? > > > - it makes code uglier and longer > > > What, swapping an underscore for a period? And you would hope the generated code is essentially the same. > > > > It boils down to supinfo.lock vs supinfo_lock. I do prefer the former > > but it's a weak opinion, I won't die on that hill. > > Me neither, just showing rationale from my side. > I can't recall the last time I intentionally used separate globals over a struct, so if there are no strong opinions otherwise I'll leave it as a struct. Cheers, Kent.
On Thu, Dec 14, 2023 at 08:18:01AM +0800, Kent Gibson wrote: > On Wed, Dec 13, 2023 at 10:07:12PM +0200, Andy Shevchenko wrote: > > On Wed, Dec 13, 2023 at 08:03:44PM +0100, Bartosz Golaszewski wrote: > > > > ... > > > > > > - it adds complications for no benefit > > It provides a placeholder for collective documentation and clarifies > scope for the reader. Turns out kernel-doc can't deal with a struct variable declaration - it needs the struct to be named. So this doesn't parse: static struct { struct rb_root tree; spinlock_t lock; } supinfo; but this does: static struct supinfo { struct rb_root tree; spinlock_t lock; } supinfo; at which point I prefer the separate struct and var declarations as per the patch. Opinions? Cheers, Kent.
On Thu, Dec 14, 2023 at 3:15 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Thu, Dec 14, 2023 at 08:18:01AM +0800, Kent Gibson wrote: > > On Wed, Dec 13, 2023 at 10:07:12PM +0200, Andy Shevchenko wrote: > > > On Wed, Dec 13, 2023 at 08:03:44PM +0100, Bartosz Golaszewski wrote: > > > > > > ... > > > > > > > > - it adds complications for no benefit > > > > It provides a placeholder for collective documentation and clarifies > > scope for the reader. > > Turns out kernel-doc can't deal with a struct variable declaration - it > needs the struct to be named. > > So this doesn't parse: > > static struct { > struct rb_root tree; > spinlock_t lock; > } supinfo; > > but this does: > > static struct supinfo { > struct rb_root tree; > spinlock_t lock; > } supinfo; > > at which point I prefer the separate struct and var declarations as per > the patch. > > Opinions? > Yeah, don't make it a kernel doc. It's a private structure, no need to expose documentation for it in docs. Just use a regular comment - say what it is and why it's here. Bart > Cheers, > Kent. >
On Thu, Dec 14, 2023 at 10:40:26AM +0100, Bartosz Golaszewski wrote: > On Thu, Dec 14, 2023 at 3:15 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Thu, Dec 14, 2023 at 08:18:01AM +0800, Kent Gibson wrote: > > > On Wed, Dec 13, 2023 at 10:07:12PM +0200, Andy Shevchenko wrote: > > > > On Wed, Dec 13, 2023 at 08:03:44PM +0100, Bartosz Golaszewski wrote: ... > > > > > > - it adds complications for no benefit > > > > > > It provides a placeholder for collective documentation and clarifies > > > scope for the reader. > > > > Turns out kernel-doc can't deal with a struct variable declaration - it > > needs the struct to be named. > > > > So this doesn't parse: > > > > static struct { > > struct rb_root tree; > > spinlock_t lock; > > } supinfo; > > > > but this does: > > > > static struct supinfo { > > struct rb_root tree; > > spinlock_t lock; > > } supinfo; > > > > at which point I prefer the separate struct and var declarations as per > > the patch. > > > > Opinions? > > Yeah, don't make it a kernel doc. It's a private structure, no need to > expose documentation for it in docs. Just use a regular comment - say > what it is and why it's here. I agree with Bart, make it plain comment if needed.
On Thu, Dec 14, 2023 at 04:35:09PM +0200, Andy Shevchenko wrote: > On Thu, Dec 14, 2023 at 10:40:26AM +0100, Bartosz Golaszewski wrote: > > ... > > > > Opinions? > > > > Yeah, don't make it a kernel doc. It's a private structure, no need to > > expose documentation for it in docs. Just use a regular comment - say > > what it is and why it's here. > > I agree with Bart, make it plain comment if needed. > It is a plain comment in v2. Cheers, Kent.
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index 02ffda6c1e51..7999c1a72cfa 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -21,6 +21,7 @@ #include <linux/mutex.h> #include <linux/pinctrl/consumer.h> #include <linux/poll.h> +#include <linux/rbtree.h> #include <linux/seq_file.h> #include <linux/spinlock.h> #include <linux/timekeeping.h> @@ -462,6 +463,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) /** * struct line - contains the state of a requested line * @desc: the GPIO descriptor for this line. + * @node: to store the object in supinfo if supplemental * @req: the corresponding line request * @irq: the interrupt triggered in response to events on this GPIO * @edflags: the edge flags, GPIO_V2_LINE_FLAG_EDGE_RISING and/or @@ -473,6 +475,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) * @line_seqno: the seqno for the current edge event in the sequence of * events for this line. * @work: the worker that implements software debouncing + * @debounce_period_us: the debounce period in microseconds * @sw_debounced: flag indicating if the software debouncer is active * @level: the current debounced physical level of the line * @hdesc: the Hardware Timestamp Engine (HTE) descriptor @@ -482,6 +485,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) */ struct line { struct gpio_desc *desc; + struct rb_node node; /* * -- edge detector specific fields -- */ @@ -514,6 +518,15 @@ struct line { * -- debouncer specific fields -- */ struct delayed_work work; + /* + * debounce_period_us is accessed by debounce_irq_handler() and + * process_hw_ts() which are disabled when modified by + * debounce_setup(), edge_detector_setup() or edge_detector_stop() + * or can live with a stale version when updated by + * edge_detector_update(). + * The modifying functions are themselves mutually exclusive. + */ + unsigned int debounce_period_us; /* * sw_debounce is accessed by linereq_set_config(), which is the * only setter, and linereq_get_values(), which can live with a @@ -546,6 +559,22 @@ struct line { #endif /* CONFIG_HTE */ }; +/** + * struct supinfo - supplementary line info + * Used to populate gpio_v2_line_info with cdev specific fields not contained + * in the struct gpio_desc. + * A line is determined to contain supplemental information by + * line_is_supplemental(). + * @lock: lock covering @tree + * @tree: a rbtree of the struct lines containing the supplemental info + */ +struct supinfo { + spinlock_t lock; + struct rb_root tree; +}; + +static struct supinfo supinfo; + /** * struct linereq - contains the state of a userspace line request * @gdev: the GPIO device the line request pertains to @@ -575,6 +604,100 @@ struct linereq { struct line lines[] __counted_by(num_lines); }; +static void supinfo_init(void) +{ + supinfo.tree = RB_ROOT; + spin_lock_init(&supinfo.lock); +} + +static void supinfo_insert(struct line *line) +{ + struct rb_node **new = &(supinfo.tree.rb_node), *parent = NULL; + struct line *entry; + + spin_lock(&supinfo.lock); + while (*new) { + entry = container_of(*new, struct line, node); + + parent = *new; + if (line->desc < entry->desc) { + new = &((*new)->rb_left); + } else if (line->desc > entry->desc) { + new = &((*new)->rb_right); + } else { + pr_warn("%s: duplicate line inserted\n", __func__); + goto out_unlock; + } + } + + rb_link_node(&line->node, parent, new); + rb_insert_color(&line->node, &supinfo.tree); +out_unlock: + spin_unlock(&supinfo.lock); +} + +static void supinfo_erase(struct line *line) +{ + spin_lock(&supinfo.lock); + rb_erase(&line->node, &supinfo.tree); + spin_unlock(&supinfo.lock); +} + +static struct line *supinfo_find(struct gpio_desc *desc) +{ + struct rb_node *node = supinfo.tree.rb_node; + struct line *line; + + while (node) { + line = container_of(node, struct line, node); + if (desc < line->desc) + node = node->rb_left; + else if (desc > line->desc) + node = node->rb_right; + else + return line; + } + return NULL; +} + +static void supinfo_to_lineinfo(struct gpio_desc *desc, + struct gpio_v2_line_info *info) +{ + struct gpio_v2_line_attribute *attr; + struct line *line; + + spin_lock(&supinfo.lock); + line = supinfo_find(desc); + if (line) { + attr = &info->attrs[info->num_attrs]; + attr->id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE; + attr->debounce_period_us = READ_ONCE(line->debounce_period_us); + info->num_attrs++; + } + spin_unlock(&supinfo.lock); +} + +static inline bool line_is_supplemental(struct line *line) +{ + return READ_ONCE(line->debounce_period_us) != 0; +} + +static void line_set_debounce_period(struct line *line, + unsigned int debounce_period_us) +{ + bool was_suppl = line_is_supplemental(line); + + WRITE_ONCE(line->debounce_period_us, debounce_period_us); + + if (line_is_supplemental(line) == was_suppl) + return; + + if (was_suppl) + supinfo_erase(line); + else + supinfo_insert(line); +} + #define GPIO_V2_LINE_BIAS_FLAGS \ (GPIO_V2_LINE_FLAG_BIAS_PULL_UP | \ GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN | \ @@ -723,7 +846,7 @@ static enum hte_return process_hw_ts(struct hte_ts_data *ts, void *p) line->total_discard_seq++; line->last_seqno = ts->seq; mod_delayed_work(system_wq, &line->work, - usecs_to_jiffies(READ_ONCE(line->desc->debounce_period_us))); + usecs_to_jiffies(READ_ONCE(line->debounce_period_us))); } else { if (unlikely(ts->seq < line->line_seqno)) return HTE_CB_HANDLED; @@ -864,7 +987,7 @@ static irqreturn_t debounce_irq_handler(int irq, void *p) struct line *line = p; mod_delayed_work(system_wq, &line->work, - usecs_to_jiffies(READ_ONCE(line->desc->debounce_period_us))); + usecs_to_jiffies(READ_ONCE(line->debounce_period_us))); return IRQ_HANDLED; } @@ -946,7 +1069,7 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us) /* try hardware */ ret = gpiod_set_debounce(line->desc, debounce_period_us); if (!ret) { - WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us); + line_set_debounce_period(line, debounce_period_us); return ret; } if (ret != -ENOTSUPP) @@ -1025,8 +1148,7 @@ static void edge_detector_stop(struct line *line) cancel_delayed_work_sync(&line->work); WRITE_ONCE(line->sw_debounced, 0); WRITE_ONCE(line->edflags, 0); - if (line->desc) - WRITE_ONCE(line->desc->debounce_period_us, 0); + line_set_debounce_period(line, 0); /* do not change line->level - see comment in debounced_value() */ } @@ -1051,7 +1173,7 @@ static int edge_detector_setup(struct line *line, ret = debounce_setup(line, debounce_period_us); if (ret) return ret; - WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us); + line_set_debounce_period(line, debounce_period_us); } /* detection disabled or sw debouncer will provide edge detection */ @@ -1093,12 +1215,12 @@ static int edge_detector_update(struct line *line, gpio_v2_line_config_debounce_period(lc, line_idx); if ((active_edflags == edflags) && - (READ_ONCE(line->desc->debounce_period_us) == debounce_period_us)) + (READ_ONCE(line->debounce_period_us) == debounce_period_us)) return 0; /* sw debounced and still will be...*/ if (debounce_period_us && READ_ONCE(line->sw_debounced)) { - WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us); + line_set_debounce_period(line, debounce_period_us); return 0; } @@ -1573,6 +1695,7 @@ static ssize_t linereq_read(struct file *file, char __user *buf, static void linereq_free(struct linereq *lr) { + struct line *line; unsigned int i; if (lr->device_unregistered_nb.notifier_call) @@ -1580,9 +1703,12 @@ static void linereq_free(struct linereq *lr) &lr->device_unregistered_nb); for (i = 0; i < lr->num_lines; i++) { - if (lr->lines[i].desc) { - edge_detector_stop(&lr->lines[i]); - gpiod_free(lr->lines[i].desc); + line = &lr->lines[i]; + if (line->desc) { + edge_detector_stop(line); + if (line_is_supplemental(line)) + supinfo_erase(line); + gpiod_free(line->desc); } } kfifo_free(&lr->events); @@ -2274,8 +2400,6 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc, struct gpio_chip *gc = desc->gdev->chip; bool ok_for_pinctrl; unsigned long flags; - u32 debounce_period_us; - unsigned int num_attrs = 0; memset(info, 0, sizeof(*info)); info->offset = gpio_chip_hwgpio(desc); @@ -2341,14 +2465,6 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc, else if (test_bit(FLAG_EVENT_CLOCK_HTE, &desc->flags)) info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE; - debounce_period_us = READ_ONCE(desc->debounce_period_us); - if (debounce_period_us) { - info->attrs[num_attrs].id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE; - info->attrs[num_attrs].debounce_period_us = debounce_period_us; - num_attrs++; - } - info->num_attrs = num_attrs; - spin_unlock_irqrestore(&gpio_lock, flags); } @@ -2455,6 +2571,7 @@ static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip, return -EBUSY; } gpio_desc_to_lineinfo(desc, &lineinfo); + supinfo_to_lineinfo(desc, &lineinfo); if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) { if (watch) @@ -2545,6 +2662,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb, chg.event_type = action; chg.timestamp_ns = ktime_get_ns(); gpio_desc_to_lineinfo(desc, &chg.info); + supinfo_to_lineinfo(desc, &chg.info); ret = kfifo_in_spinlocked(&cdev->events, &chg, 1, &cdev->wait.lock); if (ret) @@ -2812,3 +2930,10 @@ void gpiolib_cdev_unregister(struct gpio_device *gdev) cdev_device_del(&gdev->chrdev, &gdev->dev); blocking_notifier_call_chain(&gdev->device_notifier, 0, NULL); } + +static int __init gpiolib_cdev_init(void) +{ + supinfo_init(); + return 0; +} +postcore_initcall(gpiolib_cdev_init);
Store the debounce period for a requested line locally, rather than in the debounce_period_us field in the gpiolib struct gpio_desc. Add a global tree of lines containing supplemental line information to make the debounce period available to be reported by the GPIO_V2_GET_LINEINFO_IOCTL and the line change notifier. Signed-off-by: Kent Gibson <warthog618@gmail.com> --- drivers/gpio/gpiolib-cdev.c | 167 +++++++++++++++++++++++++++++++----- 1 file changed, 146 insertions(+), 21 deletions(-)