Re: [PATCH 20/22] gpio: protect the pointer to gpio_chip in gpio_device with SRCU

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Wed, Jan 31, 2024 at 12:00 PM Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
>
> On Wed, Jan 31, 2024 at 02:17:30AM -0800, brgl@xxxxxxxx wrote:
> > On Wed, 31 Jan 2024 10:42:20 +0100, "Paul E. McKenney"
> > <paulmck@xxxxxxxxxx> said:
> > > On Wed, Jan 31, 2024 at 10:28:19AM +0100, Bartosz Golaszewski wrote:
> > >> On Wed, Jan 31, 2024 at 10:24 AM Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
> > >> >
> > >> > On Wed, Jan 31, 2024 at 10:02:40AM +0100, Bartosz Golaszewski wrote:
> > >> > > On Wed, Jan 31, 2024 at 3:21 AM kernel test robot <lkp@xxxxxxxxx> wrote:
> > >> > > >
> > >> > > > Hi Bartosz,
> > >> > > >
> > >> > > > kernel test robot noticed the following build warnings:
> > >> > > >
> > >> > > > [auto build test WARNING on brgl/gpio/for-next]
> > >> > > > [also build test WARNING on linus/master v6.8-rc2 next-20240130]
> > >> > > > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > >> > > > And when submitting patch, we suggest to use '--base' as documented in
> > >> > > > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > >> > > >
> > >> > > > url:    https://github.com/intel-lab-lkp/linux/commits/Bartosz-Golaszewski/gpio-protect-the-list-of-GPIO-devices-with-SRCU/20240130-205537
> > >> > > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
> > >> > > > patch link:    https://lore.kernel.org/r/20240130124828.14678-21-brgl%40bgdev.pl
> > >> > > > patch subject: [PATCH 20/22] gpio: protect the pointer to gpio_chip in gpio_device with SRCU
> > >> > > > config: x86_64-randconfig-122-20240131 (https://download.01.org/0day-ci/archive/20240131/202401311050.YNdm98Hv-lkp@xxxxxxxxx/config)
> > >> > > > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > >> > > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240131/202401311050.YNdm98Hv-lkp@xxxxxxxxx/reproduce)
> > >> > > >
> > >> > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > >> > > > the same patch/commit), kindly add following tags
> > >> > > > | Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > >> > > > | Closes: https://lore.kernel.org/oe-kbuild-all/202401311050.YNdm98Hv-lkp@xxxxxxxxx/
> > >> > > >
> > >> > > > sparse warnings: (new ones prefixed by >>)
> > >> > > > >> drivers/gpio/gpiolib.c:444:22: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.c:444:22: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.c:444:22: sparse:    struct gpio_chip *
> > >> > > >    drivers/gpio/gpiolib.c:1103:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.c:1103:9: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.c:1103:9: sparse:    struct gpio_chip *
> > >> > > >    drivers/gpio/gpiolib.c:1182:22: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.c:1182:22: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.c:1182:22: sparse:    struct gpio_chip *
> > >> > > >    drivers/gpio/gpiolib.c:2970:14: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.c:2970:14: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.c:2970:14: sparse:    struct gpio_chip *
> > >> > > >    drivers/gpio/gpiolib.c:3004:22: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.c:3004:22: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.c:3004:22: sparse:    struct gpio_chip *
> > >> > > >    drivers/gpio/gpiolib.c:3585:14: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.c:3585:14: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.c:3585:14: sparse:    struct gpio_chip *
> > >> > > >    drivers/gpio/gpiolib.c:4772:14: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.c:4772:14: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.c:4772:14: sparse:    struct gpio_chip *
> > >> > > >    drivers/gpio/gpiolib.c:4846:14: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.c:4846:14: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.c:4846:14: sparse:    struct gpio_chip *
> > >> > > >    drivers/gpio/gpiolib.c: note: in included file:
> > >> > > > >> drivers/gpio/gpiolib.h:202:1: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip *
> > >> > > > >> drivers/gpio/gpiolib.h:202:1: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip *
> > >> > > > >> drivers/gpio/gpiolib.h:202:1: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip *
> > >> > > > >> drivers/gpio/gpiolib.h:202:1: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip *
> > >> > > > >> drivers/gpio/gpiolib.h:202:1: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip *
> > >> > > > >> drivers/gpio/gpiolib.h:202:1: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip *
> > >> > > > >> drivers/gpio/gpiolib.h:202:1: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip *
> > >> > > > >> drivers/gpio/gpiolib.h:202:1: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip *
> > >> > > > >> drivers/gpio/gpiolib.h:202:1: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip *
> > >> > > > >> drivers/gpio/gpiolib.h:202:1: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip *
> > >> > > > >> drivers/gpio/gpiolib.h:202:1: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip *
> > >> > > > >> drivers/gpio/gpiolib.h:202:1: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip *
> > >> > > > >> drivers/gpio/gpiolib.h:202:1: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip *
> > >> > > > >> drivers/gpio/gpiolib.h:202:1: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip [noderef] __rcu *
> > >> > > >    drivers/gpio/gpiolib.h:202:1: sparse:    struct gpio_chip *
> > >> > > >
> > >> > > > vim +444 drivers/gpio/gpiolib.c
> > >> > > >
> > >> > > >    422
> > >> > > >    423  /*
> > >> > > >    424   * Convert a GPIO name to its descriptor
> > >> > > >    425   * Note that there is no guarantee that GPIO names are globally unique!
> > >> > > >    426   * Hence this function will return, if it exists, a reference to the first GPIO
> > >> > > >    427   * line found that matches the given name.
> > >> > > >    428   */
> > >> > > >    429  static struct gpio_desc *gpio_name_to_desc(const char * const name)
> > >> > > >    430  {
> > >> > > >    431          struct gpio_device *gdev;
> > >> > > >    432          struct gpio_desc *desc;
> > >> > > >    433          struct gpio_chip *gc;
> > >> > > >    434
> > >> > > >    435          if (!name)
> > >> > > >    436                  return NULL;
> > >> > > >    437
> > >> > > >    438          guard(srcu)(&gpio_devices_srcu);
> > >> > > >    439
> > >> > > >    440          list_for_each_entry_srcu(gdev, &gpio_devices, list,
> > >> > > >    441                                   srcu_read_lock_held(&gpio_devices_srcu)) {
> > >> > > >    442                  guard(srcu)(&gdev->srcu);
> > >> > > >    443
> > >> > > >  > 444                  gc = rcu_dereference(gdev->chip);
> > >> > > >    445                  if (!gc)
> > >> > > >    446                          continue;
> > >> > > >    447
> > >> > > >    448                  for_each_gpio_desc(gc, desc) {
> > >> > > >    449                          if (desc->name && !strcmp(desc->name, name))
> > >> > > >    450                                  return desc;
> > >> > > >    451                  }
> > >> > > >    452          }
> > >> > > >    453
> > >> > > >    454          return NULL;
> > >> > > >    455  }
> > >> > > >    456
> > >> > > >
> > >> > > > --
> > >> > > > 0-DAY CI Kernel Test Service
> > >> > > > https://github.com/intel/lkp-tests/wiki
> > >> > >
> > >> > > Paul,
> > >> > >
> > >> > > Should I care about these warnings? They seem to be emitted for a lot
> > >> > > of RCU code already upstream. I'm not even sure how I'd go about
> > >> > > addressing them honestly.
> > >> >
> > >> > This is maintainer's choice.
> > >> >
> > >> > The fix would be to apply __rcu to the definition of ->chip.  The benefit
> > >> > is that it finds bugs where rcu-protected pointers are used without RCU
> > >> > primitives and vice versa.
> > >>
> > >> Ah, good point. I marked the other RCU-protected fields like
> > >> descriptor label but forgot this one.
> > >>
> > >> It also seems like I need to use __rcu for all function arguments
> > >> taking an RCU-protected pointer as argument?
> > >
> > > Not if that argument gets its value from rcu_dereference(), which is
> > > the usual pattern.
> > >
> > > And if you are passing an RCU-protected pointer to a function *before*
> > > passing it through rcu_dereference(), I would have some questions for you.
> > >
> > > Or are you instead passing a pointer to an RCU-protected pointer as an
> > > argument to your functions?
> >
> > No, I'm not. Thanks for making it clear. With the following changes to the
> > series, the warnings are now gone:
> >
> > diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> > index 6a421309319e..15349f92d0ec 100644
> > --- a/drivers/gpio/gpiolib-sysfs.c
> > +++ b/drivers/gpio/gpiolib-sysfs.c
> > @@ -762,7 +762,7 @@ EXPORT_SYMBOL_GPL(gpiod_unexport);
> >
> >  int gpiochip_sysfs_register(struct gpio_device *gdev)
> >  {
> > -     struct gpio_chip *chip = gdev->chip;
> > +     struct gpio_chip *chip;
>
> This is protected by an update-side lock, correct?
>

No, it may be called from two places. One needs to be protected (it
isn't in my series but I'll fix it), the other does not as we are
holding the pointer to gdev already.

> >       struct device *parent;
> >       struct device *dev;
> >
> > @@ -775,6 +775,12 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
> >       if (!class_is_registered(&gpio_class))
> >               return 0;
> >
> > +     guard(srcu)(&gdev->srcu);
> > +
> > +     chip = rcu_dereference(gdev->chip);
>
> If so, you rely on that lock's protection by replacing the above
> two lines of code with something like this:
>
>         chip = rcu_dereference_protected(gdev->chip,
>                                          lockdep_is_held(&my_lock));
>

Thanks, I'll see if I can use it elsewhere.

Bart

> > +     if (!chip)
> > +             return -ENODEV;
> > +
> >       /*
> >        * For sysfs backward compatibility we need to preserve this
> >        * preferred parenting to the gpio_chip parent field, if set.
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 7ecdd8cc39c5..ea0675514891 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -221,7 +221,7 @@ struct gpio_chip *gpiod_to_chip(const struct
> > gpio_desc *desc)
> >  {
> >       if (!desc)
> >               return NULL;
> > -     return desc->gdev->chip;
> > +     return rcu_dereference(desc->gdev->chip);
> >  }
> >  EXPORT_SYMBOL_GPL(gpiod_to_chip);
> >
> > @@ -291,7 +291,7 @@ EXPORT_SYMBOL(gpio_device_get_label);
> >   */
> >  struct gpio_chip *gpio_device_get_chip(struct gpio_device *gdev)
> >  {
> > -     return gdev->chip;
> > +     return rcu_dereference(gdev->chip);
> >  }
> >  EXPORT_SYMBOL_GPL(gpio_device_get_chip);
> >
> > @@ -742,7 +742,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
> >               goto err_remove_device;
> >
> >       dev_dbg(&gdev->dev, "registered GPIOs %d to %d on %s\n", gdev->base,
> > -             gdev->base + gdev->ngpio - 1, gdev->chip->label ? : "generic");
> > +             gdev->base + gdev->ngpio - 1, gdev->label);
>
> I don't know enough to comment here, but the rest of the look good to me.
>
> >       return 0;
> >
> > @@ -866,7 +866,7 @@ int gpiochip_add_data_with_key(struct gpio_chip
> > *gc, void *data,
> >               return -ENOMEM;
> >       gdev->dev.bus = &gpio_bus_type;
> >       gdev->dev.parent = gc->parent;
> > -     WRITE_ONCE(gdev->chip, gc);
> > +     rcu_assign_pointer(gdev->chip, gc);
> >
> >       gc->gpiodev = gdev;
> >       gpiochip_set_data(gc, data);
> > diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> > index c76acb8f95c6..07443d26cbca 100644
> > --- a/drivers/gpio/gpiolib.h
> > +++ b/drivers/gpio/gpiolib.h
> > @@ -59,7 +59,7 @@ struct gpio_device {
> >       int                     id;
> >       struct device           *mockdev;
> >       struct module           *owner;
> > -     struct gpio_chip        *chip;
> > +     struct gpio_chip __rcu  *chip;
> >       struct gpio_desc        *descs;
> >       int                     base;
> >       u16                     ngpio;
> >
> > Bartosz