[v3,1/1] net: phylink: add phylink_set_mac_pm() helper

Commit Message

Shenwei Wang Oct. 7, 2022, 3:42 p.m. UTC

The recent commit

'commit 47ac7b2f6a1f ("net: phy: Warn about incorrect
mdio_bus_phy_resume() state")'

requires the MAC driver explicitly tell the phy driver who is
managing the PM, otherwise you will see warning during resume
stage.

Add a helper to let the MAC driver has a way to tell the PHY
driver it will manage the PM.

Signed-off-by: Shenwei Wang <[e-mail]>
---
 Changes in v3:
  - update the API description according to Russell's feedback.

 Changes in v2:
  - add the API description
  - remove the unneccesary ASSERT_RTNL();

 drivers/net/phy/phylink.c | 18 ++++++++++++++++++
 include/linux/phylink.h   |  1 +
 2 files changed, 19 insertions(+)

--
2.34.1

Comments

Florian Fainelli Oct. 7, 2022, 9:25 p.m. UTC | #1

On 10/7/22 08:42, Shenwei Wang wrote:
> The recent commit
> 
> 'commit 47ac7b2f6a1f ("net: phy: Warn about incorrect
> mdio_bus_phy_resume() state")'
> 
> requires the MAC driver explicitly tell the phy driver who is
> managing the PM, otherwise you will see warning during resume
> stage.
> 
> Add a helper to let the MAC driver has a way to tell the PHY
> driver it will manage the PM.
> 
> Signed-off-by: Shenwei Wang <[e-mail]>

This is fine and needed, but first net-next tree is currently closed, 
and second, you need to submit the user of that helper function in the 
same patch series.

Russell King (Oracle) Oct. 8, 2022, 7:27 a.m. UTC | #2

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."

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?

Shenwei Wang Oct. 10, 2022, 2:54 p.m. UTC | #3

> -----Original Message-----
> From: Florian Fainelli <[e-mail]>
> Sent: Friday, October 7, 2022 4:26 PM
> To: Shenwei Wang <[e-mail]>; Russell King
> <[e-mail]>; Andrew Lunn <[e-mail]>; Heiner Kallweit
> <[e-mail]>; David S. Miller <[e-mail]>; Eric
> Dumazet <[e-mail]>; Jakub Kicinski <[e-mail]>; Paolo
> Abeni <[e-mail]>
> Cc: [e-mail]; [e-mail]
> Subject: [EXT] Re: [PATCH v3 1/1] net: phylink: add phylink_set_mac_pm() helper
> 
> Caution: EXT Email
> 
> On 10/7/22 08:42, Shenwei Wang wrote:
> > The recent commit
> >
> > 'commit 47ac7b2f6a1f ("net: phy: Warn about incorrect
> > mdio_bus_phy_resume() state")'
> >
> > requires the MAC driver explicitly tell the phy driver who is managing
> > the PM, otherwise you will see warning during resume stage.
> >
> > Add a helper to let the MAC driver has a way to tell the PHY driver it
> > will manage the PM.
> >
> > Signed-off-by: Shenwei Wang <[e-mail]>
> 
> This is fine and needed, but first net-next tree is currently closed, and second,
> you need to submit the user of that helper function in the same patch series.

Thank you for the review. When should I repost the patches?

Regards,
Shenwei

> --
> Florian

Shenwei Wang Oct. 10, 2022, 2:54 p.m. UTC | #4

> -----Original Message-----
> From: Russell King <[e-mail]>
> Sent: Saturday, October 8, 2022 2:28 AM
> To: Shenwei Wang <[e-mail]>
> Cc: Andrew Lunn <[e-mail]>; Heiner Kallweit <[e-mail]>;
> David S. Miller <[e-mail]>; Eric Dumazet
> <[e-mail]>; Jakub Kicinski <[e-mail]>; Paolo Abeni
> <[e-mail]>; [e-mail]; [e-mail]
> 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.

Regards,
Shenwei

> --
> RMK's Patch system:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ar
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&amp;data=05%7C01%7Cshenwei.
> wang%40nxp.com%7Cedf38f379deb4eda9ccb08daa8fe995a%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C0%7C638008108695053955%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=DiHVcXkqri4qtbsp7BwR6kPhW
> GqLzr%2BVf4tj9JXPzoQ%3D&amp;reserved=0
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Russell King (Oracle) Oct. 10, 2022, 3:22 p.m. UTC | #5

On Mon, Oct 10, 2022 at 02:54:24PM +0000, Shenwei Wang wrote:
> > -----Original Message-----
> > From: Russell King <[e-mail]>
> > Sent: Saturday, October 8, 2022 2:28 AM
> > To: Shenwei Wang <[e-mail]>
> > Cc: Andrew Lunn <[e-mail]>; Heiner Kallweit <[e-mail]>;
> > David S. Miller <[e-mail]>; Eric Dumazet
> > <[e-mail]>; Jakub Kicinski <[e-mail]>; Paolo Abeni
> > <[e-mail]>; [e-mail]; [e-mail]
> > 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.

Shenwei Wang Oct. 10, 2022, 4:11 p.m. UTC | #6

> -----Original Message-----
> From: Russell King <[e-mail]>
> Sent: Monday, October 10, 2022 10:23 AM
> To: Shenwei Wang <[e-mail]>
> Cc: Andrew Lunn <[e-mail]>; Heiner Kallweit <[e-mail]>;
> David S. Miller <[e-mail]>; Eric Dumazet
> > > 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).
> 

I prefer to use the function call because it is simple to implement and is easy to use.

> 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.
> 

Agree. Let me implement this idea in the next version.

Thanks,
Shenwei

> Russell.
> 
> --
> RMK's Patch system:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ar
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&amp;data=05%7C01%7Cshenwei.
> wang%40nxp.com%7C2d5aa021cfa74162a1e008daaad350cc%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C0%7C638010121814590052%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=vEHqzO3CSdtvSuaW80%2FaBK
> sDp895q7leiz1w%2BZNmGCU%3D&amp;reserved=0
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Russell King (Oracle) Oct. 10, 2022, 4:21 p.m. UTC | #7

On Mon, Oct 10, 2022 at 04:11:36PM +0000, Shenwei Wang wrote:
> 
> 
> > -----Original Message-----
> > From: Russell King <[e-mail]>
> > Sent: Monday, October 10, 2022 10:23 AM
> > To: Shenwei Wang <[e-mail]>
> > Cc: Andrew Lunn <[e-mail]>; Heiner Kallweit <[e-mail]>;
> > David S. Miller <[e-mail]>; Eric Dumazet
> > > > 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).
> > 
> 
> I prefer to use the function call because it is simple to implement and is easy to use.

	blah->phylink_config.mac_managed_pm = true;

in the appropriate drivers before they call phylink_create() would be
difficult to use?

Given that we use this method to configure the MAC speeds and phy
interface modes already, I'm not sure why we'd want some other approach
for this.

Shenwei Wang Oct. 10, 2022, 4:30 p.m. UTC | #8

> -----Original Message-----
> From: Russell King <[e-mail]>
> Sent: Monday, October 10, 2022 11:22 AM
> To: Shenwei Wang <[e-mail]>
> Cc: Andrew Lunn <[e-mail]>; Heiner Kallweit <[e-mail]>;
> David S. Miller <[e-mail]>; Eric Dumazet
> <[e-mail]>; Jakub Kicinski <[e-mail]>; Paolo Abeni
> <[e-mail]>; [e-mail]; [e-mail]
> Subject: Re: [EXT] Re: [PATCH v3 1/1] net: phylink: add phylink_set_mac_pm()
> helper
> 
> Caution: EXT Email
> 
> On Mon, Oct 10, 2022 at 04:11:36PM +0000, Shenwei Wang wrote:
> >
> >
> > > -----Original Message-----
> > > From: Russell King <[e-mail]>
> > > Sent: Monday, October 10, 2022 10:23 AM
> > > To: Shenwei Wang <[e-mail]>
> > > Cc: Andrew Lunn <[e-mail]>; Heiner Kallweit
> > > <[e-mail]>; David S. Miller <[e-mail]>; Eric
> > > Dumazet
> > > > > 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).
> > >
> >
> > I prefer to use the function call because it is simple to implement and is easy to
> use.
> 
>         blah->phylink_config.mac_managed_pm = true;
> 
> in the appropriate drivers before they call phylink_create() would be difficult to
> use?
> 
> Given that we use this method to configure the MAC speeds and phy interface
> modes already, I'm not sure why we'd want some other approach for this.
> 

Okay. Let's go in the config direction.

Regards,
Shenwei

> --
> RMK's Patch system:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ar
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&amp;data=05%7C01%7Cshenwei.
> wang%40nxp.com%7C82e5e5068b23483f015008daaadb86de%7C686ea1d3bc2
> b4c6fa92cd99c5c301635%7C0%7C0%7C638010157080134477%7CUnknown%7
> CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJ
> XVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=rpoL9UwtjVEQp2lWkm%2FVj
> kp99Romx%2BARfj%2FFmdpsO%2BY%3D&amp;reserved=0
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Patch

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index e9d62f9598f9..e78d9aef7a7c 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1722,6 +1722,24 @@  void phylink_stop(struct phylink *pl)
 }
 EXPORT_SYMBOL_GPL(phylink_stop);

+/**
+ * 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.
+ */
+void phylink_set_mac_pm(struct phylink *pl)
+{
+	if (pl->phydev)
+		pl->phydev->mac_managed_pm = true;
+}
+EXPORT_SYMBOL_GPL(phylink_set_mac_pm);
+
 /**
  * phylink_suspend() - handle a network device suspend event
  * @pl: a pointer to a &struct phylink returned from phylink_create()
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 6d06896fc20d..cfcc680462b9 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -540,6 +540,7 @@  void phylink_mac_change(struct phylink *, bool up);

 void phylink_start(struct phylink *);
 void phylink_stop(struct phylink *);
+void phylink_set_mac_pm(struct phylink *pl);

 void phylink_suspend(struct phylink *pl, bool mac_wol);
 void phylink_resume(struct phylink *pl);