[PATCH RFC] gpiolib: ensure that fwnode is properly set

[PATCH RFC] gpiolib: ensure that fwnode is properly set

[Brian Masney]

Note that this is a RFC patch and not meant to be merged. I looked into
a problem with linux-next-20221110 on the Qualcomm SA8540P automotive
board (sc8280xp) where the UFS host controller would fail to probe due
to repeated probe deferrals when trying to get reset-gpios via
devm_gpiod_get_optional().

of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by
of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate function
pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The
pinctrl driver doesn't define one, so of_gpiochip_add() should
automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't
happen since the fwnode member on the struct gpiochip is set to null
when of_gpiochip_add() is called. Let's work around this by ensuring
that it's set if available.

Note that this broke sometime within the last few weeks within
linux-next and I haven't bisected this. I'm posting this in the hopes
that someone may know offhand which patch(es) may have broken this.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 11fb7ec883e9..8bec66008869 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	 * Assign fwnode depending on the result of the previous calls,
 	 * if none of them succeed, assign it to the parent's one.
 	 */
-	gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
+	gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
 
 	gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
 	if (gdev->id < 0) {
-- 
2.38.1

Re: [PATCH RFC] gpiolib: ensure that fwnode is properly set

[Robert Marko]

On 14. 11. 2022. 21:29, Brian Masney wrote:
> Note that this is a RFC patch and not meant to be merged. I looked into
> a problem with linux-next-20221110 on the Qualcomm SA8540P automotive
> board (sc8280xp) where the UFS host controller would fail to probe due
> to repeated probe deferrals when trying to get reset-gpios via
> devm_gpiod_get_optional().
>
> of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by
> of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate function
> pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The
> pinctrl driver doesn't define one, so of_gpiochip_add() should
> automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't
> happen since the fwnode member on the struct gpiochip is set to null
> when of_gpiochip_add() is called. Let's work around this by ensuring
> that it's set if available.
>
> Note that this broke sometime within the last few weeks within
> linux-next and I haven't bisected this. I'm posting this in the hopes
> that someone may know offhand which patch(es) may have broken this.

Hi, the following patch should fix it for you, I have hit the same issue on
IPQ8074.

https://patchwork.ozlabs.org/project/linux-gpio/patch/20221111113732.461881-1-thierry.reding@gmail.com/

Regards,
Robert

>
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>   drivers/gpio/gpiolib.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 11fb7ec883e9..8bec66008869 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>   	 * Assign fwnode depending on the result of the previous calls,
>   	 * if none of them succeed, assign it to the parent's one.
>   	 */
> -	gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
> +	gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
>   
>   	gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
>   	if (gdev->id < 0) {

Re: [PATCH RFC] gpiolib: ensure that fwnode is properly set

[Brian Masney]

On Mon, Nov 14, 2022 at 10:02:11PM +0100, Robert Marko wrote:
> Hi, the following patch should fix it for you, I have hit the same issue on
> IPQ8074.
> 
> https://patchwork.ozlabs.org/project/linux-gpio/patch/20221111113732.461881-1-thierry.reding@gmail.com/

That fixed the issue. Thanks, Robert.

Brian

Re: [PATCH RFC] gpiolib: ensure that fwnode is properly set

[Shazad Hussain]

On 11/15/2022 2:32 AM, Robert Marko wrote:
> 
> On 14. 11. 2022. 21:29, Brian Masney wrote:
>> Note that this is a RFC patch and not meant to be merged. I looked into
>> a problem with linux-next-20221110 on the Qualcomm SA8540P automotive
>> board (sc8280xp) where the UFS host controller would fail to probe due
>> to repeated probe deferrals when trying to get reset-gpios via
>> devm_gpiod_get_optional().
>>
>> of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by
>> of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate 
>> function
>> pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The
>> pinctrl driver doesn't define one, so of_gpiochip_add() should
>> automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't
>> happen since the fwnode member on the struct gpiochip is set to null
>> when of_gpiochip_add() is called. Let's work around this by ensuring
>> that it's set if available.
>>
>> Note that this broke sometime within the last few weeks within
>> linux-next and I haven't bisected this. I'm posting this in the hopes
>> that someone may know offhand which patch(es) may have broken this.
> 
> Hi, the following patch should fix it for you, I have hit the same issue on
> IPQ8074.
> 
> https://patchwork.ozlabs.org/project/linux-gpio/patch/20221111113732.461881-1-thierry.reding@gmail.com/
>
This fixes the issue I was facing for sa8540p (sc8280xp). Thanks Robert.

Shazad

> Regards,
> Robert
> 
>>
>> Signed-off-by: Brian Masney <bmasney@redhat.com>
>> ---
>>   drivers/gpio/gpiolib.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 11fb7ec883e9..8bec66008869 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip 
>> *gc, void *data,
>>        * Assign fwnode depending on the result of the previous calls,
>>        * if none of them succeed, assign it to the parent's one.
>>        */
>> -    gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
>> +    gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
>>       gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
>>       if (gdev->id < 0) {

Re: [PATCH RFC] gpiolib: ensure that fwnode is properly set

[Marijn Suijten]

On 2022-11-14 15:29:43, Brian Masney wrote:
> Note that this is a RFC patch and not meant to be merged. I looked into
> a problem with linux-next-20221110 on the Qualcomm SA8540P automotive
> board (sc8280xp) where the UFS host controller would fail to probe due
> to repeated probe deferrals when trying to get reset-gpios via
> devm_gpiod_get_optional().
> 
> of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by
> of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate function
> pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The
> pinctrl driver doesn't define one, so of_gpiochip_add() should
> automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't
> happen since the fwnode member on the struct gpiochip is set to null
> when of_gpiochip_add() is called. Let's work around this by ensuring
> that it's set if available.
> 
> Note that this broke sometime within the last few weeks within
> linux-next and I haven't bisected this. I'm posting this in the hopes
> that someone may know offhand which patch(es) may have broken this.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>

Thanks, this fixes the following abort I'm observing on multiple
Qualcomm platforms (sdm630, Sony Nile, and msm8956, Sony Loire):

    [    0.391439] Internal error: synchronous external abort: 0000000096000210 [#1] PREEMPT SMP
    [    0.391526] Modules linked in:
    [    0.398678] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 6.1.0-rc5-next-20221114-07647-gb3ed2836a8f7 #38
    [    0.401642] Hardware name: Sony Xperia XA2 Ultra (DT)
    [    0.410879] pstate: 00400005 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
    [    0.415957] pc : msm_gpio_get_direction+0x40/0x80
    [    0.422680] lr : msm_gpio_get_direction+0x18/0x80
    [    0.427544] sp : ffff80000805b750
    [    0.432270] x29: ffff80000805b750 x28: ffff739180b48c00 x27: ffff739180b50180
    [    0.435537] x26: 0000000000000008 x25: 0000000000000002 x24: ffffdcf976192e98
    [    0.442695] x23: ffff739180b31890 x22: 0000000000000000 x21: ffff739180b48c08
    [    0.449814] x20: ffffdcf975ff4148 x19: 0000000000000008 x18: 0000000000000000
    [    0.456891] x17: 64656c62616e655f x16: 7469647561206465 x15: 0000000000000002
    [    0.464049] x14: 0000000000000001 x13: 006c7274636e6970 x12: 2e30303030303133
    [    0.471166] x11: ffffdcf9760c98d8 x10: 000000000042c70 x9 : 0000000000000004
    [    0.478243] x8 : 0101010101010101 x7 : 0073656d616e2d65 x6 : 0911040aadece9ee
    [    0.485405] x5 : 6e696c2d0a041109 x4 : 0000000000000180 x3 : 0000000000008000
    [    0.492483] x2 : 0000000000000000 x1 : ffffdcf975628db0 x0 : ffff800008808000
    [    0.499642] Call trace:
    [    0.506703]  msm_gpio_get_direction+0x40/0x80
    [    0.509008]  gpiochip_add_data_with_key+0x638/0xed0
    [    0.513481]  msm_pinctrl_probe+0x430/0x580
    [    0.518168]  sdm660_pinctrl_probe+0x18/0x30
    [    0.522376]  platform_probe+0x68/0xc0
    [    0.526413]  really_probe+0xc0/0x3dc
    [    0.530234]  __driver_probe_device+0x7c/0x190
    [    0.533922]  driver_probe_device+0x3c/0x110
    [    0.538132]  __device_attach_driver+0xbc/0x160
    [    0.542168]  bus_for_each_drv+0x7c/0xd4
    [    0.546637]  __device_attach+0x9c/0x1c0
    [    0.550370]  device_initial_probe+0x14/0x20
    [    0.554231]  bus_probe_device+0x9c/0xa4
    [    0.558358]  device_add+0x3ac/0x8b0
    [    0.562175]  of_device_add+0x54/0x64
    [    0.565689]  of_platform_device_create_pdata+0x90/0x124
    [    0.569470]  of_platform_bus_create+0x18c/0x510
    [    0.574416]  of_platform_bus_create+0x1ec/0x510
    [    0.578971]  of_platform_populate+0x60/0x150
    [    0.583445]  of_platform_default_populate_init+0xe4/0x104
    [    0.588002]  do_one_initcall+0x64/0x1dc
    [    0.593253]  kernel_init_freeable+0x1b8/0x220
    [    0.596900]  kernel_init+0x24/0x130
    [    0.601453]  ret_from_fork+0x10/0x20
    [    0.604715] Code: d37d0442 8b020000 f941b400 8b030000 (b9400000)
    [    0.608583] ---[ end trace 0000000000000000 ]---
    [    0.619572] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

This wasn't an issue on -next 20221109.

Tested-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpio/gpiolib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 11fb7ec883e9..8bec66008869 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>  	 * Assign fwnode depending on the result of the previous calls,
>  	 * if none of them succeed, assign it to the parent's one.
>  	 */
> -	gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
> +	gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
>  
>  	gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
>  	if (gdev->id < 0) {
> -- 
> 2.38.1
> 

Re: [PATCH RFC] gpiolib: ensure that fwnode is properly set

[Konrad Dybcio]

On 14/11/2022 21:29, Brian Masney wrote:
> Note that this is a RFC patch and not meant to be merged. I looked into
> a problem with linux-next-20221110 on the Qualcomm SA8540P automotive
> board (sc8280xp) where the UFS host controller would fail to probe due
> to repeated probe deferrals when trying to get reset-gpios via
> devm_gpiod_get_optional().
>
> of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by
> of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate function
> pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The
> pinctrl driver doesn't define one, so of_gpiochip_add() should
> automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't
> happen since the fwnode member on the struct gpiochip is set to null
> when of_gpiochip_add() is called. Let's work around this by ensuring
> that it's set if available.
>
> Note that this broke sometime within the last few weeks within
> linux-next and I haven't bisected this. I'm posting this in the hopes
> that someone may know offhand which patch(es) may have broken this.
>
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---

Fixes boot on 8450 on next-20221115


Tested-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

>   drivers/gpio/gpiolib.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 11fb7ec883e9..8bec66008869 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>   	 * Assign fwnode depending on the result of the previous calls,
>   	 * if none of them succeed, assign it to the parent's one.
>   	 */
> -	gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
> +	gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
>   
>   	gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
>   	if (gdev->id < 0) {

Re: [PATCH RFC] gpiolib: ensure that fwnode is properly set

[Bryan O'Donoghue]

On 14/11/2022 20:29, Brian Masney wrote:
> Note that this is a RFC patch and not meant to be merged. I looked into
> a problem with linux-next-20221110 on the Qualcomm SA8540P automotive
> board (sc8280xp) where the UFS host controller would fail to probe due
> to repeated probe deferrals when trying to get reset-gpios via
> devm_gpiod_get_optional().
> 
> of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by
> of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate function
> pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The
> pinctrl driver doesn't define one, so of_gpiochip_add() should
> automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't
> happen since the fwnode member on the struct gpiochip is set to null
> when of_gpiochip_add() is called. Let's work around this by ensuring
> that it's set if available.
> 
> Note that this broke sometime within the last few weeks within
> linux-next and I haven't bisected this. I'm posting this in the hopes
> that someone may know offhand which patch(es) may have broken this.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>   drivers/gpio/gpiolib.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 11fb7ec883e9..8bec66008869 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>   	 * Assign fwnode depending on the result of the previous calls,
>   	 * if none of them succeed, assign it to the parent's one.
>   	 */
> -	gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
> +	gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
>   
>   	gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
>   	if (gdev->id < 0) {

Fixes repeated failure to get GPIO despite the controller having 
successfully probed.

[    6.151075] ov9282 21-0060: failed to get reset gpio -517
[    6.160599] ov9282 21-0060: HW configuration is not supported
[    6.160970] hub 1-1:1.0: USB hub found
[    6.168054] imx412 22-001a: failed to get reset gpio -517
[    6.170710] hub 1-1:1.0: 4 ports detected
[    6.175904] imx412 22-001a: HW configuration is not supported

Re: [PATCH RFC] gpiolib: ensure that fwnode is properly set

[Steev Klimaszewski]

Hi,

Others in the thread pointed to Thierry's patch, but on the Lenovo
Thinkpad X13s, that patch did *not* fix the issue, and with it
applied, the X13s still immediately reboots as soon as exiting EFI
services.  With this patch applied, the X13s does *not* reboot after
exiting EFI services, so thank you for this patch.

Tested-by: Steev Klimaszewski <steev@kali.org> #Lenovo Thinkpad X13s

Re: [PATCH RFC] gpiolib: ensure that fwnode is properly set

[Neil Armstrong]

On 14/11/2022 21:29, Brian Masney wrote:
> Note that this is a RFC patch and not meant to be merged. I looked into
> a problem with linux-next-20221110 on the Qualcomm SA8540P automotive
> board (sc8280xp) where the UFS host controller would fail to probe due
> to repeated probe deferrals when trying to get reset-gpios via
> devm_gpiod_get_optional().
> 
> of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by
> of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate function
> pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The
> pinctrl driver doesn't define one, so of_gpiochip_add() should
> automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't
> happen since the fwnode member on the struct gpiochip is set to null
> when of_gpiochip_add() is called. Let's work around this by ensuring
> that it's set if available.
> 
> Note that this broke sometime within the last few weeks within
> linux-next and I haven't bisected this. I'm posting this in the hopes
> that someone may know offhand which patch(es) may have broken this.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>   drivers/gpio/gpiolib.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 11fb7ec883e9..8bec66008869 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>   	 * Assign fwnode depending on the result of the previous calls,
>   	 * if none of them succeed, assign it to the parent's one.
>   	 */
> -	gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
> +	gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
>   
>   	gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
>   	if (gdev->id < 0) {

Fixes boot on 8550 on next-20221115

Tested-by: Neil Armstrong <neil.armstrong@linaro.org>

Thanks,
Neil

Re: [PATCH RFC] gpiolib: ensure that fwnode is properly set

[Bartosz Golaszewski]

On Tue, Nov 15, 2022 at 11:02 PM Steev Klimaszewski <steev@kali.org> wrote:
>
> Hi,
>
> Others in the thread pointed to Thierry's patch, but on the Lenovo
> Thinkpad X13s, that patch did *not* fix the issue, and with it
> applied, the X13s still immediately reboots as soon as exiting EFI
> services.  With this patch applied, the X13s does *not* reboot after
> exiting EFI services, so thank you for this patch.
>
> Tested-by: Steev Klimaszewski <steev@kali.org> #Lenovo Thinkpad X13s

My bad, I was under the impression that Thierry's patch fixed this
issue too. Now applied.

Bart

Re: [PATCH RFC] gpiolib: ensure that fwnode is properly set

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3910 bytes --]

On Mon, Nov 14, 2022 at 03:29:43PM -0500, Brian Masney wrote:
> Note that this is a RFC patch and not meant to be merged. I looked into
> a problem with linux-next-20221110 on the Qualcomm SA8540P automotive
> board (sc8280xp) where the UFS host controller would fail to probe due
> to repeated probe deferrals when trying to get reset-gpios via
> devm_gpiod_get_optional().
> 
> of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by
> of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate function
> pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The
> pinctrl driver doesn't define one, so of_gpiochip_add() should
> automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't
> happen since the fwnode member on the struct gpiochip is set to null
> when of_gpiochip_add() is called. Let's work around this by ensuring
> that it's set if available.
> 
> Note that this broke sometime within the last few weeks within
> linux-next and I haven't bisected this. I'm posting this in the hopes
> that someone may know offhand which patch(es) may have broken this.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/gpio/gpiolib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 11fb7ec883e9..8bec66008869 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>  	 * Assign fwnode depending on the result of the previous calls,
>  	 * if none of them succeed, assign it to the parent's one.
>  	 */
> -	gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
> +	gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;

This doesn't look right to me. Looking at the documentation of
gc->fwnode and how it is used, the purpose of this is to allow
explicitly overriding the fwnode that the GPIO chip will use.

So really this should not be used beyond the initial registration
in gpiochip_add_data_with_key(). If the above patch fixes anything,
then I suspect somebody is using gc->fwnode outside of this
registration.

Looking at gpiolib, the only remaining place that seems to do this is
the gpio-reserved-ranges handling code, in which case, the below on top
of my initial patch might fix that. That might explain why MSM is still
seeing issues.

--- >8 ---
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 11fb7ec883e9..d692ad5c5a27 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -447,10 +447,11 @@ static unsigned long *gpiochip_allocate_mask(struct gpio_chip *gc)
 
 static unsigned int gpiochip_count_reserved_ranges(struct gpio_chip *gc)
 {
+	struct fwnode_handle *fwnode = dev_fwnode(&gc->gpiodev->dev);
 	int size;
 
 	/* Format is "start, count, ..." */
-	size = fwnode_property_count_u32(gc->fwnode, "gpio-reserved-ranges");
+	size = fwnode_property_count_u32(fwnode, "gpio-reserved-ranges");
 	if (size > 0 && size % 2 == 0)
 		return size;
 
@@ -471,6 +472,7 @@ static int gpiochip_alloc_valid_mask(struct gpio_chip *gc)
 
 static int gpiochip_apply_reserved_ranges(struct gpio_chip *gc)
 {
+	struct fwnode_handle *fwnode = dev_fwnode(&gc->gpiodev->dev);
 	unsigned int size;
 	u32 *ranges;
 	int ret;
@@ -483,7 +485,7 @@ static int gpiochip_apply_reserved_ranges(struct gpio_chip *gc)
 	if (!ranges)
 		return -ENOMEM;
 
-	ret = fwnode_property_read_u32_array(gc->fwnode, "gpio-reserved-ranges", ranges, size);
+	ret = fwnode_property_read_u32_array(fwnode, "gpio-reserved-ranges", ranges, size);
 	if (ret) {
 		kfree(ranges);
 		return ret;
--- >8 ---

I don't have a good idea about the Lenovo X13 issue, though, but I
haven't looked at ACPI at all since I don't have any hardware to test
on.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC] gpiolib: ensure that fwnode is properly set

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 780 bytes --]

On Tue, Nov 15, 2022 at 04:02:38PM -0600, Steev Klimaszewski wrote:
> Hi,
> 
> Others in the thread pointed to Thierry's patch, but on the Lenovo
> Thinkpad X13s, that patch did *not* fix the issue, and with it
> applied, the X13s still immediately reboots as soon as exiting EFI
> services.  With this patch applied, the X13s does *not* reboot after
> exiting EFI services, so thank you for this patch.
> 
> Tested-by: Steev Klimaszewski <steev@kali.org> #Lenovo Thinkpad X13s

Do you happen to know what driver the X13s is using? As mentioned in
another email, I don't think overriding gc->fwnode at this point is the
right thing to do, so I suspect this field might be used unexpectedly in
some other place, so I'd like to take a closer look at that.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC] gpiolib: ensure that fwnode is properly set

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4357 bytes --]

On Wed, Nov 16, 2022 at 11:21:37AM +0100, Thierry Reding wrote:
> On Mon, Nov 14, 2022 at 03:29:43PM -0500, Brian Masney wrote:
> > Note that this is a RFC patch and not meant to be merged. I looked into
> > a problem with linux-next-20221110 on the Qualcomm SA8540P automotive
> > board (sc8280xp) where the UFS host controller would fail to probe due
> > to repeated probe deferrals when trying to get reset-gpios via
> > devm_gpiod_get_optional().
> > 
> > of_get_named_gpiod_flags() returns -EPROBE_DEFER, which is caused by
> > of_gpiochip_match_node_and_xlate() returning 0 since the of_xlate function
> > pointer is not set for the qcom,sc8280xp-tlmm pinctrl driver. The
> > pinctrl driver doesn't define one, so of_gpiochip_add() should
> > automatically setup of_gpio_simple_xlate() on it's behalf. This doesn't
> > happen since the fwnode member on the struct gpiochip is set to null
> > when of_gpiochip_add() is called. Let's work around this by ensuring
> > that it's set if available.
> > 
> > Note that this broke sometime within the last few weeks within
> > linux-next and I haven't bisected this. I'm posting this in the hopes
> > that someone may know offhand which patch(es) may have broken this.
> > 
> > Signed-off-by: Brian Masney <bmasney@redhat.com>
> > ---
> >  drivers/gpio/gpiolib.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 11fb7ec883e9..8bec66008869 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> >  	 * Assign fwnode depending on the result of the previous calls,
> >  	 * if none of them succeed, assign it to the parent's one.
> >  	 */
> > -	gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
> > +	gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
> 
> This doesn't look right to me. Looking at the documentation of
> gc->fwnode and how it is used, the purpose of this is to allow
> explicitly overriding the fwnode that the GPIO chip will use.
> 
> So really this should not be used beyond the initial registration
> in gpiochip_add_data_with_key(). If the above patch fixes anything,
> then I suspect somebody is using gc->fwnode outside of this
> registration.
> 
> Looking at gpiolib, the only remaining place that seems to do this is
> the gpio-reserved-ranges handling code, in which case, the below on top
> of my initial patch might fix that. That might explain why MSM is still
> seeing issues.
> 
> --- >8 ---
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 11fb7ec883e9..d692ad5c5a27 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -447,10 +447,11 @@ static unsigned long *gpiochip_allocate_mask(struct gpio_chip *gc)
>  
>  static unsigned int gpiochip_count_reserved_ranges(struct gpio_chip *gc)
>  {
> +	struct fwnode_handle *fwnode = dev_fwnode(&gc->gpiodev->dev);
>  	int size;
>  
>  	/* Format is "start, count, ..." */
> -	size = fwnode_property_count_u32(gc->fwnode, "gpio-reserved-ranges");
> +	size = fwnode_property_count_u32(fwnode, "gpio-reserved-ranges");
>  	if (size > 0 && size % 2 == 0)
>  		return size;
>  
> @@ -471,6 +472,7 @@ static int gpiochip_alloc_valid_mask(struct gpio_chip *gc)
>  
>  static int gpiochip_apply_reserved_ranges(struct gpio_chip *gc)
>  {
> +	struct fwnode_handle *fwnode = dev_fwnode(&gc->gpiodev->dev);
>  	unsigned int size;
>  	u32 *ranges;
>  	int ret;
> @@ -483,7 +485,7 @@ static int gpiochip_apply_reserved_ranges(struct gpio_chip *gc)
>  	if (!ranges)
>  		return -ENOMEM;
>  
> -	ret = fwnode_property_read_u32_array(gc->fwnode, "gpio-reserved-ranges", ranges, size);
> +	ret = fwnode_property_read_u32_array(fwnode, "gpio-reserved-ranges", ranges, size);
>  	if (ret) {
>  		kfree(ranges);
>  		return ret;
> --- >8 ---
> 
> I don't have a good idea about the Lenovo X13 issue, though, but I
> haven't looked at ACPI at all since I don't have any hardware to test
> on.

Ah... looks like that device was actually a Thinkpad X13*s*, which is
based on a Qualcomm chip, so maybe this patch fixes that one, too. It
does use gpio-reserved-ranges, so seems at least likely.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC] gpiolib: ensure that fwnode is properly set

[Brian Masney]

On Wed, Nov 16, 2022 at 11:21:37AM +0100, Thierry Reding wrote:
> On Mon, Nov 14, 2022 at 03:29:43PM -0500, Brian Masney wrote:
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 11fb7ec883e9..8bec66008869 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -678,7 +678,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> >  	 * Assign fwnode depending on the result of the previous calls,
> >  	 * if none of them succeed, assign it to the parent's one.
> >  	 */
> > -	gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
> > +	gc->fwnode = gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
> 
> This doesn't look right to me. Looking at the documentation of
> gc->fwnode and how it is used, the purpose of this is to allow
> explicitly overriding the fwnode that the GPIO chip will use.
> 
> So really this should not be used beyond the initial registration
> in gpiochip_add_data_with_key(). If the above patch fixes anything,
> then I suspect somebody is using gc->fwnode outside of this
> registration.
> 
> Looking at gpiolib, the only remaining place that seems to do this is
> the gpio-reserved-ranges handling code, in which case, the below on top
> of my initial patch might fix that. That might explain why MSM is still
> seeing issues.

That is correct. The Thinkpad x13s laptop uses the driver
drivers/pinctrl/qcom/pinctrl-sc8280xp.c and
arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts defines a
gpio-reserved-ranges property. The SA8540p automotive board has the same
SoC, however the DTS we are using doesn't use the gpio-reserved-ranges
property, and why only your original patch fixed the issue for this
board.

I think my patch should be removed from the GPIO tree if Thierry's two
patches work for everyone.

Brian