[REGRESSION,RFC] net: phy: don't resume PHY via MDIO when iface is not up

  TLDR; Commit 96fb2077a517 ("net: phy: consider that suspend2ram may cut
off PHY power") caused regressions for us when resuming an interface
which is not up. It turns out the problem is another one, the above
commit only makes it visible. The attached patch is probably not the
right fix, but at least is proving my assumptions AFAICS.

Setup: I used Renesas boards for my tests, namely Salvator-XS and Ebisu.
They both use RAVB driver (drivers/net/ethernet/renesas/ravb_main.c) and
a Micrel KSZ9031 PHY (drivers/net/phy/micrel.c). I think the problems
are generic, though.

Long text: After the above commit, we could see various resume failures
on our boards, like timeouts when resetting the MDIO bus, or warning
about skew values in non-RGMII mode, although RGMII was used. All of
these happened, because phy_init_hw() was now called in
mdio_bus_phy_resume() which wasn't the case before. But the interface
was not up yet, e.g. phydev->interface was still the default and not
RGMII, so the initialization didn't work properly. phy_attach_direct()
pays attention to this:

1504         /* Do initial configuration here, now that
1505          * we have certain key parameters
1506          * (dev_flags and interface)
1507          */
1508         err = phy_init_hw(phydev);

But phy_init_hw() doesn't if the interface is not up, AFAICS.

This may be a problem in itself, but I then wondered why
mdio_bus_phy_resume() gets called anyhow because the RAVB driver sets
'phydev->mac_managed_pm = true' so once the interface is up
mdio_bus_phy_resume() never gets called. But again, the interface was
not up yet, so mac_managed_pm was not set yet.

So, in my quest to avoid mdio_bus_phy_resume() being called, I tried
this patch declaring the PHY being in suspend state when being probed.
The KSZ9031 has a soft_reset() callback, so phy_init_hw() will reset the
suspended flag when the PHY is attached. It works for me(tm),
suspend/resume now works independently of the interface being up or not.

I don't think this is the proper solution, though. It will e.g. fail if
some PHY is not using the soft_reset() callback. And I am missing the
experience in this subsystem to decide if we can clear the resume flag
in phy_init_hw() unconditionally. My gut feeling is that we can't.

So, this patch mostly demonstrates the issues we have and the things I
found out. I'd be happy if someone could point me to a proper solution,
or more information that I am missing here. Thank you in advance and
happy hacking!

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/net/phy/phy_device.c | 1 +
 1 file changed, 1 insertion(+)
  

Hi Wolfram,

Thanks for your report!

On Thu, Feb 23, 2023 at 8:36 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> TLDR; Commit 96fb2077a517 ("net: phy: consider that suspend2ram may cut
> off PHY power") caused regressions for us when resuming an interface

That is actually an LTS commit.  Upstream is commit 4c0d2e96ba055bd8
("net: phy: consider that suspend2ram may cut off PHY power") in
v5.12-rc1.

Gr{oetje,eeting}s,

                        Geert
  

Hi Geert,

> > TLDR; Commit 96fb2077a517 ("net: phy: consider that suspend2ram may cut
> > off PHY power") caused regressions for us when resuming an interface
> 
> That is actually an LTS commit.  Upstream is commit 4c0d2e96ba055bd8
> ("net: phy: consider that suspend2ram may cut off PHY power") in
> v5.12-rc1.

Oh, thank you for correcting me!

All the best,

   Wolfram
  

On 23.02.2023 08:05, Wolfram Sang wrote:
> TLDR; Commit 96fb2077a517 ("net: phy: consider that suspend2ram may cut
> off PHY power") caused regressions for us when resuming an interface
> which is not up. It turns out the problem is another one, the above
> commit only makes it visible. The attached patch is probably not the
> right fix, but at least is proving my assumptions AFAICS.
> 
> Setup: I used Renesas boards for my tests, namely Salvator-XS and Ebisu.
> They both use RAVB driver (drivers/net/ethernet/renesas/ravb_main.c) and
> a Micrel KSZ9031 PHY (drivers/net/phy/micrel.c). I think the problems
> are generic, though.
> 
> Long text: After the above commit, we could see various resume failures
> on our boards, like timeouts when resetting the MDIO bus, or warning
> about skew values in non-RGMII mode, although RGMII was used. All of
> these happened, because phy_init_hw() was now called in
> mdio_bus_phy_resume() which wasn't the case before. But the interface
> was not up yet, e.g. phydev->interface was still the default and not
> RGMII, so the initialization didn't work properly. phy_attach_direct()
> pays attention to this:
> 
> 1504         /* Do initial configuration here, now that
> 1505          * we have certain key parameters
> 1506          * (dev_flags and interface)
> 1507          */
> 1508         err = phy_init_hw(phydev);
> 
> But phy_init_hw() doesn't if the interface is not up, AFAICS.
> 
> This may be a problem in itself, but I then wondered why
> mdio_bus_phy_resume() gets called anyhow because the RAVB driver sets
> 'phydev->mac_managed_pm = true' so once the interface is up
> mdio_bus_phy_resume() never gets called. But again, the interface was
> not up yet, so mac_managed_pm was not set yet.
> 
Setting phydev->mac_managed_pm in the open() callback is too late.
It should be set as soon as the phydev is created. That's in
ravb_mdio_init() after the call to of_mdiobus_register().

It should be possible to get the phydev with:
pn = of_parse_phandle(np, "phy-handle", 0);
phy = of_phy_find_device(pn);


> So, in my quest to avoid mdio_bus_phy_resume() being called, I tried
> this patch declaring the PHY being in suspend state when being probed.
> The KSZ9031 has a soft_reset() callback, so phy_init_hw() will reset the
> suspended flag when the PHY is attached. It works for me(tm),
> suspend/resume now works independently of the interface being up or not.
> 
> I don't think this is the proper solution, though. It will e.g. fail if
> some PHY is not using the soft_reset() callback. And I am missing the
> experience in this subsystem to decide if we can clear the resume flag
> in phy_init_hw() unconditionally. My gut feeling is that we can't.
> 
> So, this patch mostly demonstrates the issues we have and the things I
> found out. I'd be happy if someone could point me to a proper solution,
> or more information that I am missing here. Thank you in advance and
> happy hacking!
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/net/phy/phy_device.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 8cff61dbc4b5..5cbb471700a8 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3108,6 +3108,7 @@ static int phy_probe(struct device *dev)
>  
>  	/* Set the state to READY by default */
>  	phydev->state = PHY_READY;
> +	phydev->suspended = 1;
>  
>  out:
>  	/* Assert the reset signal */
  

Hello Heiner,

> > This may be a problem in itself, but I then wondered why
> > mdio_bus_phy_resume() gets called anyhow because the RAVB driver sets
> > 'phydev->mac_managed_pm = true' so once the interface is up
> > mdio_bus_phy_resume() never gets called. But again, the interface was
> > not up yet, so mac_managed_pm was not set yet.
> > 
> Setting phydev->mac_managed_pm in the open() callback is too late.
> It should be set as soon as the phydev is created. That's in
> ravb_mdio_init() after the call to of_mdiobus_register().
> 
> It should be possible to get the phydev with:
> pn = of_parse_phandle(np, "phy-handle", 0);
> phy = of_phy_find_device(pn);

Awesome, thank you very much for the pointer. I applied setting
'mac_managed_pm' at probe time, and now I can resume successfully.
Sadly, this is only the first part of the problem. I still can't get the
interface up after resuming, but I still need to debug this further.
At least, the problem with mdiobus_resume getting called is fixed now.

Thank you for your help!

   Wolfram