* Re: [RFC net 1/1] net: stmmac: skip PHY scanning when PHY already attached in DT mode
2023-04-04 9:14 [RFC net 1/1] net: stmmac: skip PHY scanning when PHY already attached in DT mode Michael Sit Wei Hong
[not found] ` <CGME20230404165447eucas1p114076509978a487acfe8312e2e58ecca@eucas1p1.samsung.com>
@ 2023-04-04 17:21 ` Russell King (Oracle)
2023-04-04 21:18 ` Martin Blumenstingl
2 siblings, 0 replies; 4+ messages in thread
From: Russell King (Oracle) @ 2023-04-04 17:21 UTC (permalink / raw)
To: Michael Sit Wei Hong
Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Maxime Coquelin, Ong Boon Leong, netdev, linux-stm32,
linux-arm-kernel, linux-kernel, hkallweit1, andrew,
Martin Blumenstingl, Looi Hong Aun, Voon Weifeng,
Lai Peter Jun Ann, Zulkifli Muhammad Husaini, Tan Tee Min,
hock.leong.kweh
On Tue, Apr 04, 2023 at 05:14:42PM +0800, Michael Sit Wei Hong wrote:
> If PHY is successfully attached during phylink_fwnode_phy_connect()
> in DT mode. MAC should not need to scan for PHY again.
>
> Adding a logic to check if ovr_an_inband is set before scanning for
> a PHY, since phylink_fwnode_phy_connect() returns 0 when
>
> phy_fwnode = fwnode_get_phy_node(fwnode);
> if (IS_ERR(phy_fwnode)) {
> if (pl->cfg_link_an_mode == MLO_AN_PHY)
> return -ENODEV;
> return 0;
> }
>
> Fixes: fe2cfbc96803 ("net: stmmac: check if MAC needs to attach to a PHY")
> Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index d41a5f92aee7..4b8d3d975678 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1149,7 +1149,7 @@ static int stmmac_init_phy(struct net_device *dev)
> /* Some DT bindings do not set-up the PHY handle. Let's try to
> * manually parse it
> */
> - if (!fwnode || phy_needed || ret) {
> + if (!fwnode || (phy_needed && priv->phylink_config.ovr_an_inband) || ret) {
> int addr = priv->plat->phy_addr;
> struct phy_device *phydev;
>
Sorry, but this just doesn't look right to me. And Gnrrrrr, I wish I'd
spotted this stupidity during the review of phylink_expects_phy().
phy_needed will be true if phylink thinks there should be a PHY on the
link, that being:
MLO_AN_PHY mode
MLO_AN_INBAND mode and non-802.3z interface mode
If !phy_needed, then the code should not be attempting to attach a PHY,
but calling phylink_fwnode_phy_connect() is fine as it will just return
zero.
If phy_needed is true, then phylink_fwnode_phy_connect() will check to
see whether a PHY is in the fwnode. If we fail to find a PHY, then if
we're in MLO_AN_PHY mode, that's an error, and we return -ENODEV. If
there is no PHY device associated with the handle, we also return
-ENODEV.
If phy_needed is true, and phylink_fwnode_phy_connect() doesn't find
a PHY in the fwnode, and we're in MLO_AN_INBAND mode (e.g. for SGMII)
then we'll return zero, because we can cope without a PHY in this
instance - it's a success. If we do find a PHY, then we will make use
of it, and also return zero.
The problem is this hacky code wants to know the difference between
those two situations, but phylink doesn't allow you to, and I don't
think now that phylink_expects_phy() solves that problem.
I think you're better off doing this:
struct fwnode_handle *phy_fwnode;
if (!phylink_expects_phy(priv->phylink))
return 0;
fwnode = of_fwnode_handle(priv->plat->phylink_node);
if (!fwnode)
fwnode = dev_fwnode(priv->device);
if (fwnode)
phy_fwnode = fwnode_get_phy_node(fwnode);
else
phy_fwnode = NULL;
if (!phy_fwnode) {
... do non-DT PHY stuff ...
ret = phylink_connect_phy(priv->phylink, phydev);
} else {
fwnode_handle_put(phy_fwnode);
ret = phylink_fwnode_phy_connect(priv->phylink, fwnode, 0);
}
... ethtool wol stuff ...
Doesn't that more closely reflect what you actually want this code
to be doing, rather than messing about trying to guess it from
phylink's return code etc?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [RFC net 1/1] net: stmmac: skip PHY scanning when PHY already attached in DT mode
2023-04-04 9:14 [RFC net 1/1] net: stmmac: skip PHY scanning when PHY already attached in DT mode Michael Sit Wei Hong
[not found] ` <CGME20230404165447eucas1p114076509978a487acfe8312e2e58ecca@eucas1p1.samsung.com>
2023-04-04 17:21 ` Russell King (Oracle)
@ 2023-04-04 21:18 ` Martin Blumenstingl
2 siblings, 0 replies; 4+ messages in thread
From: Martin Blumenstingl @ 2023-04-04 21:18 UTC (permalink / raw)
To: Michael Sit Wei Hong
Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Maxime Coquelin, Ong Boon Leong, netdev, linux-stm32,
linux-arm-kernel, linux-kernel, linux, hkallweit1, andrew,
Looi Hong Aun, Voon Weifeng, Lai Peter Jun Ann,
Zulkifli Muhammad Husaini, Tan Tee Min, hock.leong.kweh
On Tue, Apr 4, 2023 at 11:15 AM Michael Sit Wei Hong
<michael.wei.hong.sit@intel.com> wrote:
>
> If PHY is successfully attached during phylink_fwnode_phy_connect()
> in DT mode. MAC should not need to scan for PHY again.
>
> Adding a logic to check if ovr_an_inband is set before scanning for
> a PHY, since phylink_fwnode_phy_connect() returns 0 when
>
> phy_fwnode = fwnode_get_phy_node(fwnode);
> if (IS_ERR(phy_fwnode)) {
> if (pl->cfg_link_an_mode == MLO_AN_PHY)
> return -ENODEV;
> return 0;
> }
>
> Fixes: fe2cfbc96803 ("net: stmmac: check if MAC needs to attach to a PHY")
> Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>
Thank you for working on this! Your patch fixes the issue I reported,
Ethernet is back on my X96 Air so this patch is:
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
My understanding is that Russell King is asking for a second iteration
of this patch with his feedback incorporated. I'm happy to test this
second version as well if you keep me Cc'ed.
For this second version you can also (unconditionally) add:
Reported-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Best regards,
Martin
^ permalink raw reply [flat|nested] 4+ messages in thread