| Message ID | 20220601070707.3946847-4-saravanak@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | deferred_probe_timeout logic clean up | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
Hi Saravana, On Wed, Jun 1, 2022 at 2:44 PM Saravana Kannan <saravanak@google.com> wrote: > Now that fw_devlink=on by default and fw_devlink supports interrupt > properties, the execution will never get to the point where > driver_deferred_probe_check_state() is called before the supplier has > probed successfully or before deferred probe timeout has expired. > > So, delete the call and replace it with -ENODEV. > > Signed-off-by: Saravana Kannan <saravanak@google.com> Thanks for your patch, which is now commit f8217275b57aa48d ("net: mdio: Delete usage of driver_deferred_probe_check_state()") in driver-core/driver-core-next. Seems like I missed something when providing my T-b for this series, sorry for that. arch/arm/boot/dts/r8a7791-koelsch.dts has: ðer { pinctrl-0 = <ðer_pins>, <&phy1_pins>; pinctrl-names = "default"; phy-handle = <&phy1>; renesas,ether-link-active-low; status = "okay"; phy1: ethernet-phy@1 { compatible = "ethernet-phy-id0022.1537", "ethernet-phy-ieee802.3-c22"; reg = <1>; interrupt-parent = <&irqc0>; interrupts = <0 IRQ_TYPE_LEVEL_LOW>; micrel,led-mode = <1>; reset-gpios = <&gpio5 22 GPIO_ACTIVE_LOW>; }; }; Despite the interrupts property, ðer is now probed before irqc0 (interrupt-controller@e61c0000 in arch/arm/boot/dts/r8a7791.dtsi), causing the PHY not finding its interrupt, and resorting to polling: -Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=185) +Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=POLL) Reverting this commit, and commit 9cbffc7a59561be9 ("driver core: Delete driver_deferred_probe_check_state()") fixes that. > --- a/drivers/net/mdio/fwnode_mdio.c > +++ b/drivers/net/mdio/fwnode_mdio.c > @@ -47,9 +47,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, > * just fall back to poll mode > */ > if (rc == -EPROBE_DEFER) > - rc = driver_deferred_probe_check_state(&phy->mdio.dev); > - if (rc == -EPROBE_DEFER) > - return rc; > + rc = -ENODEV; > > if (rc > 0) { > phy->irq = rc; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, Jul 5, 2022 at 2:11 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Saravana, > > On Wed, Jun 1, 2022 at 2:44 PM Saravana Kannan <saravanak@google.com> wrote: > > Now that fw_devlink=on by default and fw_devlink supports interrupt > > properties, the execution will never get to the point where > > driver_deferred_probe_check_state() is called before the supplier has > > probed successfully or before deferred probe timeout has expired. > > > > So, delete the call and replace it with -ENODEV. > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > Thanks for your patch, which is now commit f8217275b57aa48d ("net: > mdio: Delete usage of driver_deferred_probe_check_state()") in > driver-core/driver-core-next. > > Seems like I missed something when providing my T-b for this series, > sorry for that. No worries. Appreciate any testing help. > > arch/arm/boot/dts/r8a7791-koelsch.dts has: > > ðer { > pinctrl-0 = <ðer_pins>, <&phy1_pins>; > pinctrl-names = "default"; > > phy-handle = <&phy1>; > renesas,ether-link-active-low; > status = "okay"; > > phy1: ethernet-phy@1 { > compatible = "ethernet-phy-id0022.1537", > "ethernet-phy-ieee802.3-c22"; > reg = <1>; > interrupt-parent = <&irqc0>; > interrupts = <0 IRQ_TYPE_LEVEL_LOW>; > micrel,led-mode = <1>; > reset-gpios = <&gpio5 22 GPIO_ACTIVE_LOW>; > }; > }; > > Despite the interrupts property, ðer is now probed before irqc0 > (interrupt-controller@e61c0000 in arch/arm/boot/dts/r8a7791.dtsi), > causing the PHY not finding its interrupt, and resorting to polling: I'd still expect the device link to have been created properly for this phy device. Could you enable the logging in device_link_add() to check the link is created between the phy and the IRQ? My guess is that this probably has something to do with phys being attached to drivers differently. > > -Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY > driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=185) > +Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY > driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=POLL) Can you drop a WARN() where this is printed to get the stack trace to check my hypothesis? -Saravana > > Reverting this commit, and commit 9cbffc7a59561be9 ("driver core: > Delete driver_deferred_probe_check_state()") fixes that. > > > --- a/drivers/net/mdio/fwnode_mdio.c > > +++ b/drivers/net/mdio/fwnode_mdio.c > > @@ -47,9 +47,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, > > * just fall back to poll mode > > */ > > if (rc == -EPROBE_DEFER) > > - rc = driver_deferred_probe_check_state(&phy->mdio.dev); > > - if (rc == -EPROBE_DEFER) > > - return rc; > > + rc = -ENODEV; > > > > if (rc > 0) { > > phy->irq = rc; > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Saravana, On Wed, Jul 13, 2022 at 3:40 AM Saravana Kannan <saravanak@google.com> wrote: > On Tue, Jul 5, 2022 at 2:11 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Wed, Jun 1, 2022 at 2:44 PM Saravana Kannan <saravanak@google.com> wrote: > > > Now that fw_devlink=on by default and fw_devlink supports interrupt > > > properties, the execution will never get to the point where > > > driver_deferred_probe_check_state() is called before the supplier has > > > probed successfully or before deferred probe timeout has expired. > > > > > > So, delete the call and replace it with -ENODEV. > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > Thanks for your patch, which is now commit f8217275b57aa48d ("net: > > mdio: Delete usage of driver_deferred_probe_check_state()") in > > driver-core/driver-core-next. > > > > Seems like I missed something when providing my T-b for this series, > > sorry for that. > > > arch/arm/boot/dts/r8a7791-koelsch.dts has: > > > > ðer { > > pinctrl-0 = <ðer_pins>, <&phy1_pins>; > > pinctrl-names = "default"; > > > > phy-handle = <&phy1>; > > renesas,ether-link-active-low; > > status = "okay"; > > > > phy1: ethernet-phy@1 { > > compatible = "ethernet-phy-id0022.1537", > > "ethernet-phy-ieee802.3-c22"; > > reg = <1>; > > interrupt-parent = <&irqc0>; > > interrupts = <0 IRQ_TYPE_LEVEL_LOW>; > > micrel,led-mode = <1>; > > reset-gpios = <&gpio5 22 GPIO_ACTIVE_LOW>; > > }; > > }; > > > > Despite the interrupts property, ðer is now probed before irqc0 > > (interrupt-controller@e61c0000 in arch/arm/boot/dts/r8a7791.dtsi), > > causing the PHY not finding its interrupt, and resorting to polling: > > I'd still expect the device link to have been created properly for > this phy device. Could you enable the logging in device_link_add() to > check the link is created between the phy and the IRQ? > > My guess is that this probably has something to do with phys being > attached to drivers differently. Comparison of dmesg before/after enabling debugging, for related nodes: +interrupt-controller@e61c0000 Linked as a fwnode consumer to clock-controller@e6150000 +pmic@58 Linked as a fwnode consumer to interrupt-controller@e61c0000 +regulator@68 Linked as a fwnode consumer to interrupt-controller@e61c0000 Other user of irqc +ethernet@ee700000 Linked as a fwnode consumer to clock-controller@e6150000 +ethernet@ee700000 Linked as a fwnode consumer to pinctrl@e6060000 +ethernet-phy@1 Linked as a fwnode consumer to interrupt-controller@e61c0000 +ethernet-phy@1 Linked as a fwnode consumer to gpio@e6055000 PHY linked correctly to consumers +device: 'e61c0000.interrupt-controller': device_add +device: 'platform:e6150000.clock-controller--platform:e61c0000.interrupt-controller': device_add +devices_kset: Moving e61c0000.interrupt-controller to end of list +platform e61c0000.interrupt-controller: Linked as a consumer to e6150000.clock-controller +interrupt-controller@e61c0000 Dropping the fwnode link to clock-controller@e6150000 +platform e61c0000.interrupt-controller: error -EPROBE_DEFER: supplier e6150000.clock-controller not ready Tried to probe irqc (why? consumer not ready), deferred. +device: 'platform:e61c0000.interrupt-controller--platform:e60b0000.i2c': device_add +platform e60b0000.i2c: Linked as a sync state only consumer to e61c0000.interrupt-controller I guess sync state means through other (child) consumers (pmic, regulator) above? +device: 'ee700000.ethernet': device_add +device: 'platform:e6060000.pinctrl--platform:ee700000.ethernet': device_add +devices_kset: Moving ee700000.ethernet to end of list +platform ee700000.ethernet: Linked as a consumer to e6060000.pinctrl +ethernet@ee700000 Dropping the fwnode link to pinctrl@e6060000 +device: 'platform:e6150000.clock-controller--platform:ee700000.ethernet': device_add +devices_kset: Moving ee700000.ethernet to end of list +platform ee700000.ethernet: Linked as a consumer to e6150000.clock-controller +ethernet@ee700000 Dropping the fwnode link to clock-controller@e6150000 +device: 'platform:e6055000.gpio--platform:ee700000.ethernet': device_add +platform ee700000.ethernet: Linked as a sync state only consumer to e6055000.gpio +device: 'platform:e61c0000.interrupt-controller--platform:ee700000.ethernet': device_add +platform ee700000.ethernet: Linked as a sync state only consumer to e61c0000.interrupt-controller Hence linking ethernet to child (phy) consumers. +device: 'ee700000.ethernet-ffffffff': device_add Probing ethernet... libphy: fwnode_get_phy_id: fwnode /soc/ethernet@ee700000/ethernet-phy@1 phy_id = 0x00221537 libphy: fwnode_get_phy_id: fwnode /soc/ethernet@ee700000/ethernet-phy@1 phy_id = 0x00221537 +fwnode_mdiobus_phy_device_register: fwnode_irq_get() returned -517 +fwnode_mdiobus_phy_device_register: ignoring -EPROBE_DEFER This is the part that got changed by this patch. +device: 'ee700000.ethernet-ffffffff:01': device_add +device: 'platform:e6055000.gpio--mdio_bus:ee700000.ethernet-ffffffff:01': device_add +devices_kset: Moving ee700000.ethernet-ffffffff:01 to end of list +mdio_bus ee700000.ethernet-ffffffff:01: Linked as a consumer to e6055000.gpio +ethernet-phy@1 Dropping the fwnode link to gpio@e6055000 +device: 'platform:e61c0000.interrupt-controller--mdio_bus:ee700000.ethernet-ffffffff:01': device_add +devices_kset: Moving ee700000.ethernet-ffffffff:01 to end of list +mdio_bus ee700000.ethernet-ffffffff:01: Linked as a consumer to e61c0000.interrupt-controller +ethernet-phy@1 Dropping the fwnode link to interrupt-controller@e61c0000 +mdio_bus ee700000.ethernet-ffffffff:01: error -EPROBE_DEFER: supplier e61c0000.interrupt-controller not ready Why was ethernet probed this early? We knew the supplier of the phy was still missing? +device: 'eth1': device_add sh-eth ee700000.ethernet eth1: Base address at 0xee700000, 2e:09:0a:00:6d:85, IRQ 104. +sh-eth ee700000.ethernet: Dropping the link to e6055000.gpio +device: 'platform:e6055000.gpio--platform:ee700000.ethernet': device_unregister +sh-eth ee700000.ethernet: Dropping the link to e61c0000.interrupt-controller +device: 'platform:e61c0000.interrupt-controller--platform:ee700000.ethernet': device_unregister +devices_kset: Moving e61c0000.interrupt-controller to end of list +devices_kset: Moving ee700000.ethernet-ffffffff:01 to end of list renesas_irqc e61c0000.interrupt-controller: driving 10 irqs Finally, irqc is probed. +device: '6-0058': device_add +device: 'platform:e61c0000.interrupt-controller--i2c:6-0058': device_add +devices_kset: Moving 6-0058 to end of list +i2c 6-0058: Linked as a consumer to e61c0000.interrupt-controller +pmic@58 Dropping the fwnode link to interrupt-controller@e61c0000 +device: '6-0068': device_add +device: 'platform:e61c0000.interrupt-controller--i2c:6-0068': device_add +devices_kset: Moving 6-0068 to end of list +i2c 6-0068: Linked as a consumer to e61c0000.interrupt-controller +regulator@68 Dropping the fwnode link to interrupt-controller@e61c0000 Propagating other irqc suppliers to the parent of their consumers +i2c-sh_mobile e60b0000.i2c: Dropping the link to e61c0000.interrupt-controller +device: 'platform:e61c0000.interrupt-controller--platform:e60b0000.i2c': device_unregister +devices_kset: Moving ee700000.ethernet-ffffffff:01 to end of list Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=POLL) sh-eth ee700000.ethernet eth1: Link is Up - 100Mbps/Full - flow control off Sending DHCP requests ., OK > > -Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY > > driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=185) > > +Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY > > driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=POLL) > > Can you drop a WARN() where this is printed to get the stack trace to > check my hypothesis? That didn't help much, as this is the messenger, not the cause. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Saravana, On Tue, Jul 5, 2022 at 11:11 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Wed, Jun 1, 2022 at 2:44 PM Saravana Kannan <saravanak@google.com> wrote: > > Now that fw_devlink=on by default and fw_devlink supports interrupt > > properties, the execution will never get to the point where > > driver_deferred_probe_check_state() is called before the supplier has > > probed successfully or before deferred probe timeout has expired. > > > > So, delete the call and replace it with -ENODEV. > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > Thanks for your patch, which is now commit f8217275b57aa48d ("net: > mdio: Delete usage of driver_deferred_probe_check_state()") in > driver-core/driver-core-next. > > Seems like I missed something when providing my T-b for this series, > sorry for that. > > arch/arm/boot/dts/r8a7791-koelsch.dts has: > > ðer { > pinctrl-0 = <ðer_pins>, <&phy1_pins>; > pinctrl-names = "default"; > > phy-handle = <&phy1>; > renesas,ether-link-active-low; > status = "okay"; > > phy1: ethernet-phy@1 { > compatible = "ethernet-phy-id0022.1537", > "ethernet-phy-ieee802.3-c22"; > reg = <1>; > interrupt-parent = <&irqc0>; > interrupts = <0 IRQ_TYPE_LEVEL_LOW>; > micrel,led-mode = <1>; > reset-gpios = <&gpio5 22 GPIO_ACTIVE_LOW>; > }; > }; > > Despite the interrupts property, ðer is now probed before irqc0 > (interrupt-controller@e61c0000 in arch/arm/boot/dts/r8a7791.dtsi), > causing the PHY not finding its interrupt, and resorting to polling: > > -Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY > driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=185) > +Micrel KSZ8041RNLI ee700000.ethernet-ffffffff:01: attached PHY > driver (mii_bus:phy_addr=ee700000.ethernet-ffffffff:01, irq=POLL) > > Reverting this commit, and commit 9cbffc7a59561be9 ("driver core: > Delete driver_deferred_probe_check_state()") fixes that. FTR, this issue is now present in v6.0-rc1. I haven't tried your newest series yet. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c index 1c1584fca632..3e79c2c51929 100644 --- a/drivers/net/mdio/fwnode_mdio.c +++ b/drivers/net/mdio/fwnode_mdio.c @@ -47,9 +47,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, * just fall back to poll mode */ if (rc == -EPROBE_DEFER) - rc = driver_deferred_probe_check_state(&phy->mdio.dev); - if (rc == -EPROBE_DEFER) - return rc; + rc = -ENODEV; if (rc > 0) { phy->irq = rc;
Now that fw_devlink=on by default and fw_devlink supports interrupt properties, the execution will never get to the point where driver_deferred_probe_check_state() is called before the supplier has probed successfully or before deferred probe timeout has expired. So, delete the call and replace it with -ENODEV. Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/net/mdio/fwnode_mdio.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)