Re: [PATCH 8/8] gpiolib: sanitize the return value of gpio_chip::get_direction()

[Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>]

On 10.02.2025 11:52, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>

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