In shared MDIO suspend/resume usecase for ex. with MDIO producer
(0xff0c0000) eth1 and MDIO consumer(0xff0b0000) eth0 there is a
constraint that ethernet interface(ff0c0000) MDIO bus producer
has to be resumed before the consumer ethernet interface(ff0b0000).
However above constraint is not met when GEM0(ff0b0000) is resumed first.
There is phy_error on GEM0 and interface becomes non-functional on resume.
suspend:
[ 46.477795] macb ff0c0000.ethernet eth1: Link is Down
[ 46.483058] macb ff0c0000.ethernet: gem-ptp-timer ptp clock unregistered.
[ 46.490097] macb ff0b0000.ethernet eth0: Link is Down
[ 46.495298] macb ff0b0000.ethernet: gem-ptp-timer ptp clock unregistered.
resume:
[ 46.633840] macb ff0b0000.ethernet eth0: configuring for phy/sgmii link mode
macb_mdio_read -> pm_runtime_get_sync(GEM1) it return -EACCES error.
The suspend/resume is dependent on probe order so to fix this dependency
ensure that MDIO producer ethernet node is always probed first followed
by MDIO consumer ethernet node.
During MDIO registration find out if MDIO bus is shared and check if MDIO
producer platform node(traverse by 'phy-handle' property) is bound. If not
bound then defer the MDIO consumer ethernet node probe. Doing it ensures
that in suspend/resume MDIO producer is resumed followed by MDIO consumer
ethernet node.
Signed-off-by: Radhey Shyam Pandey <[email protected]>
---
As an alternative to this defer probe approach i also explored using
devicelink framework in ndo_open and create a link between consumer and
producer and that solves suspend/resume issue but incase MDIO producer
probe fails MDIO consumer ethernet node remain non-functional. So a
simpler solution seem to defer MDIO consumer ethernet probe till all
dependencies are met.
---
drivers/net/ethernet/cadence/macb_main.c | 26 +++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index d0ea8dbfa213..5c903d566bc7 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -853,7 +853,8 @@ static int macb_mii_probe(struct net_device *dev)
static int macb_mdiobus_register(struct macb *bp)
{
- struct device_node *child, *np = bp->pdev->dev.of_node;
+ struct device_node *child, *np = bp->pdev->dev.of_node, *mdio_np, *dev_np;
+ struct platform_device *mdio_pdev;
/* If we have a child named mdio, probe it instead of looking for PHYs
* directly under the MAC node
@@ -884,6 +885,29 @@ static int macb_mdiobus_register(struct macb *bp)
return of_mdiobus_register(bp->mii_bus, np);
}
+ /* For shared MDIO usecases find out MDIO producer platform
+ * device node by traversing through phy-handle DT property.
+ */
+ np = of_parse_phandle(np, "phy-handle", 0);
+ mdio_np = of_get_parent(np);
+ of_node_put(np);
+ dev_np = of_get_parent(mdio_np);
+ of_node_put(mdio_np);
+ mdio_pdev = of_find_device_by_node(dev_np);
+ of_node_put(dev_np);
+
+ /* Check if the MDIO producer device is probed */
+ device_lock(&mdio_pdev->dev);
+ if (mdio_pdev && !device_is_bound(&mdio_pdev->dev)) {
+ device_unlock(&mdio_pdev->dev);
+ platform_device_put(mdio_pdev);
+ netdev_info(bp->dev, "Defer probe as mdio producer %s is not probed\n",
+ dev_name(&mdio_pdev->dev));
+ return -EPROBE_DEFER;
+ }
+
+ device_unlock(&mdio_pdev->dev);
+ platform_device_put(mdio_pdev);
return mdiobus_register(bp->mii_bus);
}
--
2.25.1
On Tue, 28 Jun 2022 23:38:54 +0530 Radhey Shyam Pandey wrote:
> In shared MDIO suspend/resume usecase for ex. with MDIO producer
> (0xff0c0000) eth1 and MDIO consumer(0xff0b0000) eth0 there is a
> constraint that ethernet interface(ff0c0000) MDIO bus producer
> has to be resumed before the consumer ethernet interface(ff0b0000).
ERROR: modpost: "device_is_bound" [drivers/net/ethernet/cadence/macb.ko] undefined!
make[2]: *** [../scripts/Makefile.modpost:128: modules-only.symvers] Error 1
make[1]: *** [/home/nipa/net-next/Makefile:1757: modules] Error 2
make: *** [Makefile:219: __sub-make] Error 2
> -----Original Message-----
> From: Jakub Kicinski <[email protected]>
> Sent: Wednesday, June 29, 2022 10:39 AM
> To: Pandey, Radhey Shyam <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; git (AMD-Xilinx)
> <[email protected]>; Katakam, Harini <[email protected]>
> Subject: Re: [PATCH net-next] net: macb: In shared MDIO usecase make MDIO
> producer ethernet node to probe first
>
> On Tue, 28 Jun 2022 23:38:54 +0530 Radhey Shyam Pandey wrote:
> > In shared MDIO suspend/resume usecase for ex. with MDIO producer
> > (0xff0c0000) eth1 and MDIO consumer(0xff0b0000) eth0 there is a
> > constraint that ethernet interface(ff0c0000) MDIO bus producer has to
> > be resumed before the consumer ethernet interface(ff0b0000).
>
> ERROR: modpost: "device_is_bound" [drivers/net/ethernet/cadence/macb.ko]
> undefined!
> make[2]: *** [../scripts/Makefile.modpost:128: modules-only.symvers] Error 1
> make[1]: *** [/home/nipa/net-next/Makefile:1757: modules] Error 2
> make: *** [Makefile:219: __sub-make] Error 2
Oh, I could also see this error when making it a module compilation.
I will fix it in v2. As an alternative to device_is_bound() i think we
can check the presence of drvdata. Similar approach i see it in ongoing
onboard_usb_hub driver series[1]. Does it look ok?/any other suggestions?
- if (mdio_pdev && !device_is_bound(&mdio_pdev->dev))
+ if (mdio_pdev && !dev_get_drvdata(&mdio_pdev->dev))
[1]: https://lore.kernel.org/all/20220622144857.v23.2.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid/
Listed in v21 changes.
Please let me know if there is any other feedback on the
shared MDIO dependency implementation.
Thanks,
Radhey
On Wed, 29 Jun 2022 15:24:55 +0000 Pandey, Radhey Shyam wrote:
> Oh, I could also see this error when making it a module compilation.
> I will fix it in v2. As an alternative to device_is_bound() i think we
> can check the presence of drvdata. Similar approach i see it in ongoing
> onboard_usb_hub driver series[1]. Does it look ok?/any other suggestions?
>
> - if (mdio_pdev && !device_is_bound(&mdio_pdev->dev))
> + if (mdio_pdev && !dev_get_drvdata(&mdio_pdev->dev))
>
> [1]: https://lore.kernel.org/all/20220622144857.v23.2.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid/
> Listed in v21 changes.
Yeah, no real opinion here. The entire patch looks odd but I lost track
of the devlink discussions. Could you CC Saravana, Greg, Rafael, etc
on the next version? And the Ethernet PHY maintainers.