| Message ID | 20240613144112.349707-1-paul.geurts@prodrive-technologies.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fec_main: Register net device before initializing the MDIO bus | expand |
On Thu, Jun 13, 2024 at 04:41:11PM +0200, Paul Geurts wrote: > Registration of the FEC MDIO bus triggers a probe of all devices > connected to that bus. DSA based Ethernet switch devices connect to the > uplink Ethernet port during probe. When a DSA based, MDIO controlled > Ethernet switch is connected to FEC, it cannot connect the uplink port, > as the FEC MDIO port is registered before the net device is being > registered. This causes an unnecessary defer of the Ethernet switch > driver probe. > > Register the net device before initializing and registering the MDIO > bus. The problem with this is, as soon as you call register_netdev(), the device is alive and sending packets. It can be sending packets even before register_netdev() returns, e.g. in the case of NFS root. So fec_enet_open() gets called, and tried to find its PHY. But the MDIO bus is not registered yet.... So yes, DSA ends up doing an EPROBE_DEFER cycle. Not much we can do about that. We can try to keep the DSA probe functions as cheap as possible, and put all the expensive stuff in setup(), which will only be called once we have all the needed resources. Andrew --- pw-bot: cr
> On Thu, Jun 13, 2024 at 04:41:11PM +0200, Paul Geurts wrote: > > Registration of the FEC MDIO bus triggers a probe of all devices > > connected to that bus. DSA based Ethernet switch devices connect to the > > uplink Ethernet port during probe. When a DSA based, MDIO controlled > > Ethernet switch is connected to FEC, it cannot connect the uplink port, > > as the FEC MDIO port is registered before the net device is being > > registered. This causes an unnecessary defer of the Ethernet switch > > driver probe. > > > > Register the net device before initializing and registering the MDIO > > bus. > > The problem with this is, as soon as you call register_netdev(), the > device is alive and sending packets. It can be sending packets even > before register_netdev() returns, e.g. in the case of NFS root. So > fec_enet_open() gets called, and tried to find its PHY. But the MDIO > bus is not registered yet.... Valid argument there. I was trying to make the initialization more efficient, but you are correct. > > So yes, DSA ends up doing an EPROBE_DEFER cycle. Not much we can do > about that. We can try to keep the DSA probe functions as cheap as > possible, and put all the expensive stuff in setup(), which will only > be called once we have all the needed resources. > > Andrew > > --- > pw-bot: cr >
On Fri, Jun 14, 2024 at 10:40:50AM +0200, Paul Geurts wrote: > > On Thu, Jun 13, 2024 at 04:41:11PM +0200, Paul Geurts wrote: > > > Registration of the FEC MDIO bus triggers a probe of all devices > > > connected to that bus. DSA based Ethernet switch devices connect to the > > > uplink Ethernet port during probe. When a DSA based, MDIO controlled > > > Ethernet switch is connected to FEC, it cannot connect the uplink port, > > > as the FEC MDIO port is registered before the net device is being > > > registered. This causes an unnecessary defer of the Ethernet switch > > > driver probe. > > > > > > Register the net device before initializing and registering the MDIO > > > bus. > > > > The problem with this is, as soon as you call register_netdev(), the > > device is alive and sending packets. It can be sending packets even > > before register_netdev() returns, e.g. in the case of NFS root. So > > fec_enet_open() gets called, and tried to find its PHY. But the MDIO > > bus is not registered yet.... > > Valid argument there. I was trying to make the initialization more efficient, > but you are correct. Any changes to make this more efficient probably needs to be generic, and not in a specific Ethernet driver. One idea might be, when parsing the DT node for the MDIO bus, look to see if the device on the bus has a compatible which indicates it is not a PHY. If so, try to make the driver core put the device straight into deferred state, without even trying the first probe. Also, look at driver core devlink stuff. It tries to keep track of resource dependencies, and only probe devices when their resources are available. It does not seem to understand DSA too well, and it might be made better. But this is complex, we have dependency loops in network drivers, which causes devlink issues. Andrew
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 881ece735dcf..ed71f1f25ab9 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -4500,10 +4500,6 @@ fec_probe(struct platform_device *pdev) /* Decide which interrupt line is wakeup capable */ fec_enet_get_wakeup_irq(pdev); - ret = fec_enet_mii_init(pdev); - if (ret) - goto failed_mii_init; - /* Carrier starts down, phylib will bring it up */ netif_carrier_off(ndev); fec_enet_clk_enable(ndev, false); @@ -4515,6 +4511,10 @@ fec_probe(struct platform_device *pdev) if (ret) goto failed_register; + ret = fec_enet_mii_init(pdev); + if (ret) + goto failed_mii_init; + device_init_wakeup(&ndev->dev, fep->wol_flag & FEC_WOL_HAS_MAGIC_PACKET); @@ -4528,9 +4528,9 @@ fec_probe(struct platform_device *pdev) return 0; -failed_register: - fec_enet_mii_remove(fep); failed_mii_init: + unregister_netdev(ndev); +failed_register: failed_irq: fec_enet_deinit(ndev); failed_init: @@ -4577,8 +4577,8 @@ fec_drv_remove(struct platform_device *pdev) cancel_work_sync(&fep->tx_timeout_work); fec_ptp_stop(pdev); - unregister_netdev(ndev); fec_enet_mii_remove(fep); + unregister_netdev(ndev); if (fep->reg_phy) regulator_disable(fep->reg_phy);
Registration of the FEC MDIO bus triggers a probe of all devices connected to that bus. DSA based Ethernet switch devices connect to the uplink Ethernet port during probe. When a DSA based, MDIO controlled Ethernet switch is connected to FEC, it cannot connect the uplink port, as the FEC MDIO port is registered before the net device is being registered. This causes an unnecessary defer of the Ethernet switch driver probe. Register the net device before initializing and registering the MDIO bus. Fixes: e6b043d512fa ("netdev/fec.c: add phylib supporting to enable carrier detection (v2)") Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com> --- drivers/net/ethernet/freescale/fec_main.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)