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

[Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>]

Hi Bartosz,

On 10.02.2025 11:51, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> As per the API contract - gpio_chip::get_direction() may fail and return
> a negative error number. However, we treat it as if it always returned 0
> or 1. Check the return value of the callback and propagate the error
> number up the stack.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> ---
>   drivers/gpio/gpiolib.c | 44 +++++++++++++++++++++++++++++---------------
>   1 file changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 679ed764cb14..5d3774dc748b 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1057,8 +1057,11 @@ 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)) {
> -			assign_bit(FLAG_IS_OUT,
> -				   &desc->flags, !gc->get_direction(gc, desc_index));
> +			ret = gc->get_direction(gc, desc_index);
> +			if (ret < 0)
> +				goto err_cleanup_desc_srcu;
> +
> +			assign_bit(FLAG_IS_OUT, &desc->flags, !ret);
>   		} else {
>   			assign_bit(FLAG_IS_OUT,
>   				   &desc->flags, !gc->direction_input);

This change breaks bcm2835 pincontrol/gpio driver (and probably others) 
in next-20250218. The problem is that some gpio lines are initially 
configured as alternate function (i.e. uart) and .get_direction returns 
-EINVAL for them, what in turn causes the whole gpio chip fail to 
register. Here is the log with WARN_ON() added to line 
drivers/pinctrl/bcm/pinctrl-bcm2835.c:350 from Raspberry Pi 4B:

  ------------[ cut here ]------------
  WARNING: CPU: 0 PID: 1 at drivers/pinctrl/bcm/pinctrl-bcm2835.c:350 
bcm2835_gpio_get_direction+0x80/0x98
  Modules linked in:
  CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 
6.14.0-rc3-next-20250218-dirty #9817
  Hardware name: Raspberry Pi 4 Model B (DT)
  pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
  pc : bcm2835_gpio_get_direction+0x80/0x98
  lr : bcm2835_gpio_get_direction+0x18/0x98
  ...
  Call trace:
   bcm2835_gpio_get_direction+0x80/0x98 (P)
   gpiochip_add_data_with_key+0x874/0xef0
   bcm2835_pinctrl_probe+0x354/0x53c
   platform_probe+0x68/0xdc
   really_probe+0xbc/0x298
   __driver_probe_device+0x78/0x12c
   driver_probe_device+0xdc/0x164
   __driver_attach+0x9c/0x1ac
   bus_for_each_dev+0x74/0xd4
   driver_attach+0x24/0x30
   bus_add_driver+0xe4/0x208
   driver_register+0x60/0x128
   __platform_driver_register+0x24/0x30
   bcm2835_pinctrl_driver_init+0x20/0x2c
   do_one_initcall+0x64/0x308
   kernel_init_freeable+0x280/0x4e8
   kernel_init+0x20/0x1d8
   ret_from_fork+0x10/0x20
  irq event stamp: 100380
  hardirqs last  enabled at (100379): [<ffff8000812d7d5c>] 
_raw_spin_unlock_irqrestore+0x74/0x78
  hardirqs last disabled at (100380): [<ffff8000812c8918>] el1_dbg+0x24/0x8c
  softirqs last  enabled at (93674): [<ffff80008005ed4c>] 
handle_softirqs+0x4c4/0x4dc
  softirqs last disabled at (93669): [<ffff8000800105a0>] 
__do_softirq+0x14/0x20
  ---[ end trace 0000000000000000 ]---
  gpiochip_add_data_with_key: GPIOs 512..569 (pinctrl-bcm2711) failed to 
register, -22
  pinctrl-bcm2835 fe200000.gpio: could not add GPIO chip
  pinctrl-bcm2835 fe200000.gpio: probe with driver pinctrl-bcm2835 
failed with error -22


Any suggestions how to fix this issue? Should we add 
GPIO_LINE_DIRECTION_UNKNOWN?


> @@ -2728,13 +2731,18 @@ int gpiod_direction_input_nonotify(struct gpio_desc *desc)
>   	if (guard.gc->direction_input) {
>   		ret = guard.gc->direction_input(guard.gc,
>   						gpio_chip_hwgpio(desc));
> -	} else if (guard.gc->get_direction &&
> -		  (guard.gc->get_direction(guard.gc,
> -					   gpio_chip_hwgpio(desc)) != 1)) {
> -		gpiod_warn(desc,
> -			   "%s: missing direction_input() operation and line is output\n",
> -			   __func__);
> -		return -EIO;
> +	} else if (guard.gc->get_direction) {
> +		ret = guard.gc->get_direction(guard.gc,
> +					      gpio_chip_hwgpio(desc));
> +		if (ret < 0)
> +			return ret;
> +
> +		if (ret != GPIO_LINE_DIRECTION_IN) {
> +			gpiod_warn(desc,
> +				   "%s: missing direction_input() operation and line is output\n",
> +				    __func__);
> +			return -EIO;
> +		}
>   	}
>   	if (ret == 0) {
>   		clear_bit(FLAG_IS_OUT, &desc->flags);
> @@ -2771,12 +2779,18 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
>   						 gpio_chip_hwgpio(desc), val);
>   	} else {
>   		/* Check that we are in output mode if we can */
> -		if (guard.gc->get_direction &&
> -		    guard.gc->get_direction(guard.gc, gpio_chip_hwgpio(desc))) {
> -			gpiod_warn(desc,
> -				"%s: missing direction_output() operation\n",
> -				__func__);
> -			return -EIO;
> +		if (guard.gc->get_direction) {
> +			ret = guard.gc->get_direction(guard.gc,
> +						      gpio_chip_hwgpio(desc));
> +			if (ret < 0)
> +				return ret;
> +
> +			if (ret != GPIO_LINE_DIRECTION_OUT) {
> +				gpiod_warn(desc,
> +					   "%s: missing direction_output() operation\n",
> +					   __func__);
> +				return -EIO;
> +			}
>   		}
>   		/*
>   		 * If we can't actively set the direction, we are some
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland