| Message ID | 20220406202323.3390405-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: mdio: don't defer probe forever if PHY IRQ provider is missing | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | fail | Errors and warnings before: 0 this patch: 4 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | fail | Errors and warnings before: 0 this patch: 4 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 0 this patch: 4 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 11 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Wed, 6 Apr 2022 23:23:23 +0300 Vladimir Oltean wrote: > diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c > index 1becb1a731f6..1c1584fca632 100644 > --- a/drivers/net/mdio/fwnode_mdio.c > +++ b/drivers/net/mdio/fwnode_mdio.c > @@ -43,6 +43,11 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, > int rc; > > rc = fwnode_irq_get(child, 0); > + /* Don't wait forever if the IRQ provider doesn't become available, > + * just fall back to poll mode > + */ > + if (rc == -EPROBE_DEFER) > + rc = driver_deferred_probe_check_state(&phy->mdio.dev); This one's not exported, allmodconfig build fails.
On Wed, Apr 06, 2022 at 03:34:43PM -0700, Jakub Kicinski wrote: > On Wed, 6 Apr 2022 23:23:23 +0300 Vladimir Oltean wrote: > > diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c > > index 1becb1a731f6..1c1584fca632 100644 > > --- a/drivers/net/mdio/fwnode_mdio.c > > +++ b/drivers/net/mdio/fwnode_mdio.c > > @@ -43,6 +43,11 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, > > int rc; > > > > rc = fwnode_irq_get(child, 0); > > + /* Don't wait forever if the IRQ provider doesn't become available, > > + * just fall back to poll mode > > + */ > > + if (rc == -EPROBE_DEFER) > > + rc = driver_deferred_probe_check_state(&phy->mdio.dev); > > This one's not exported, allmodconfig build fails. Oops, I didn't realize that all its callers except for FWNODE_MDIO are built-in. Do you prefer me exporting the symbol as part of the same patch or a different one?
On Wed, 6 Apr 2022 23:19:57 +0000 Vladimir Oltean wrote: > On Wed, Apr 06, 2022 at 03:34:43PM -0700, Jakub Kicinski wrote: > > > + if (rc == -EPROBE_DEFER) > > > + rc = driver_deferred_probe_check_state(&phy->mdio.dev); > > This one's not exported, allmodconfig build fails. > > Oops, I didn't realize that all its callers except for FWNODE_MDIO are built-in. > > Do you prefer me exporting the symbol as part of the same patch or a different one? I presume single patch is fine, but driver_deferred_probe_check_state() lives in Greg's realm, so let's add Greg in case he prefers a separate patch or more.
On Wed, Apr 06, 2022 at 05:25:50PM -0700, Jakub Kicinski wrote: > On Wed, 6 Apr 2022 23:19:57 +0000 Vladimir Oltean wrote: > > On Wed, Apr 06, 2022 at 03:34:43PM -0700, Jakub Kicinski wrote: > > > > + if (rc == -EPROBE_DEFER) > > > > + rc = driver_deferred_probe_check_state(&phy->mdio.dev); > > > This one's not exported, allmodconfig build fails. > > > > Oops, I didn't realize that all its callers except for FWNODE_MDIO are built-in. > > > > Do you prefer me exporting the symbol as part of the same patch or a different one? > > I presume single patch is fine, but driver_deferred_probe_check_state() > lives in Greg's realm, so let's add Greg in case he prefers a separate > patch or more. separate patch that I can NAK as I do not understand why this is needed at all :) thanks, greg k-h
On Thu, 7 Apr 2022 07:23:24 +0200 Greg Kroah-Hartman wrote: > separate patch that I can NAK as I do not understand why this is needed > at all :) Just in case you meant lack of context rather than the patch itself, here's the original posting from lore: https://lore.kernel.org/all/20220406202323.3390405-1-vladimir.oltean@nxp.com/ tl;dr we can fallback to polling if we can't get the IRQ, afaiu.
On Wed, Apr 06, 2022 at 10:35:24PM -0700, Jakub Kicinski wrote: > On Thu, 7 Apr 2022 07:23:24 +0200 Greg Kroah-Hartman wrote: > > separate patch that I can NAK as I do not understand why this is needed > > at all :) > > Just in case you meant lack of context rather than the patch itself, > here's the original posting from lore: > > https://lore.kernel.org/all/20220406202323.3390405-1-vladimir.oltean@nxp.com/ > > tl;dr we can fallback to polling if we can't get the IRQ, afaiu. Ah, ok, that seems sane.
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c index 1becb1a731f6..1c1584fca632 100644 --- a/drivers/net/mdio/fwnode_mdio.c +++ b/drivers/net/mdio/fwnode_mdio.c @@ -43,6 +43,11 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, int rc; rc = fwnode_irq_get(child, 0); + /* Don't wait forever if the IRQ provider doesn't become available, + * 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;
When a driver for an interrupt controller is missing, of_irq_get() returns -EPROBE_DEFER ad infinitum, causing fwnode_mdiobus_phy_device_register(), and ultimately, the entire of_mdiobus_register() call, to fail. In turn, any phy_connect() call towards a PHY on this MDIO bus will also fail. This is not what is expected to happen, because the PHY library falls back to poll mode when of_irq_get() returns a hard error code, and the MDIO bus, PHY and attached Ethernet controller work fine, albeit suboptimally, when the PHY library polls for link status. However, -EPROBE_DEFER has special handling given the assumption that at some point probe deferral will stop, and the driver for the supplier will kick in and create the IRQ domain. Reasons for which the interrupt controller may be missing: - It is not yet written. This may happen if a more recent DT blob (with an interrupt-parent for the PHY) is used to boot an old kernel where the driver didn't exist, and that kernel worked with the vintage-correct DT blob using poll mode. - It is compiled out. Behavior is the same as above. - It is compiled as a module. The kernel will wait for a number of seconds specified in the "deferred_probe_timeout" boot parameter for user space to load the required module. The current default is 0, which times out at the end of initcalls. It is possible that this might cause regressions unless users adjust this boot parameter. The proposed solution is to use the driver_deferred_probe_check_state() helper function provided by the driver core, which gives up after some -EPROBE_DEFER attempts, taking "deferred_probe_timeout" into consideration. The return code is changed from -EPROBE_DEFER into -ENODEV or -ETIMEDOUT, depending on whether the kernel is compiled with support for modules or not. Fixes: 66bdede495c7 ("of_mdio: Fix broken PHY IRQ in case of probe deferral") Suggested-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- This will not apply to stable kernels prior to commit bc1bee3b87ee ("net: mdiobus: Introduce fwnode_mdiobus_register_phy()"). I am planning to send an adapted patch for those when Greg sends out the emails stating that the patch fails to apply. drivers/net/mdio/fwnode_mdio.c | 5 +++++ 1 file changed, 5 insertions(+)