From: Marek Szyprowski <m.szyprowski@samsung.com>
To: Tzung-Bi Shih <tzungbi@kernel.org>,
Bartosz Golaszewski <brgl@kernel.org>,
Linus Walleij <linusw@kernel.org>
Cc: Shuah Khan <shuah@kernel.org>,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org,
linux-kselftest@vger.kernel.org
Subject: Re: [PATCH v4 4/6] gpio: Ensure struct gpio_chip for gpiochip_setup_dev()
Date: Fri, 27 Feb 2026 22:36:12 +0100 [thread overview]
Message-ID: <506ce9b3-d492-4fce-9d02-330e411911e2@samsung.com> (raw)
In-Reply-To: <20260223061726.82161-5-tzungbi@kernel.org>
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
samsung_pinctrl_probe from platform_probe+0x5c/0x98
platform_probe from really_probe+0xe0/0x424
really_probe from __driver_probe_device+0x9c/0x1f4
__driver_probe_device from driver_probe_device+0x30/0xc0
driver_probe_device from __device_attach_driver+0xbc/0x180
__device_attach_driver from bus_for_each_drv+0x84/0xdc
bus_for_each_drv from __device_attach+0xb0/0x214
__device_attach from device_initial_probe+0x3c/0x48
device_initial_probe from bus_probe_device+0x24/0x80
bus_probe_device from device_add+0x5c0/0x810
device_add from of_platform_device_create_pdata+0xac/0x104
of_platform_device_create_pdata from of_platform_bus_create+0x210/0x534
of_platform_bus_create from of_platform_bus_create+0x27c/0x534
of_platform_bus_create from of_platform_populate+0x90/0x150
of_platform_populate from of_platform_default_populate_init+0xd0/0xe0
of_platform_default_populate_init from do_one_initcall+0x70/0x3bc
do_one_initcall from kernel_init_freeable+0x1c0/0x248
kernel_init_freeable from kernel_init+0x18/0x12c
kernel_init from ret_from_fork+0x14/0x28
Exception stack(0xf082dfb0 to 0xf082dff8)
dfa0: 00000000 00000000 00000000
00000000
dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
irq event stamp: 55167
hardirqs last enabled at (55387): [<c01c3578>] __up_console_sem+0x50/0x60
hardirqs last disabled at (55398): [<c01c3564>] __up_console_sem+0x3c/0x60
softirqs last enabled at (55352): [<c013c8fc>] handle_softirqs+0x32c/0x5c0
softirqs last disabled at (55419): [<c013cd3c>] __irq_exit_rcu+0x144/0x1f0
---[ end trace 0000000000000000 ]---
> ---
> v4:
> - To be consistent, rename `chip` -> `gc`.
> - Add lockdep checks.
>
> v3: https://lore.kernel.org/all/20260213092958.864411-5-tzungbi@kernel.org
> - Pass struct gpio_chip * only.
>
> v2: https://lore.kernel.org/all/20260203061059.975605-5-tzungbi@kernel.org
> - No changes.
>
> v1: https://lore.kernel.org/all/20260116081036.352286-10-tzungbi@kernel.org
>
> drivers/gpio/gpiolib-cdev.c | 14 ++++----------
> drivers/gpio/gpiolib-cdev.h | 2 +-
> drivers/gpio/gpiolib-sysfs.c | 21 ++++++++-------------
> drivers/gpio/gpiolib-sysfs.h | 4 ++--
> drivers/gpio/gpiolib.c | 24 +++++++++++++++++-------
> 5 files changed, 32 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 73ae77f0f213..a154b04e9316 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -2782,11 +2782,13 @@ static const struct file_operations gpio_fileops = {
> #endif
> };
>
> -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-cdev.h b/drivers/gpio/gpiolib-cdev.h
> index b42644cbffb8..4a9cb3335d99 100644
> --- a/drivers/gpio/gpiolib-cdev.h
> +++ b/drivers/gpio/gpiolib-cdev.h
> @@ -7,7 +7,7 @@
>
> struct gpio_device;
>
> -int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt);
> +int gpiolib_cdev_register(struct gpio_chip *gc, dev_t devt);
> void gpiolib_cdev_unregister(struct gpio_device *gdev);
>
> #endif /* GPIOLIB_CDEV_H */
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index 1c25a7dd3db4..748a3eb1bf35 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -983,13 +983,15 @@ void gpiod_unexport(struct gpio_desc *desc)
> }
> EXPORT_SYMBOL_GPL(gpiod_unexport);
>
> -int gpiochip_sysfs_register(struct gpio_device *gdev)
> +int gpiochip_sysfs_register(struct gpio_chip *gc)
> {
> + struct gpio_device *gdev = gc->gpiodev;
> struct gpiodev_data *data;
> - struct gpio_chip *chip;
> struct device *parent;
> int err;
>
> + lockdep_assert_held(&gdev->srcu);
> +
> /*
> * Many systems add gpio chips for SOC support very early,
> * before driver model support is available. In those cases we
> @@ -999,18 +1001,12 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
> if (!class_is_registered(&gpio_class))
> return 0;
>
> - guard(srcu)(&gdev->srcu);
> -
> - chip = srcu_dereference(gdev->chip, &gdev->srcu);
> - if (!chip)
> - return -ENODEV;
> -
> /*
> * For sysfs backward compatibility we need to preserve this
> * preferred parenting to the gpio_chip parent field, if set.
> */
> - if (chip->parent)
> - parent = chip->parent;
> + if (gc->parent)
> + parent = gc->parent;
> else
> parent = &gdev->dev;
>
> @@ -1029,7 +1025,7 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
> MKDEV(0, 0), data,
> gpiochip_groups,
> GPIOCHIP_NAME "%d",
> - chip->base);
> + gc->base);
> if (IS_ERR(data->cdev_base)) {
> err = PTR_ERR(data->cdev_base);
> kfree(data);
> @@ -1085,10 +1081,9 @@ void gpiochip_sysfs_unregister(struct gpio_chip *gc)
> */
> static int gpiofind_sysfs_register(struct gpio_chip *gc, const void *data)
> {
> - struct gpio_device *gdev = gc->gpiodev;
> int ret;
>
> - ret = gpiochip_sysfs_register(gdev);
> + ret = gpiochip_sysfs_register(gc);
> if (ret)
> gpiochip_err(gc, "failed to register the sysfs entry: %d\n", ret);
>
> diff --git a/drivers/gpio/gpiolib-sysfs.h b/drivers/gpio/gpiolib-sysfs.h
> index fd5db5384681..d0998de043a2 100644
> --- a/drivers/gpio/gpiolib-sysfs.h
> +++ b/drivers/gpio/gpiolib-sysfs.h
> @@ -7,12 +7,12 @@ struct gpio_device;
>
> #ifdef CONFIG_GPIO_SYSFS
>
> -int gpiochip_sysfs_register(struct gpio_device *gdev);
> +int gpiochip_sysfs_register(struct gpio_chip *gc);
> void gpiochip_sysfs_unregister(struct gpio_chip *gc);
>
> #else
>
> -static inline int gpiochip_sysfs_register(struct gpio_device *gdev)
> +static inline int gpiochip_sysfs_register(struct gpio_chip *gc)
> {
> 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))
> #define gcdev_unregister(gdev) gpiolib_cdev_unregister((gdev))
> #else
> /*
> * gpiolib_cdev_register() indirectly calls device_add(), which is still
> * required even when cdev is not selected.
> */
> -#define gcdev_register(gdev, devt) device_add(&(gdev)->dev)
> +#define gcdev_register(gc, devt) device_add(&(gc)->gpiodev->dev)
> #define gcdev_unregister(gdev) device_del(&(gdev)->dev)
> #endif
>
> @@ -896,8 +896,9 @@ static const struct device_type gpio_dev_type = {
> * An initial reference count has been held in gpiochip_add_data_with_key().
> * The caller should drop the reference via gpio_device_put() on errors.
> */
> -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;
> }
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
next prev parent reply other threads:[~2026-02-27 21:36 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-02-23 6:17 [PATCH v4 0/6] gpio: Refactor and add selftest Tzung-Bi Shih
2026-02-23 6:17 ` [PATCH v4 1/6] gpio: Access `gpio_bus_type` in gpiochip_setup_dev() Tzung-Bi Shih
2026-03-11 11:44 ` Geert Uytterhoeven
2026-03-11 14:36 ` Bartosz Golaszewski
2026-03-12 4:52 ` Tzung-Bi Shih
2026-02-23 6:17 ` [PATCH v4 2/6] gpio: Remove redundant check for struct gpio_chip Tzung-Bi Shih
2026-02-23 6:17 ` [PATCH v4 3/6] gpio: sysfs: " Tzung-Bi Shih
2026-02-23 6:17 ` [PATCH v4 4/6] gpio: Ensure struct gpio_chip for gpiochip_setup_dev() Tzung-Bi Shih
[not found] ` <CGME20260227213613eucas1p2f2754ab3867fc422f44790eb34c79182@eucas1p2.samsung.com>
2026-02-27 21:36 ` Marek Szyprowski [this message]
2026-02-28 10:03 ` Bartosz Golaszewski
2026-02-28 13:20 ` Tzung-Bi Shih
2026-02-23 6:17 ` [PATCH v4 5/6] gpio: cdev: Don't check struct gpio_chip in gpio_chrdev_open() Tzung-Bi Shih
2026-02-23 6:17 ` [PATCH v4 6/6] selftests: gpio: Add gpio-cdev-uaf tests Tzung-Bi Shih
2026-02-25 10:26 ` [PATCH v4 0/6] gpio: Refactor and add selftest Bartosz Golaszewski
2026-02-26 12:44 ` Tzung-Bi Shih
2026-02-27 9:10 ` Bartosz Golaszewski
2026-02-27 9:08 ` Bartosz Golaszewski
2026-02-27 9:22 ` Linus Walleij
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=506ce9b3-d492-4fce-9d02-330e411911e2@samsung.com \
--to=m.szyprowski@samsung.com \
--cc=brgl@kernel.org \
--cc=linusw@kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=shuah@kernel.org \
--cc=tzungbi@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®