[1/4] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

Message ID 20231212054253.50094-2-warthog618@gmail.com
State New
Headers
Series gpiolib: cdev: relocate debounce_period_us |

Commit Message

Kent Gibson Dec. 12, 2023, 5:42 a.m. UTC
  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

andy@kernel.org Dec. 13, 2023, 1:54 p.m. UTC | #1
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);
  
Kent Gibson Dec. 13, 2023, 2:27 p.m. UTC | #2
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.
  
Bartosz Golaszewski Dec. 13, 2023, 3:40 p.m. UTC | #3
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
  
Kent Gibson Dec. 13, 2023, 3:59 p.m. UTC | #4
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.
  
andy@kernel.org Dec. 13, 2023, 4:12 p.m. UTC | #5
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.
  
Bartosz Golaszewski Dec. 13, 2023, 4:14 p.m. UTC | #6
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
  
Kent Gibson Dec. 13, 2023, 4:15 p.m. UTC | #7
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.
  
Bartosz Golaszewski Dec. 13, 2023, 4:15 p.m. UTC | #8
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
>
>
>
  
Bartosz Golaszewski Dec. 13, 2023, 4:16 p.m. UTC | #9
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.
  
andy@kernel.org Dec. 13, 2023, 4:27 p.m. UTC | #10
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.
  
andy@kernel.org Dec. 13, 2023, 4:29 p.m. UTC | #11
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.
  
Bartosz Golaszewski Dec. 13, 2023, 7:03 p.m. UTC | #12
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
>
>
  
andy@kernel.org Dec. 13, 2023, 8:07 p.m. UTC | #13
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.
  
Kent Gibson Dec. 14, 2023, 12:18 a.m. UTC | #14
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.
  
Kent Gibson Dec. 14, 2023, 2:15 a.m. UTC | #15
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.
  
Bartosz Golaszewski Dec. 14, 2023, 9:40 a.m. UTC | #16
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.
>
  
andy@kernel.org Dec. 14, 2023, 2:35 p.m. UTC | #17
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.
  
Kent Gibson Dec. 14, 2023, 2:47 p.m. UTC | #18
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.
  

Patch

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);