Re: [EXT] Re: [PATCH v3 1/1] net: phylink: add phylink_set_mac_pm() helper

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Oct 10, 2022 at 02:54:24PM +0000, Shenwei Wang wrote:
> > -----Original Message-----
> > From: Russell King <linux@armlinux.org.uk>
> > Sent: Saturday, October 8, 2022 2:28 AM
> > To: Shenwei Wang <shenwei.wang@nxp.com>
> > Cc: Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit <hkallweit1@gmail.com>;
> > David S. Miller <davem@davemloft.net>; Eric Dumazet
> > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> > <pabeni@redhat.com>; netdev@vger.kernel.org; imx@lists.linux.dev
> > Subject: [EXT] Re: [PATCH v3 1/1] net: phylink: add phylink_set_mac_pm() helper
> > 
> > Caution: EXT Email
> > 
> > On Fri, Oct 07, 2022 at 10:42:46AM -0500, Shenwei Wang wrote:
> > > +/**
> > > + * phylink_set_mac_pm() - set phydev->mac_managed_pm to true
> > > + * @pl: a pointer to a &struct phylink returned from phylink_create()
> > > + *
> > > + * Set the phydev->mac_managed_pm, which is under the phylink
> > > +instance
> > > + * specified by @pl, to true. This is to indicate that the MAC driver
> > > +is
> > > + * responsible for PHY PM.
> > > + *
> > > + * The function can be called in the end of net_device_ops ndo_open()
> > > +method
> > > + * or any place after phy is connected.
> > 
> > May I suggest a different wording:
> > 
> > "If the driver wishes to use this feature, this function should be called each time
> > after the driver connects a PHY with phylink."
> > 
> 
> Your wording is much better. Will use it in the next version.
> 
> > This makes it clear that after one of:
> > 
> > phylink_connect_phy()
> > phylink_of_phy_connect()
> > phylink_fwnode_phy_connect()
> > 
> > has been called, and the driver wants to call this function, the driver needs to
> > call this every time just after the driver connects a PHY.
> > 
> > The alternative is that we store this information away when this function is
> > called, and always update the phydev when one is connected.
> > 
> > There is also the question whether this should also be applied to PHYs on SFP
> > modules or not. Should a network driver using mac managed PM, but also
> > supports SFPs, and a copper SFP is plugged in with an accessible PHY, what
> > should happen if the system goes into a low power state?
> > 
> 
> In theory, the SFP should be covered by this patch too. Since the resume flow is
> Controlled by the value of phydev->mac_managed_pm, it should work in the same
> way after the phydev is linked to the SFP phy instance.

It won't, because the MAC doesn't know when it needs to call your new
function.

Given this, I think a different approach is needed here:

1) require a MAC to call this function after phylink_create() and record
   the configuration in struct phylink, or put a configuration boolean in
   the phylink_config structure (probably better).

2) whenever any PHY is attached, check the status of this feature, and
   pass the configuration on to phylib.

That means MACs don't have to keep calling the function - they declare
early on whether they will be using MAC managed PM or not and then
they're done with that. Keeps it simple.

Russell.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!