Re: [PATCH v4 4/6] gpio: Ensure struct gpio_chip for gpiochip_setup_dev()

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Sat, Feb 28, 2026 at 11:03:35AM +0100, Bartosz Golaszewski wrote:
> On Fri, Feb 27, 2026 at 10:36 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
> >
> > On 23.02.2026 07:17, Tzung-Bi Shih wrote:
> > > Ensure struct gpio_chip for gpiochip_setup_dev().  This eliminates a few
> > > checks for struct gpio_chip.
> > >
> > > Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> >
> > This patch landed in today's linux-next as commit cf674f1a0c98 ("gpio:
> > Ensure struct gpio_chip for gpiochip_setup_dev()"). In my tests I found
> > that it triggers a warning on every test board I have, so I suspect that
> > something is missing in the code. Here is an example of such warning:
> >
> > ------------[ cut here ]------------
> > WARNING: drivers/gpio/gpiolib-cdev.c:2735 at
> > gpiolib_cdev_register+0x114/0x140, CPU#1: swapper/0/1
> > Modules linked in:
> > CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted
> > 7.0.0-rc1-next-20260227-00065-g6af4b9cfeded #12259 PREEMPT
> > Hardware name: Samsung Exynos (Flattened Device Tree)
> > Call trace:
> >   unwind_backtrace from show_stack+0x10/0x14
> >   show_stack from dump_stack_lvl+0x68/0x88
> >   dump_stack_lvl from __warn+0x94/0x210
> >   __warn from warn_slowpath_fmt+0x1b0/0x1bc
> >   warn_slowpath_fmt from gpiolib_cdev_register+0x114/0x140
> >   gpiolib_cdev_register from gpiochip_setup_dev+0x4c/0xd0
> >   gpiochip_setup_dev from gpiochip_add_data_with_key+0x960/0xad4
> >   gpiochip_add_data_with_key from devm_gpiochip_add_data_with_key+0x20/0x5c
> >   devm_gpiochip_add_data_with_key from samsung_pinctrl_probe+0x8fc/0xbe8
[...]
> > > ---
> > > v4:
> > > - To be consistent, rename `chip` -> `gc`.
> > > - Add lockdep checks.
> > >
[...]
> > > -int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
> > > +int gpiolib_cdev_register(struct gpio_chip *gc, dev_t devt)
> > >   {
> > > -     struct gpio_chip *gc;
> > > +     struct gpio_device *gdev = gc->gpiodev;
> > >       int ret;
> > >
> > > +     lockdep_assert_held(&gdev->srcu);
> > > +
> > >       cdev_init(&gdev->chrdev, &gpio_fileops);
> > >       gdev->chrdev.owner = THIS_MODULE;
> > >       gdev->dev.devt = MKDEV(MAJOR(devt), gdev->id);
> > > @@ -2802,14 +2804,6 @@ int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
> > >               return ret;
> > >       }
> > >
> > > -     guard(srcu)(&gdev->srcu);
> > > -     gc = srcu_dereference(gdev->chip, &gdev->srcu);
> > > -     if (!gc) {
> > > -             cdev_device_del(&gdev->chrdev, &gdev->dev);
> > > -             destroy_workqueue(gdev->line_state_wq);
> > > -             return -ENODEV;
> > > -     }
> > > -
> > >       gpiochip_dbg(gc, "added GPIO chardev (%d:%d)\n", MAJOR(devt), gdev->id);
> > >
> > >       return 0;
[...]
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index d5070c538ba5..44635e9a29c3 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -881,14 +881,14 @@ static const struct device_type gpio_dev_type = {
> > >   };
> > >
> > >   #ifdef CONFIG_GPIO_CDEV
> > > -#define gcdev_register(gdev, devt)   gpiolib_cdev_register((gdev), (devt))
> > > +#define gcdev_register(gc, devt)     gpiolib_cdev_register((gc), (devt))
[...]
> > > -static int gpiochip_setup_dev(struct gpio_device *gdev)
> > > +static int gpiochip_setup_dev(struct gpio_chip *gc)
> > >   {
> > > +     struct gpio_device *gdev = gc->gpiodev;
> > >       struct fwnode_handle *fwnode = dev_fwnode(&gdev->dev);
> > >       int ret;
> > >
> > > @@ -910,11 +911,11 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
> > >       if (fwnode && !fwnode->dev)
> > >               fwnode_dev_initialized(fwnode, false);
> > >
> > > -     ret = gcdev_register(gdev, gpio_devt);
> > > +     ret = gcdev_register(gc, gpio_devt);
> > >       if (ret)
> > >               return ret;
> > >
> > > -     ret = gpiochip_sysfs_register(gdev);
> > > +     ret = gpiochip_sysfs_register(gc);
> > >       if (ret)
> > >               goto err_remove_device;
> > >
> > > @@ -961,13 +962,22 @@ static void machine_gpiochip_add(struct gpio_chip *gc)
> > >   static void gpiochip_setup_devs(void)
> > >   {
> > >       struct gpio_device *gdev;
> > > +     struct gpio_chip *gc;
> > >       int ret;
> > >
> > >       guard(srcu)(&gpio_devices_srcu);
> > >
> > >       list_for_each_entry_srcu(gdev, &gpio_devices, list,
> > >                                srcu_read_lock_held(&gpio_devices_srcu)) {
> > > -             ret = gpiochip_setup_dev(gdev);
> > > +             guard(srcu)(&gdev->srcu);
> > > +
> > > +             gc = srcu_dereference(gdev->chip, &gdev->srcu);
> > > +             if (!gc) {
> > > +                     dev_err(&gdev->dev, "Underlying GPIO chip is gone\n");
> > > +                     continue;
> > > +             }
> > > +
> > > +             ret = gpiochip_setup_dev(gc);
> > >               if (ret) {
> > >                       gpio_device_put(gdev);
> > >                       dev_err(&gdev->dev,
> > > @@ -1225,7 +1235,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> > >        * (i.e., `gpio_bus_type` is ready).  Otherwise, defer until later.
> > >        */
> > >       if (gpiolib_initialized) {
> > > -             ret = gpiochip_setup_dev(gdev);
> > > +             ret = gpiochip_setup_dev(gc);
> > >               if (ret)
> > >                       goto err_teardown_shared;
> > >       }
> >
> 
> gpiolib_cdev_register() is only called from
> gpiochip_add_data_with_key(). I don't think we need the lockdep check
> in the former?

It the calling path is:

  gpiochip_setup_devs()
  -> gpiochip_setup_dev()
  -> ...

The lockdep check should work.

If the calling path is:

  gpiochip_add_data_with_key()
  -> gpiochip_setup_dev()
  -> gcdev_register()
  -> gpiolib_cdev_register()

The SRCU won't be held as `gc` is ensured, and the lockdep check emits
the warning.  gpiochip_sysfs_register() shares the similar concern.

This is easily to reproduce as most cases should fall into the latter calling
path.  I overlooked the case because I always tested with revocable rework
series which eliminates the lockdep checks...

Given that both gpiolib_cdev_register() and gpiochip_sysfs_register() are
only called from gpiolib but no external users, maybe a simple fix is
removing the lockdep checks?

  gpiolib_cdev_register()
  -> (called from) gcdev_register()
    -> (called from) gpiochip_setup_dev()

  gpiochip_sysfs_register()
  -> (called from) gpiochip_setup_dev()
  or
  -> (called from) gpiofind_sysfs_register()
    -> (called from) gpiolib_sysfs_init()

Proposed a fix in:
https://lore.kernel.org/all/20260228131430.102388-1-tzungbi@kernel.org