REGRESSION: [PATCH v5 3/3] gpiolib: Show correct direction from the beginning

From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
To: "linus.walleij@linaro.org" <linus.walleij@linaro.org>,
	"timur@kernel.org" <timur@kernel.org>,
	"swboyd@chromium.org" <swboyd@chromium.org>,
	"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
	"jhugo@codeaurora.org" <jhugo@codeaurora.org>,
	"ricardo.ribalda@gmail.com" <ricardo.ribalda@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: REGRESSION: [PATCH v5 3/3] gpiolib: Show correct direction from the beginning
Date: Fri, 12 Oct 2018 09:00:41 +0000	[thread overview]
Message-ID: <1539334839.30485.9.camel@toradex.com> (raw)
In-Reply-To: <20181005065300.22882-3-ricardo.ribalda@gmail.com>

On Fri, 2018-10-05 at 08:53 +0200, Ricardo Ribalda Delgado wrote:
> Current code assumes that the direction is input if direction_input
> function is set.
> This might not be the case on GPIOs with programmable direction.

Unfortunately, this breaks at least Apalis T30 and Apalis TK1. Enabling
earlycon reveals the following:

[    0.721165] Unable to handle kernel NULL pointer dereference at
virtual addre
ss 000001f8
[    0.729570] pgd = (ptrval)
[    0.732417] [000001f8] *pgd=00000000
[    0.736137] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
[    0.741643] Modules linked in:
[    0.744819] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.19.0-rc7-
next-2018101
2 #6
[    0.752579] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[    0.759092] PC is at gpiod_hog+0x2c/0x150
[    0.763255] LR is at of_gpiochip_add+0x34c/0x510
[    0.768040] pc : [<c044c9a4>]    lr : [<c044e840>]    psr: 60000013
[    0.774534] sp : f68c9cd0  ip : 00000000  fp : f68c9d18
[    0.779946] r10: c0ccb3c8  r9 : 00000000  r8 : 00000000
[    0.785359] r7 : 00000007  r6 : c20019c4  r5 : f6a7b970  r4 :
f6a78a24
[    0.792121] r3 : 00000000  r2 : 00000000  r1 : c20019c4  r0 :
f6a7b970
[    0.798884] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA
ARM  Segment none
[    0.806273] Control: 10c5387d  Table: 8000404a  DAC: 00000051
[    0.812227] Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
[    0.818451] Stack: (0xf68c9cd0 to 0xf68ca000)
...
[    1.043490] [<c044c9a4>] (gpiod_hog) from [<c044e840>]
(of_gpiochip_add+0x34c/0x510)
[    1.051531] [<c044e840>] (of_gpiochip_add) from [<c044d1cc>]
(gpiochip_add_data_with_key+0x668/0x958)
[    1.061091] [<c044d1cc>] (gpiochip_add_data_with_key) from
[<c044d504>] (devm_gpiochip_add_data+0x48/0x84)
[    1.071109] [<c044d504>] (devm_gpiochip_add_data) from [<c045166c>]
(tegra_gpio_probe+0x2d4/0x420)
[    1.080413] [<c045166c>] (tegra_gpio_probe) from [<c0574040>]
(platform_drv_probe+0x48/0x98)
[    1.089171] [<c0574040>] (platform_drv_probe) from [<c0572164>]
(really_probe+0x1e0/0x2cc)
[    1.097746] [<c0572164>] (really_probe) from [<c05723b4>]
(driver_probe_device+0x60/0x16c)
[    1.106317] [<c05723b4>] (driver_probe_device) from [<c057259c>]
(__driver_attach+0xdc/0xe0)
[    1.115071] [<c057259c>] (__driver_attach) from [<c05704a8>]
(bus_for_each_dev+0x74/0xb4)
[    1.123554] [<c05704a8>] (bus_for_each_dev) from [<c0571644>]
(bus_add_driver+0x1c0/0x204)
[    1.132122] [<c0571644>] (bus_add_driver) from [<c05731b8>]
(driver_register+0x74/0x108)
[    1.140521] [<c05731b8>] (driver_register) from [<c0102ebc>]
(do_one_initcall+0x54/0x284)
[    1.149015] [<c0102ebc>] (do_one_initcall) from [<c0e01134>]
(kernel_init_freeable+0x2d0/0x364)
[    1.158043] [<c0e01134>] (kernel_init_freeable) from [<c0a24c78>]
(kernel_init+0x8/0x110)
[    1.166527] [<c0a24c78>] (kernel_init) from [<c01010e8>]
(ret_from_fork+0x14/0x2c)
[    1.174375] Exception stack(0xf68c9fb0 to 0xf68c9ff8)
...

Just reverting this one patch made it boot again. I will investigate
further...

> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/gpio/gpiolib.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 907019b67a58..e016b22658ff 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1349,20 +1349,6 @@ int gpiochip_add_data_with_key(struct
> gpio_chip *chip, void *data,
>  
>  	spin_unlock_irqrestore(&gpio_lock, flags);
>  
> -	for (i = 0; i < chip->ngpio; i++) {
> -		struct gpio_desc *desc = &gdev->descs[i];
> -
> -		desc->gdev = gdev;
> -
> -		/* REVISIT: most hardware initializes GPIOs as
> inputs (often
> -		 * with pullups enabled) so power usage is
> minimized. Linux
> -		 * code should set the gpio direction first thing;
> but until
> -		 * it does, and in case chip->get_direction is not
> set, we may
> -		 * expose the wrong direction in sysfs.
> -		 */
> -		desc->flags = !chip->direction_input ? (1 <<
> FLAG_IS_OUT) : 0;
> -	}
> -
>  #ifdef CONFIG_PINCTRL
>  	INIT_LIST_HEAD(&gdev->pin_ranges);
>  #endif
> @@ -1391,6 +1377,19 @@ int gpiochip_add_data_with_key(struct
> gpio_chip *chip, void *data,
>  	if (status)
>  		goto err_remove_chip;
>  
> +	for (i = 0; i < chip->ngpio; i++) {
> +		struct gpio_desc *desc = &gdev->descs[i];
> +
> +		desc->gdev = gdev;
> +
> +		if (chip->get_direction &&
> gpiochip_line_is_valid(chip, i))
> +			desc->flags = !chip->get_direction(chip, i)
> ?
> +					(1 << FLAG_IS_OUT) : 0;
> +		else
> +			desc->flags = !chip->direction_input ?
> +					(1 << FLAG_IS_OUT) : 0;
> +	}
> +
>  	acpi_gpiochip_add(chip);
>  
>  	machine_gpiochip_add(chip);