[v2,2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

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

Commit Message

Kent Gibson Dec. 14, 2023, 9:58 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, 145 insertions(+), 22 deletions(-)
  

Comments

Bartosz Golaszewski Dec. 14, 2023, 2:29 p.m. UTC | #1
On Thu, Dec 14, 2023 at 10:58 AM Kent Gibson <warthog618@gmail.com> 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.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>  drivers/gpio/gpiolib-cdev.c | 167 +++++++++++++++++++++++++++++++-----
>  1 file changed, 145 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index d03698812f61..7da3b3706547 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>
> @@ -461,6 +462,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>
>  /**
>   * struct line - contains the state of a requested line
> + * @node: to store the object in supinfo if supplemental
>   * @desc: the GPIO descriptor for this line.
>   * @req: the corresponding line request
>   * @irq: the interrupt triggered in response to events on this GPIO
> @@ -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
> @@ -481,6 +484,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>   * @last_seqno: the last sequence number before debounce period expires
>   */
>  struct line {
> +       struct rb_node node;
>         struct gpio_desc *desc;
>         /*
>          * -- 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,19 @@ struct line {
>  #endif /* CONFIG_HTE */
>  };
>
> +/*
> + * 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().
> + */
> +static struct {
> +       /* a rbtree of the struct lines containing the supplemental info */
> +       struct rb_root tree;
> +       /* covers tree */
> +       spinlock_t lock;

Looks like this is never taken from atomic context? Can this be a mutex instead?

> +} supinfo;
> +
>  /**
>   * struct linereq - contains the state of a userspace line request
>   * @gdev: the GPIO device the line request pertains to
> @@ -575,6 +601,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;
> +
> +       scoped_guard(spinlock, &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 {
> +                               /* this should never happen */
> +                               WARN(1, "duplicate line inserted");
> +                               return;
> +                       }
> +               }
> +
> +               rb_link_node(&line->node, parent, new);
> +               rb_insert_color(&line->node, &supinfo.tree);
> +       }
> +}
> +
> +static void supinfo_erase(struct line *line)
> +{
> +       scoped_guard(spinlock, &supinfo.lock)
> +               rb_erase(&line->node, &supinfo.tree);
> +}
> +
> +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;
> +
> +       scoped_guard(spinlock, &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++;
> +               }
> +       }
> +}
> +
> +static inline bool line_is_supplemental(struct line *line)

I would call this function line_has_suppinfo().

> +{
> +       return READ_ONCE(line->debounce_period_us);
> +}
> +
> +static void line_set_debounce_period(struct line *line,
> +                                    unsigned int debounce_period_us)
> +{
> +       bool was_suppl = line_is_supplemental(line);

This logic could use some comments, it took me a while to figure out
what it's doing.

> +
> +       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 +843,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 +984,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 +1066,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 +1145,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 +1170,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 +1212,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;
>         }
>
> @@ -1561,6 +1680,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)
> @@ -1568,10 +1688,14 @@ 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)
> +                       continue;
> +
> +               edge_detector_stop(line);
> +               if (line_is_supplemental(line))
> +                       supinfo_erase(line);
> +               gpiod_free(line->desc);
>         }
>         kfifo_free(&lr->events);
>         kfree(lr->label);
> @@ -2256,8 +2380,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);
> @@ -2323,14 +2445,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);
>  }
>
> @@ -2437,6 +2551,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)
> @@ -2527,6 +2642,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)
> @@ -2786,3 +2902,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);
> --
> 2.39.2
>

Bart
  
Kent Gibson Dec. 14, 2023, 2:45 p.m. UTC | #2
On Thu, Dec 14, 2023 at 03:29:28PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 14, 2023 at 10:58 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> >
> > +/*
> > + * 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().
> > + */
> > +static struct {
> > +       /* a rbtree of the struct lines containing the supplemental info */
> > +       struct rb_root tree;
> > +       /* covers tree */
> > +       spinlock_t lock;
>
> Looks like this is never taken from atomic context? Can this be a mutex instead?
>

Correct, only from thread context.

Can be a mutex, but it only covers tree lookups which should be quick
as the tree is kept minimal, and I wouldn't expect it to ever get to the
mutex slowpath, so a spinlock seemed more appropriate.

Cheers,
Kent.
  
Bartosz Golaszewski Dec. 14, 2023, 2:56 p.m. UTC | #3
On Thu, Dec 14, 2023 at 3:45 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Dec 14, 2023 at 03:29:28PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Dec 14, 2023 at 10:58 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > >
> > > +/*
> > > + * 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().
> > > + */
> > > +static struct {
> > > +       /* a rbtree of the struct lines containing the supplemental info */
> > > +       struct rb_root tree;
> > > +       /* covers tree */
> > > +       spinlock_t lock;
> >
> > Looks like this is never taken from atomic context? Can this be a mutex instead?
> >
>
> Correct, only from thread context.
>
> Can be a mutex, but it only covers tree lookups which should be quick
> as the tree is kept minimal, and I wouldn't expect it to ever get to the
> mutex slowpath, so a spinlock seemed more appropriate.
>

Fair enough.

Bart

> Cheers,
> Kent.
  
andy@kernel.org Dec. 14, 2023, 3:03 p.m. UTC | #4
On Thu, Dec 14, 2023 at 05:58:11PM +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.

...

> +/*
> + * 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().
> + */
> +static struct {
> +	/* a rbtree of the struct lines containing the supplemental info */
> +	struct rb_root tree;
> +	/* covers tree */
> +	spinlock_t lock;
> +} supinfo;

...

> +static void supinfo_init(void)
> +{
> +	supinfo.tree = RB_ROOT;
> +	spin_lock_init(&supinfo.lock);
> +}

Can it be done statically?

supinfo = {
	.tree = RB_ROOT,
	.lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
};

...

> +static int __init gpiolib_cdev_init(void)
> +{
> +	supinfo_init();
> +	return 0;
> +}

A comment why it's postcore initcall?

/* postcore initcall is chosen because ... */

> +postcore_initcall(gpiolib_cdev_init);
  
Kent Gibson Dec. 14, 2023, 3:08 p.m. UTC | #5
On Thu, Dec 14, 2023 at 03:56:37PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 14, 2023 at 3:45 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Dec 14, 2023 at 03:29:28PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Dec 14, 2023 at 10:58 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > >
> > > > +/*
> > > > + * 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().
> > > > + */
> > > > +static struct {
> > > > +       /* a rbtree of the struct lines containing the supplemental info */
> > > > +       struct rb_root tree;
> > > > +       /* covers tree */
> > > > +       spinlock_t lock;
> > >
> > > Looks like this is never taken from atomic context? Can this be a mutex instead?
> > >
> >
> > Correct, only from thread context.
> >
> > Can be a mutex, but it only covers tree lookups which should be quick
> > as the tree is kept minimal, and I wouldn't expect it to ever get to the
> > mutex slowpath, so a spinlock seemed more appropriate.
> >
>
> Fair enough.
>
> Bart
>

While I think of it, what tree should I be basing on?
These patches are based on v6.7-rc5, and I'm not aware of any other
changes they may contend with, but best to be on the right tree to be
sure.

Cheers,
Kent.
  
andy@kernel.org Dec. 14, 2023, 3:09 p.m. UTC | #6
On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:

...

> > +/*
> > + * 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().
> > + */
> > +static struct {
> > +	/* a rbtree of the struct lines containing the supplemental info */
> > +	struct rb_root tree;
> > +	/* covers tree */
> > +	spinlock_t lock;
> > +} supinfo;

Hmm... If I read the kernel-doc script it should support anonymous structs
and unions...

...

> > +static void supinfo_init(void)
> > +{
> > +	supinfo.tree = RB_ROOT;
> > +	spin_lock_init(&supinfo.lock);
> > +}
> 
> Can it be done statically?
> 
> supinfo = {
> 	.tree = RB_ROOT,
> 	.lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),

I even checked the current tree, we have 32 users of this pattern in drivers/.

> };
  
Kent Gibson Dec. 14, 2023, 4:14 p.m. UTC | #7
On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
>
> ...
>
> > > +/*
> > > + * 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().
> > > + */
> > > +static struct {
> > > +	/* a rbtree of the struct lines containing the supplemental info */
> > > +	struct rb_root tree;
> > > +	/* covers tree */
> > > +	spinlock_t lock;
> > > +} supinfo;
>
> Hmm... If I read the kernel-doc script it should support anonymous structs
> and unions...
>
> ...
>
> > > +static void supinfo_init(void)
> > > +{
> > > +	supinfo.tree = RB_ROOT;
> > > +	spin_lock_init(&supinfo.lock);
> > > +}
> >
> > Can it be done statically?
> >
> > supinfo = {
> > 	.tree = RB_ROOT,
> > 	.lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
>
> I even checked the current tree, we have 32 users of this pattern in drivers/.
>

Ah, that is what you meant.  Yeah sure can - the supinfo_init() is
another hangover from when I was trying to create the supinfo per chip,
but now it is a global a static initialiser makes sense.

And I still haven't received the email you quote there.

Cheers,
Kent.
  
andy@kernel.org Dec. 14, 2023, 4:41 p.m. UTC | #8
On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:

...

> > > > +static void supinfo_init(void)
> > > > +{
> > > > +	supinfo.tree = RB_ROOT;
> > > > +	spin_lock_init(&supinfo.lock);
> > > > +}
> > >
> > > Can it be done statically?
> > >
> > > supinfo = {
> > > 	.tree = RB_ROOT,
> > > 	.lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
> >
> > I even checked the current tree, we have 32 users of this pattern in drivers/.
> 
> Ah, that is what you meant.  Yeah sure can - the supinfo_init() is
> another hangover from when I was trying to create the supinfo per chip,
> but now it is a global a static initialiser makes sense.

Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
than above.

> And I still haven't received the email you quote there.

:-( I'm not sure we will get it, it most likely that I removed it already
and it has disappeared due to problems with email server...
  
Bartosz Golaszewski Dec. 14, 2023, 9:06 p.m. UTC | #9
On Thu, Dec 14, 2023 at 5:41 PM Andy Shevchenko <andy@kernel.org> wrote:
>
> On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> > On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
>
> ...
>
> > > > > +static void supinfo_init(void)
> > > > > +{
> > > > > +       supinfo.tree = RB_ROOT;
> > > > > +       spin_lock_init(&supinfo.lock);
> > > > > +}
> > > >
> > > > Can it be done statically?
> > > >
> > > > supinfo = {
> > > >   .tree = RB_ROOT,
> > > >   .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),

Double underscore typically means it's private and shouldn't be used.

> > >
> > > I even checked the current tree, we have 32 users of this pattern in drivers/.
> >
> > Ah, that is what you meant.  Yeah sure can - the supinfo_init() is
> > another hangover from when I was trying to create the supinfo per chip,
> > but now it is a global a static initialiser makes sense.
>
> Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
> than above.

Yeah, so maybe we should use non-struct, global variables after all.

Bart

>
> > And I still haven't received the email you quote there.
>
> :-( I'm not sure we will get it, it most likely that I removed it already
> and it has disappeared due to problems with email server...
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
  
Kent Gibson Dec. 15, 2023, 1:04 a.m. UTC | #10
On Thu, Dec 14, 2023 at 10:06:14PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 14, 2023 at 5:41 PM Andy Shevchenko <andy@kernel.org> wrote:
> >
> > On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> > > On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
> >
> > ...
> >
> > > > > > +static void supinfo_init(void)
> > > > > > +{
> > > > > > +       supinfo.tree = RB_ROOT;
> > > > > > +       spin_lock_init(&supinfo.lock);
> > > > > > +}
> > > > >
> > > > > Can it be done statically?
> > > > >
> > > > > supinfo = {
> > > > >   .tree = RB_ROOT,
> > > > >   .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
>
> Double underscore typically means it's private and shouldn't be used.
>

You mean like __assign_bit(), __set_bit(), __clear_bit() and __free() -
all used in gpiolib.c?

> > > >
> > > > I even checked the current tree, we have 32 users of this pattern in drivers/.
> > >
> > > Ah, that is what you meant.  Yeah sure can - the supinfo_init() is
> > > another hangover from when I was trying to create the supinfo per chip,
> > > but now it is a global a static initialiser makes sense.
> >
> > Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
> > than above.
>
> Yeah, so maybe we should use non-struct, global variables after all.
>

Despite the 32 cases cited that already use that pattern?
9 of which use __SPIN_LOCK_UNLOCKED().
Sounds like a pretty convincing argument to use the struct ;-).

But lets keep it as kosher as possible and split out the struct :-(.

Cheers,
Kent.
  
Bartosz Golaszewski Dec. 15, 2023, 8:07 a.m. UTC | #11
On Fri, Dec 15, 2023 at 2:04 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Dec 14, 2023 at 10:06:14PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Dec 14, 2023 at 5:41 PM Andy Shevchenko <andy@kernel.org> wrote:
> > >
> > > On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> > > > On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > > > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
> > >
> > > ...
> > >
> > > > > > > +static void supinfo_init(void)
> > > > > > > +{
> > > > > > > +       supinfo.tree = RB_ROOT;
> > > > > > > +       spin_lock_init(&supinfo.lock);
> > > > > > > +}
> > > > > >
> > > > > > Can it be done statically?
> > > > > >
> > > > > > supinfo = {
> > > > > >   .tree = RB_ROOT,
> > > > > >   .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
> >
> > Double underscore typically means it's private and shouldn't be used.
> >
>
> You mean like __assign_bit(), __set_bit(), __clear_bit() and __free() -
> all used in gpiolib.c?
>

Touché. But this is just lack of strict naming conventions. :( Another
common use of leading underscores are "unlocked" (or in this case:
non-atomic) variants of functions.

> > > > >
> > > > > I even checked the current tree, we have 32 users of this pattern in drivers/.
> > > >

As opposed to ~1200 uses of DEFINE_SPINLOCK if you really want to go there. :)

> > > > Ah, that is what you meant.  Yeah sure can - the supinfo_init() is
> > > > another hangover from when I was trying to create the supinfo per chip,
> > > > but now it is a global a static initialiser makes sense.
> > >
> > > Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
> > > than above.
> >
> > Yeah, so maybe we should use non-struct, global variables after all.
> >
>
> Despite the 32 cases cited that already use that pattern?
> 9 of which use __SPIN_LOCK_UNLOCKED().
> Sounds like a pretty convincing argument to use the struct ;-).
>
> But lets keep it as kosher as possible and split out the struct :-(.
>

I'll leave it for you to decide, I don't have a strong opinion and the
entire file is your code so you should pick.

Bart

> Cheers,
> Kent.
>
  
Kent Gibson Dec. 15, 2023, 8:15 a.m. UTC | #12
On Fri, Dec 15, 2023 at 09:07:48AM +0100, Bartosz Golaszewski wrote:
> On Fri, Dec 15, 2023 at 2:04 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Dec 14, 2023 at 10:06:14PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Dec 14, 2023 at 5:41 PM Andy Shevchenko <andy@kernel.org> wrote:
> > > >
> > > > On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> > > > > On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > > > > > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > > > > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
> > > >
> > > > ...
> > > >
> > > > > > > > +static void supinfo_init(void)
> > > > > > > > +{
> > > > > > > > +       supinfo.tree = RB_ROOT;
> > > > > > > > +       spin_lock_init(&supinfo.lock);
> > > > > > > > +}
> > > > > > >
> > > > > > > Can it be done statically?
> > > > > > >
> > > > > > > supinfo = {
> > > > > > >   .tree = RB_ROOT,
> > > > > > >   .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
> > >
> > > Double underscore typically means it's private and shouldn't be used.
> > >
> >
> > You mean like __assign_bit(), __set_bit(), __clear_bit() and __free() -
> > all used in gpiolib.c?
> >
>
> Touché. But this is just lack of strict naming conventions. :( Another
> common use of leading underscores are "unlocked" (or in this case:
> non-atomic) variants of functions.
>

Sorry, should've added a ;-) to the end of that one - not giving you a
hard time, just found it amusing.

> > > > > >
> > > > > > I even checked the current tree, we have 32 users of this pattern in drivers/.
> > > > >
>
> As opposed to ~1200 uses of DEFINE_SPINLOCK if you really want to go there. :)
>

To be clear, that is Andy's quote you are replying to :-).

> > > > > Ah, that is what you meant.  Yeah sure can - the supinfo_init() is
> > > > > another hangover from when I was trying to create the supinfo per chip,
> > > > > but now it is a global a static initialiser makes sense.
> > > >
> > > > Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
> > > > than above.
> > >
> > > Yeah, so maybe we should use non-struct, global variables after all.
> > >
> >
> > Despite the 32 cases cited that already use that pattern?
> > 9 of which use __SPIN_LOCK_UNLOCKED().
> > Sounds like a pretty convincing argument to use the struct ;-).
> >
> > But lets keep it as kosher as possible and split out the struct :-(.
> >
>
> I'll leave it for you to decide, I don't have a strong opinion and the
> entire file is your code so you should pick.
>

I've split it out in v3.

Cheers,
Kent.
  
andy@kernel.org Dec. 15, 2023, 4:31 p.m. UTC | #13
On Thu, Dec 14, 2023 at 10:06:14PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 14, 2023 at 5:41 PM Andy Shevchenko <andy@kernel.org> wrote:
> > On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> > > On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:

...

> > > > > > +static void supinfo_init(void)
> > > > > > +{
> > > > > > +       supinfo.tree = RB_ROOT;
> > > > > > +       spin_lock_init(&supinfo.lock);
> > > > > > +}
> > > > >
> > > > > Can it be done statically?
> > > > >
> > > > > supinfo = {
> > > > >   .tree = RB_ROOT,
> > > > >   .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
> 
> Double underscore typically means it's private and shouldn't be used.

Right, but when you have a struct you have no other means to initialize this
directly.

> > > > I even checked the current tree, we have 32 users of this pattern in drivers/.

See, there are users of the __ initializers.

> > > Ah, that is what you meant.  Yeah sure can - the supinfo_init() is
> > > another hangover from when I was trying to create the supinfo per chip,
> > > but now it is a global a static initialiser makes sense.
> >
> > Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
> > than above.
> 
> Yeah, so maybe we should use non-struct, global variables after all.

At least this will allow to get rid of (questionable) initcall.

> > > And I still haven't received the email you quote there.
> >
> > :-( I'm not sure we will get it, it most likely that I removed it already
> > and it has disappeared due to problems with email server...
  

Patch

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index d03698812f61..7da3b3706547 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>
@@ -461,6 +462,7 @@  static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 
 /**
  * struct line - contains the state of a requested line
+ * @node: to store the object in supinfo if supplemental
  * @desc: the GPIO descriptor for this line.
  * @req: the corresponding line request
  * @irq: the interrupt triggered in response to events on this GPIO
@@ -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
@@ -481,6 +484,7 @@  static int linehandle_create(struct gpio_device *gdev, void __user *ip)
  * @last_seqno: the last sequence number before debounce period expires
  */
 struct line {
+	struct rb_node node;
 	struct gpio_desc *desc;
 	/*
 	 * -- 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,19 @@  struct line {
 #endif /* CONFIG_HTE */
 };
 
+/*
+ * 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().
+ */
+static struct {
+	/* a rbtree of the struct lines containing the supplemental info */
+	struct rb_root tree;
+	/* covers tree */
+	spinlock_t lock;
+} supinfo;
+
 /**
  * struct linereq - contains the state of a userspace line request
  * @gdev: the GPIO device the line request pertains to
@@ -575,6 +601,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;
+
+	scoped_guard(spinlock, &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 {
+				/* this should never happen */
+				WARN(1, "duplicate line inserted");
+				return;
+			}
+		}
+
+		rb_link_node(&line->node, parent, new);
+		rb_insert_color(&line->node, &supinfo.tree);
+	}
+}
+
+static void supinfo_erase(struct line *line)
+{
+	scoped_guard(spinlock, &supinfo.lock)
+		rb_erase(&line->node, &supinfo.tree);
+}
+
+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;
+
+	scoped_guard(spinlock, &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++;
+		}
+	}
+}
+
+static inline bool line_is_supplemental(struct line *line)
+{
+	return READ_ONCE(line->debounce_period_us);
+}
+
+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 +843,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 +984,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 +1066,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 +1145,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 +1170,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 +1212,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;
 	}
 
@@ -1561,6 +1680,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)
@@ -1568,10 +1688,14 @@  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)
+			continue;
+
+		edge_detector_stop(line);
+		if (line_is_supplemental(line))
+			supinfo_erase(line);
+		gpiod_free(line->desc);
 	}
 	kfifo_free(&lr->events);
 	kfree(lr->label);
@@ -2256,8 +2380,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);
@@ -2323,14 +2445,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);
 }
 
@@ -2437,6 +2551,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)
@@ -2527,6 +2642,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)
@@ -2786,3 +2902,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);