| Message ID | 20221114202943.2389489-1-bmasney@redhat.com |
|---|---|
| State | Accepted |
| Commit | 24c94060fc9b4e0f19e6e018869db46db21d6bc7 |
| Headers | show |
| Series | [RFC] gpiolib: ensure that fwnode is properly set | expand |
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
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 >
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) {
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
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
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
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
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
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) {
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(-)