From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
As per the API contract, the get_direction() callback can only
return 0, 1 or a negative error number. Add a wrapper around the callback
calls that filters out anything else.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
drivers/gpio/gpiolib.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 683a03d237c0..7f2aca9f81a1 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -341,6 +341,22 @@ static int gpiochip_find_base_unlocked(u16 ngpio)
}
}
+static int gpiochip_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+ int ret;
+
+ lockdep_assert_held(&gc->gpiodev->srcu);
+
+ if (WARN_ON(!gc->get_direction))
+ return -EOPNOTSUPP;
+
+ ret = gc->get_direction(gc, offset);
+ if (ret > 1)
+ ret = -EBADE;
+
+ return ret;
+}
+
/**
* gpiod_get_direction - return the current direction of a GPIO
* @desc: GPIO to get the direction of
@@ -381,7 +397,7 @@ int gpiod_get_direction(struct gpio_desc *desc)
if (!guard.gc->get_direction)
return -ENOTSUPP;
- ret = guard.gc->get_direction(guard.gc, offset);
+ ret = gpiochip_get_direction(guard.gc, offset);
if (ret < 0)
return ret;
@@ -1057,7 +1073,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
desc->gdev = gdev;
if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) {
- ret = gc->get_direction(gc, desc_index);
+ ret = gpiochip_get_direction(gc, desc_index);
if (ret < 0)
goto err_cleanup_desc_srcu;
@@ -2770,8 +2786,7 @@ int gpiod_direction_input_nonotify(struct gpio_desc *desc)
ret = gpiochip_direction_input(guard.gc,
gpio_chip_hwgpio(desc));
} else if (guard.gc->get_direction) {
- ret = guard.gc->get_direction(guard.gc,
- gpio_chip_hwgpio(desc));
+ ret = gpiochip_get_direction(guard.gc, gpio_chip_hwgpio(desc));
if (ret < 0)
return ret;
@@ -2818,8 +2833,8 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
} else {
/* Check that we are in output mode if we can */
if (guard.gc->get_direction) {
- ret = guard.gc->get_direction(guard.gc,
- gpio_chip_hwgpio(desc));
+ ret = gpiochip_get_direction(guard.gc,
+ gpio_chip_hwgpio(desc));
if (ret < 0)
return ret;
--
2.45.2On 10.02.2025 11:52, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> As per the API contract, the get_direction() callback can only
> return 0, 1 or a negative error number. Add a wrapper around the callback
> calls that filters out anything else.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
This patch landed in today's linux-next as commit e623c4303ed11
("gpiolib: sanitize the return value of gpio_chip::get_direction()"). It
introduced a lockdep warning from the gpiochip_get_direction() function.
IMHO it looks that gpiochip_add_data_with_key() lacks proper srcu
locking/annotation for the newly created gpio chip. Here is the log:
gpio gpiochip1: Static allocation of GPIO base is deprecated, use
dynamic allocation.
------------[ cut here ]------------
WARNING: CPU: 2 PID: 35 at drivers/gpio/gpiolib.c:349
gpiochip_get_direction+0x48/0x66
Modules linked in: cdns_usb_common roles cdns3_starfive
snd_soc_simple_card snd_soc_simple_card_utils phy_jh7110_dphy_rx
clk_starfive_jh7110_vout pcie_starfive(+) clk_starfive_jh7110_isp
jh7110_trng sfctemp dwmac_starfive stmmac_platform
spi_cadence_quadspi(+) clk_starfive_jh7110_stg stmmac
clk_starfive_jh7110_aon jh7110_pwmdac pcs_xpcs phy_jh7110_usb spi_pl022
phy_jh7110_pcie snd_soc_spdif_tx i2c_dev drm
drm_panel_orientation_quirks backlight dm_mod configfs ip_tables x_tables
CPU: 2 UID: 0 PID: 35 Comm: kworker/u18:0 Tainted: G W
6.14.0-rc4-next-20250225 #1054
Tainted: [W]=WARN
Hardware name: StarFive VisionFive 2 v1.2A (DT)
Workqueue: events_unbound deferred_probe_work_func
epc : gpiochip_get_direction+0x48/0x66
ra : gpiochip_get_direction+0x46/0x66
...
[<ffffffff805fc72c>] gpiochip_get_direction+0x48/0x66
[<ffffffff80603a14>] gpiochip_add_data_with_key+0x74a/0xde2
[<ffffffff806044e6>] devm_gpiochip_add_data_with_key+0x1e/0x5a
[<ffffffff805f8738>] jh7110_pinctrl_probe+0x298/0x3aa
[<ffffffff80731116>] platform_probe+0x4e/0x92
[<ffffffff8000c366>] really_probe+0x10a/0x2de
[<ffffffff8000c5e4>] __driver_probe_device.part.0+0xaa/0xe0
[<ffffffff8072ee34>] driver_probe_device+0x78/0xc4
[<ffffffff8072eee6>] __device_attach_driver+0x66/0xc6
[<ffffffff8072d0b0>] bus_for_each_drv+0x5c/0xb0
[<ffffffff8072f33e>] __device_attach+0x84/0x13c
[<ffffffff8072f55e>] device_initial_probe+0xe/0x16
[<ffffffff8072e002>] bus_probe_device+0x88/0x8a
[<ffffffff8072e516>] deferred_probe_work_func+0xd4/0xee
[<ffffffff80047b7e>] process_one_work+0x1d0/0x57a
[<ffffffff8004854e>] worker_thread+0x166/0x2cc
[<ffffffff80051568>] kthread+0xdc/0x1b4
[<ffffffff80bcb942>] ret_from_fork+0xe/0x18
irq event stamp: 17857
hardirqs last enabled at (17857): [<ffffffff80bca986>]
_raw_spin_unlock_irqrestore+0x4c/0x4e
hardirqs last disabled at (17856): [<ffffffff80bca73c>]
_raw_spin_lock_irqsave+0x5e/0x64
softirqs last enabled at (17322): [<ffffffff80adff1a>]
inet6_fill_ifla6_attrs+0x3d0/0x420
softirqs last disabled at (17320): [<ffffffff80adfefe>]
inet6_fill_ifla6_attrs+0x3b4/0x420
---[ end trace 0000000000000000 ]---
> ---
> drivers/gpio/gpiolib.c | 27 +++++++++++++++++++++------
> 1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 683a03d237c0..7f2aca9f81a1 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -341,6 +341,22 @@ static int gpiochip_find_base_unlocked(u16 ngpio)
> }
> }
>
> +static int gpiochip_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> + int ret;
> +
> + lockdep_assert_held(&gc->gpiodev->srcu);
> +
> + if (WARN_ON(!gc->get_direction))
> + return -EOPNOTSUPP;
> +
> + ret = gc->get_direction(gc, offset);
> + if (ret > 1)
> + ret = -EBADE;
> +
> + return ret;
> +}
> +
> /**
> * gpiod_get_direction - return the current direction of a GPIO
> * @desc: GPIO to get the direction of
> @@ -381,7 +397,7 @@ int gpiod_get_direction(struct gpio_desc *desc)
> if (!guard.gc->get_direction)
> return -ENOTSUPP;
>
> - ret = guard.gc->get_direction(guard.gc, offset);
> + ret = gpiochip_get_direction(guard.gc, offset);
> if (ret < 0)
> return ret;
>
> @@ -1057,7 +1073,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> desc->gdev = gdev;
>
> if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) {
> - ret = gc->get_direction(gc, desc_index);
> + ret = gpiochip_get_direction(gc, desc_index);
> if (ret < 0)
> goto err_cleanup_desc_srcu;
>
> @@ -2770,8 +2786,7 @@ int gpiod_direction_input_nonotify(struct gpio_desc *desc)
> ret = gpiochip_direction_input(guard.gc,
> gpio_chip_hwgpio(desc));
> } else if (guard.gc->get_direction) {
> - ret = guard.gc->get_direction(guard.gc,
> - gpio_chip_hwgpio(desc));
> + ret = gpiochip_get_direction(guard.gc, gpio_chip_hwgpio(desc));
> if (ret < 0)
> return ret;
>
> @@ -2818,8 +2833,8 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
> } else {
> /* Check that we are in output mode if we can */
> if (guard.gc->get_direction) {
> - ret = guard.gc->get_direction(guard.gc,
> - gpio_chip_hwgpio(desc));
> + ret = gpiochip_get_direction(guard.gc,
> + gpio_chip_hwgpio(desc));
> if (ret < 0)
> return ret;
>
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On Tue, Feb 25, 2025 at 11:13 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 10.02.2025 11:52, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > As per the API contract, the get_direction() callback can only
> > return 0, 1 or a negative error number. Add a wrapper around the callback
> > calls that filters out anything else.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> This patch landed in today's linux-next as commit e623c4303ed11
> ("gpiolib: sanitize the return value of gpio_chip::get_direction()"). It
> introduced a lockdep warning from the gpiochip_get_direction() function.
> IMHO it looks that gpiochip_add_data_with_key() lacks proper srcu
> locking/annotation for the newly created gpio chip. Here is the log:
>
> gpio gpiochip1: Static allocation of GPIO base is deprecated, use
> dynamic allocation.
> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 35 at drivers/gpio/gpiolib.c:349
> gpiochip_get_direction+0x48/0x66
> Modules linked in: cdns_usb_common roles cdns3_starfive
> snd_soc_simple_card snd_soc_simple_card_utils phy_jh7110_dphy_rx
> clk_starfive_jh7110_vout pcie_starfive(+) clk_starfive_jh7110_isp
> jh7110_trng sfctemp dwmac_starfive stmmac_platform
> spi_cadence_quadspi(+) clk_starfive_jh7110_stg stmmac
> clk_starfive_jh7110_aon jh7110_pwmdac pcs_xpcs phy_jh7110_usb spi_pl022
> phy_jh7110_pcie snd_soc_spdif_tx i2c_dev drm
> drm_panel_orientation_quirks backlight dm_mod configfs ip_tables x_tables
> CPU: 2 UID: 0 PID: 35 Comm: kworker/u18:0 Tainted: G W
> 6.14.0-rc4-next-20250225 #1054
> Tainted: [W]=WARN
> Hardware name: StarFive VisionFive 2 v1.2A (DT)
> Workqueue: events_unbound deferred_probe_work_func
> epc : gpiochip_get_direction+0x48/0x66
> ra : gpiochip_get_direction+0x46/0x66
> ...
> [<ffffffff805fc72c>] gpiochip_get_direction+0x48/0x66
> [<ffffffff80603a14>] gpiochip_add_data_with_key+0x74a/0xde2
> [<ffffffff806044e6>] devm_gpiochip_add_data_with_key+0x1e/0x5a
> [<ffffffff805f8738>] jh7110_pinctrl_probe+0x298/0x3aa
> [<ffffffff80731116>] platform_probe+0x4e/0x92
> [<ffffffff8000c366>] really_probe+0x10a/0x2de
> [<ffffffff8000c5e4>] __driver_probe_device.part.0+0xaa/0xe0
> [<ffffffff8072ee34>] driver_probe_device+0x78/0xc4
> [<ffffffff8072eee6>] __device_attach_driver+0x66/0xc6
> [<ffffffff8072d0b0>] bus_for_each_drv+0x5c/0xb0
> [<ffffffff8072f33e>] __device_attach+0x84/0x13c
> [<ffffffff8072f55e>] device_initial_probe+0xe/0x16
> [<ffffffff8072e002>] bus_probe_device+0x88/0x8a
> [<ffffffff8072e516>] deferred_probe_work_func+0xd4/0xee
> [<ffffffff80047b7e>] process_one_work+0x1d0/0x57a
> [<ffffffff8004854e>] worker_thread+0x166/0x2cc
> [<ffffffff80051568>] kthread+0xdc/0x1b4
> [<ffffffff80bcb942>] ret_from_fork+0xe/0x18
> irq event stamp: 17857
> hardirqs last enabled at (17857): [<ffffffff80bca986>]
> _raw_spin_unlock_irqrestore+0x4c/0x4e
> hardirqs last disabled at (17856): [<ffffffff80bca73c>]
> _raw_spin_lock_irqsave+0x5e/0x64
> softirqs last enabled at (17322): [<ffffffff80adff1a>]
> inet6_fill_ifla6_attrs+0x3d0/0x420
> softirqs last disabled at (17320): [<ffffffff80adfefe>]
> inet6_fill_ifla6_attrs+0x3b4/0x420
> ---[ end trace 0000000000000000 ]---
>
Thanks for the report. We don't need to hold the SRCU when registering
the chip. I'm now thinking I should revert using
gpiochip_get_direction() in gpiochip_add_data_with_key() to directly
calling the get_direction() callback and not checking its return
value.
Bartosz
On Mon, Feb 10, 2025 at 11:52:02AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> As per the API contract, the get_direction() callback can only
> return 0, 1 or a negative error number. Add a wrapper around the callback
> calls that filters out anything else.
...
> +static int gpiochip_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> + int ret;
> +
> + lockdep_assert_held(&gc->gpiodev->srcu);
> +
> + if (WARN_ON(!gc->get_direction))
> + return -EOPNOTSUPP;
> +
> + ret = gc->get_direction(gc, offset);
> + if (ret > 1)
Would it be better to use the respective GPIO*... macro instead of 1?
> + ret = -EBADE;
> +
> + return ret;
> +}
--
With Best Regards,
Andy Shevchenko
On Mon, Feb 24, 2025 at 5:33 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, Feb 10, 2025 at 11:52:02AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > As per the API contract, the get_direction() callback can only
> > return 0, 1 or a negative error number. Add a wrapper around the callback
> > calls that filters out anything else.
>
> ...
>
> > +static int gpiochip_get_direction(struct gpio_chip *gc, unsigned int offset)
> > +{
> > + int ret;
> > +
> > + lockdep_assert_held(&gc->gpiodev->srcu);
> > +
> > + if (WARN_ON(!gc->get_direction))
> > + return -EOPNOTSUPP;
> > +
> > + ret = gc->get_direction(gc, offset);
> > + if (ret > 1)
>
> Would it be better to use the respective GPIO*... macro instead of 1?
>
I did consider it but I don't like comparing against enums, it doesn't
feel right as the value behind the name can change. I think I prefer
it like this even if it's not the best solution either. Maybe we could
be more explicit and say:
if (!(ret == IN || ret == OUT || ret < 0)
?
Bart
Mon, Feb 24, 2025 at 08:55:26PM +0100, Bartosz Golaszewski kirjoitti:
> On Mon, Feb 24, 2025 at 5:33 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Mon, Feb 10, 2025 at 11:52:02AM +0100, Bartosz Golaszewski wrote:
...
> > > +static int gpiochip_get_direction(struct gpio_chip *gc, unsigned int offset)
> > > +{
> > > + int ret;
> > > +
> > > + lockdep_assert_held(&gc->gpiodev->srcu);
> > > +
> > > + if (WARN_ON(!gc->get_direction))
> > > + return -EOPNOTSUPP;
> > > +
> > > + ret = gc->get_direction(gc, offset);
> > > + if (ret > 1)
> >
> > Would it be better to use the respective GPIO*... macro instead of 1?
> >
>
> I did consider it but I don't like comparing against enums, it doesn't
> feel right as the value behind the name can change. I think I prefer
> it like this even if it's not the best solution either. Maybe we could
> be more explicit and say:
>
> if (!(ret == IN || ret == OUT || ret < 0)
>
> ?
Yep, I like this.
--
With Best Regards,
Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.