diff mbox series

[RFC,net-next] net: phy: Don't suspend/resume device not in use

Message ID AM9PR04MB8506772CFCC05CE71C383A6AE2202@AM9PR04MB8506.eurprd04.prod.outlook.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series [RFC,net-next] net: phy: Don't suspend/resume device not in use | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/apply fail Patch does not apply to net-next-1

Commit Message

Jan Petrous March 7, 2024, 3:45 p.m. UTC 
In the case when an MDIO bus contains PHY device not attached to
the any netdev or is attached to the external netdev, controlled
by another driver and the driver is disabled, the bus, when PM suspend
occurs, is trying to suspend/resume also the unattached phydev.

/* Synopsys DWMAC built-in driver (stmmac) */
gmac0: ethernet@4033c000 {
        compatible = "snps,dwc-qos-ethernet", "nxp,s32cc-dwmac";

        phy-handle = <&gmac0_mdio_c_phy4>;
        phy-mode = "rgmii-id";

        gmac0_mdio: mdio@0 {
                compatible = "snps,dwmac-mdio";

                /* AQR107 attached to pfe_netif1 */
                gmac0_mdio_c_phy1: ethernet-phy@1 {
                        compatible = "ethernet-phy-ieee802.3-c45";
                        reg = <1>;
                };

                /* KSZ9031RNX attached to gmac0 */
                gmac0_mdio_c_phy4: ethernet-phy@4 {
                        reg = <4>;
                };
        };
};

/* PFE controller, loadable driver pfeng.ko */
pfe: pfe@46000000 {
        compatible = "nxp,s32g-pfe";

        /* Network interface 'pfe1' */
        pfe_netif1: ethernet@11 {
                compatible = "nxp,s32g-pfe-netif";

                phy-mode = "sgmii";
                phy-handle = <&gmac0_mdio_c_phy1>;
        };
};

Because such device didn't go through attach process, internal
parameters like phy_dev->interface is set to the default value, which
is not correct for some drivers. Ie. Aquantia PHY AQR107 doesn't
support PHY_INTERFACE_MODE_GMII and trying to use phy_init_hw()
in mdio_bus_phy_resume() ends up with the following error caused
by initial check of supported interfaces in aqr107_config_init():

[   63.927708] Aquantia AQR113C stmmac-0:08: PM: failed to resume: error -19']

Signed-off-by: Jan Petrous <jan.petrous@oss.nxp.com>
---
 drivers/net/phy/phy_device.c | 8 ++++++++
 1 file changed, 8 insertions(+)

--
2.43.0

Comments

Andrew Lunn March 7, 2024, 4:22 p.m. UTC | #1 
> Because such device didn't go through attach process, internal
> parameters like phy_dev->interface is set to the default value, which
> is not correct for some drivers. Ie. Aquantia PHY AQR107 doesn't
> support PHY_INTERFACE_MODE_GMII and trying to use phy_init_hw()
> in mdio_bus_phy_resume() ends up with the following error caused
> by initial check of supported interfaces in aqr107_config_init():
> 
> [   63.927708] Aquantia AQR113C stmmac-0:08: PM: failed to resume: error -19']
> 
> Signed-off-by: Jan Petrous <jan.petrous@oss.nxp.com>
> ---
>  drivers/net/phy/phy_device.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 3611ea64875e..30c03ac6b84c 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -311,6 +311,10 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
>  {
>         struct phy_device *phydev = to_phy_device(dev);
> 
> +       /* Don't suspend device if not in use state */
> +       if (phydev->state <= PHY_READY)
> +               return 0;
> +
>         if (phydev->mac_managed_pm)
>                 return 0;

If nothing is using it, suspending it does however make sense. It is
consuming power, which could be saved by suspending it. It makes the
code asymmetrical, but i would throw this hunk away.

> 
> @@ -344,6 +348,10 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
>         struct phy_device *phydev = to_phy_device(dev);
>         int ret;
> 
> +       /* Don't resume device which wasn't previously in use state */
> +       if (phydev->state <= PHY_READY)
> +               return 0;
> +

This is the real fix to your problem. phy_attach_direct() also does a
phy_resume(), so if the device does come along and claim the PHY after
the resume, it looks like this should work, without the matching
suspend.

	Andrew
Jan Petrous March 7, 2024, 4:44 p.m. UTC | #2 
> 
> > Because such device didn't go through attach process, internal
> > parameters like phy_dev->interface is set to the default value, which
> > is not correct for some drivers. Ie. Aquantia PHY AQR107 doesn't
> > support PHY_INTERFACE_MODE_GMII and trying to use phy_init_hw()
> > in mdio_bus_phy_resume() ends up with the following error caused
> > by initial check of supported interfaces in aqr107_config_init():
> >
> > [   63.927708] Aquantia AQR113C stmmac-0:08: PM: failed to resume: error -
> 19']
> >
> > Signed-off-by: Jan Petrous <jan.petrous@oss.nxp.com>
> > ---
> >  drivers/net/phy/phy_device.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 3611ea64875e..30c03ac6b84c 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -311,6 +311,10 @@ static __maybe_unused int
> mdio_bus_phy_suspend(struct device *dev)
> >  {
> >         struct phy_device *phydev = to_phy_device(dev);
> >
> > +       /* Don't suspend device if not in use state */
> > +       if (phydev->state <= PHY_READY)
> > +               return 0;
> > +
> >         if (phydev->mac_managed_pm)
> >                 return 0;
> 
> If nothing is using it, suspending it does however make sense. It is
> consuming power, which could be saved by suspending it. It makes the
> code asymmetrical, but i would throw this hunk away.

Yes, I also thought about still having possibility to suspend it, even not used.
I'm ok with removal the hunk.

> 
> >
> > @@ -344,6 +348,10 @@ static __maybe_unused int
> mdio_bus_phy_resume(struct device *dev)
> >         struct phy_device *phydev = to_phy_device(dev);
> >         int ret;
> >
> > +       /* Don't resume device which wasn't previously in use state */
> > +       if (phydev->state <= PHY_READY)
> > +               return 0;
> > +
> 
> This is the real fix to your problem. phy_attach_direct() also does a
> phy_resume(), so if the device does come along and claim the PHY after
> the resume, it looks like this should work, without the matching
> suspend.

Well, I like symmetry, this was the reason I checked both PM directions.
But you are right, it works for me with resume hunk only.

Thanks.
/Jan
diff mbox series

Patch

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3611ea64875e..30c03ac6b84c 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -311,6 +311,10 @@  static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
 {
        struct phy_device *phydev = to_phy_device(dev);

+       /* Don't suspend device if not in use state */
+       if (phydev->state <= PHY_READY)
+               return 0;
+
        if (phydev->mac_managed_pm)
                return 0;

@@ -344,6 +348,10 @@  static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
        struct phy_device *phydev = to_phy_device(dev);
        int ret;

+       /* Don't resume device which wasn't previously in use state */
+       if (phydev->state <= PHY_READY)
+               return 0;
+
        if (phydev->mac_managed_pm)
                return 0;