[1/4] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
Commit Message
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(-)
Comments
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.
@@ -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);