There are two issues in rockchip_usb2phy_otg_port_init(): (1) even if
devm_extcon_register_notifier() returns error, the code proceeds to
the next if statement and (2) if no extcon is defined in of_node, the
return value of property_enabled() is reused as the return value of
the whole function. If the return value of property_enable() is
nonzero, (2) results in an unexpected probe failure and kernel panic
in delayed work:
Unable to handle kernel NULL pointer dereference at virtual address 00000000
Mem abort info:
ESR = 0x0000000086000006
EC = 0x21: IABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
FSC = 0x06: level 2 translation fault
user pgtable: 4k pages, 48-bit VAs, pgdp=00000000019dc000
[0000000000000000] pgd=080000000131a003, p4d=080000000131a003, pud=080000000
Internal error: Oops: 86000006 [#1] PREEMPT SMP
Modules linked in: ipv6
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 6.0.0-rc5 #114
Hardware name: FriendlyElec NanoPi M4 (DT)
pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : 0x0
lr : call_timer_fn.constprop.0+0x24/0x80
sp : ffff80000a40ba40
x29: ffff80000a40ba40 x28: 0000000000000000 x27: ffff80000a40baf0
x26: ffff800009e779c0 x25: ffff00007fb28070 x24: ffff00007fb28028
x23: ffff80000a40baf0 x22: 0000000000000000 x21: 0000000000000101
x20: ffff0000006ad880 x19: 0000000000000000 x18: 0000000000000006
x17: ffff8000761af000 x16: ffff80000800c000 x15: 0000000000004000
x14: ffff0000006ad880 x13: 000000000000030a x12: 0000000000000000
x11: ffff8000761af000 x10: ffff80000800bf40 x9 : ffff00007fb2d038
x8 : 0000000000000001 x7 : 0000000000000009 x6 : 0000000000000240
x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000200
x2 : 000000003fffffff x1 : 0000000000000000 x0 : ffff0000016c9710
Call trace:
0x0
__run_timers+0x220/0x264
run_timer_softirq+0x20/0x40
__do_softirq+0x10c/0x298
__irq_exit_rcu+0xec/0xf4
irq_exit_rcu+0x10/0x1c
el1_interrupt+0x38/0x70
el1h_64_irq_handler+0x18/0x24
el1h_64_irq+0x64/0x68
cpuidle_enter_state+0x130/0x2fc
cpuidle_enter+0x38/0x50
do_idle+0x22c/0x2c0
cpu_startup_entry+0x24/0x30
secondary_start_kernel+0x130/0x14c
__secondary_switched+0xb0/0xb4
Code: bad PC value
---[ end trace 0000000000000000 ]---
Kernel panic - not syncing: Oops: Fatal exception in interrupt
SMP: stopping secondary CPUs
Kernel Offset: disabled
CPU features: 0x4000,0820b021,00001086
Memory Limit: none
---[ end Kernel panic - not syncing: Oops: Fatal exception in interrupt ]---
Refactor the control flow to avoid both issues. Since the code below
out: label does no cleanup, return error codes immediately instead of
using gotos and return success at the end of the function.
Cc: [email protected]
Fixes: 8dc60f8da22f ("phy: rockchip-inno-usb2: Sync initial otg state")
Signed-off-by: Mikhail Rudenko <[email protected]>
---
drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 0b1e9337ee8e..d31b35d55927 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -1144,8 +1144,7 @@ static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
rport->mode = of_usb_get_dr_mode_by_phy(child_np, -1);
if (rport->mode == USB_DR_MODE_HOST ||
rport->mode == USB_DR_MODE_UNKNOWN) {
- ret = 0;
- goto out;
+ return 0;
}
INIT_DELAYED_WORK(&rport->chg_work, rockchip_chg_detect_work);
@@ -1154,7 +1153,7 @@ static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
ret = rockchip_usb2phy_port_irq_init(rphy, rport, child_np);
if (ret) {
dev_err(rphy->dev, "failed to init irq for host port\n");
- goto out;
+ return ret;
}
if (!IS_ERR(rphy->edev)) {
@@ -1162,8 +1161,10 @@ static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
ret = devm_extcon_register_notifier(rphy->dev, rphy->edev,
EXTCON_USB_HOST, &rport->event_nb);
- if (ret)
+ if (ret) {
dev_err(rphy->dev, "register USB HOST notifier failed\n");
+ return ret;
+ }
if (!of_property_read_bool(rphy->dev->of_node, "extcon")) {
/* do initial sync of usb state */
@@ -1172,8 +1173,7 @@ static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
}
}
-out:
- return ret;
+ return 0;
}
static int rockchip_usb2phy_probe(struct platform_device *pdev)
--
2.37.3
On 9/17/22 11:58, Mikhail Rudenko wrote:
> There are two issues in rockchip_usb2phy_otg_port_init(): (1) even if
> devm_extcon_register_notifier() returns error, the code proceeds to
> the next if statement and (2) if no extcon is defined in of_node, the
> return value of property_enabled() is reused as the return value of
> the whole function. If the return value of property_enable() is
> nonzero, (2) results in an unexpected probe failure and kernel panic
> in delayed work:
This should be fixed by the following patch which is accepted already:
https://lore.kernel.org/lkml/[email protected]/
>
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> Mem abort info:
> ESR = 0x0000000086000006
> EC = 0x21: IABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> FSC = 0x06: level 2 translation fault
> user pgtable: 4k pages, 48-bit VAs, pgdp=00000000019dc000
> [0000000000000000] pgd=080000000131a003, p4d=080000000131a003, pud=080000000
> Internal error: Oops: 86000006 [#1] PREEMPT SMP
> Modules linked in: ipv6
> CPU: 1 PID: 0 Comm: swapper/1 Not tainted 6.0.0-rc5 #114
> Hardware name: FriendlyElec NanoPi M4 (DT)
> pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : 0x0
> lr : call_timer_fn.constprop.0+0x24/0x80
> sp : ffff80000a40ba40
> x29: ffff80000a40ba40 x28: 0000000000000000 x27: ffff80000a40baf0
> x26: ffff800009e779c0 x25: ffff00007fb28070 x24: ffff00007fb28028
> x23: ffff80000a40baf0 x22: 0000000000000000 x21: 0000000000000101
> x20: ffff0000006ad880 x19: 0000000000000000 x18: 0000000000000006
> x17: ffff8000761af000 x16: ffff80000800c000 x15: 0000000000004000
> x14: ffff0000006ad880 x13: 000000000000030a x12: 0000000000000000
> x11: ffff8000761af000 x10: ffff80000800bf40 x9 : ffff00007fb2d038
> x8 : 0000000000000001 x7 : 0000000000000009 x6 : 0000000000000240
> x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000200
> x2 : 000000003fffffff x1 : 0000000000000000 x0 : ffff0000016c9710
> Call trace:
> 0x0
> __run_timers+0x220/0x264
> run_timer_softirq+0x20/0x40
> __do_softirq+0x10c/0x298
> __irq_exit_rcu+0xec/0xf4
> irq_exit_rcu+0x10/0x1c
> el1_interrupt+0x38/0x70
> el1h_64_irq_handler+0x18/0x24
> el1h_64_irq+0x64/0x68
> cpuidle_enter_state+0x130/0x2fc
> cpuidle_enter+0x38/0x50
> do_idle+0x22c/0x2c0
> cpu_startup_entry+0x24/0x30
> secondary_start_kernel+0x130/0x14c
> __secondary_switched+0xb0/0xb4
> Code: bad PC value
> ---[ end trace 0000000000000000 ]---
> Kernel panic - not syncing: Oops: Fatal exception in interrupt
> SMP: stopping secondary CPUs
> Kernel Offset: disabled
> CPU features: 0x4000,0820b021,00001086
> Memory Limit: none
> ---[ end Kernel panic - not syncing: Oops: Fatal exception in interrupt ]---
>
> Refactor the control flow to avoid both issues. Since the code below
> out: label does no cleanup, return error codes immediately instead of
> using gotos and return success at the end of the function.
>
> Cc: [email protected]
> Fixes: 8dc60f8da22f ("phy: rockchip-inno-usb2: Sync initial otg state")
> Signed-off-by: Mikhail Rudenko <[email protected]>
> ---
> drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index 0b1e9337ee8e..d31b35d55927 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> @@ -1144,8 +1144,7 @@ static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
> rport->mode = of_usb_get_dr_mode_by_phy(child_np, -1);
> if (rport->mode == USB_DR_MODE_HOST ||
> rport->mode == USB_DR_MODE_UNKNOWN) {
> - ret = 0;
> - goto out;
> + return 0;
> }
>
> INIT_DELAYED_WORK(&rport->chg_work, rockchip_chg_detect_work);
> @@ -1154,7 +1153,7 @@ static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
> ret = rockchip_usb2phy_port_irq_init(rphy, rport, child_np);
> if (ret) {
> dev_err(rphy->dev, "failed to init irq for host port\n");
> - goto out;
> + return ret;
> }
>
> if (!IS_ERR(rphy->edev)) {
> @@ -1162,8 +1161,10 @@ static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
>
> ret = devm_extcon_register_notifier(rphy->dev, rphy->edev,
> EXTCON_USB_HOST, &rport->event_nb);
> - if (ret)
> + if (ret) {
> dev_err(rphy->dev, "register USB HOST notifier failed\n");
> + return ret;
> + }
>
> if (!of_property_read_bool(rphy->dev->of_node, "extcon")) {
> /* do initial sync of usb state */
> @@ -1172,8 +1173,7 @@ static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
> }
> }
>
> -out:
> - return ret;
> + return 0;
> }
>
> static int rockchip_usb2phy_probe(struct platform_device *pdev)
On 2022-09-17 at 12:12 -05, Samuel Holland <[email protected]> wrote:
> On 9/17/22 11:58, Mikhail Rudenko wrote:
>> There are two issues in rockchip_usb2phy_otg_port_init(): (1) even if
>> devm_extcon_register_notifier() returns error, the code proceeds to
>> the next if statement and (2) if no extcon is defined in of_node, the
>> return value of property_enabled() is reused as the return value of
>> the whole function. If the return value of property_enable() is
>> nonzero, (2) results in an unexpected probe failure and kernel panic
>> in delayed work:
>
> This should be fixed by the following patch which is accepted already:
>
> https://lore.kernel.org/lkml/[email protected]/
Ah, I see now. Sorry for the noise!
--
Best regards,
Mikhail Rudenko