Re: [PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink
On Wed, May 13, 2020 at 04:16:04PM +0200, Nicolas Ferre wrote:
> Russell,
>
> Thanks for the feedback.
>
> On 13/05/2020 at 15:05, Russell King - ARM Linux admin wrote:
> > On Wed, May 06, 2020 at 01:37:39PM +0200, nicolas.fe...@microchip.com wrote:
> > > From: Nicolas Ferre
> > >
> > > Keep previous function goals and integrate phylink actions to them.
> > >
> > > phylink_ethtool_get_wol() is not enough to figure out if Ethernet driver
> > > supports Wake-on-Lan.
> > > Initialization of "supported" and "wolopts" members is done in phylink
> > > function, no need to keep them in calling function.
> > >
> > > phylink_ethtool_set_wol() return value is not enough to determine
> > > if WoL is enabled for the calling Ethernet driver. Call it first
> > > but don't rely on its return value as most of simple PHY drivers
> > > don't implement a set_wol() function.
> > >
> > > Fixes: 7897b071ac3b ("net: macb: convert to phylink")
> > > Signed-off-by: Nicolas Ferre
> > > Reviewed-by: Florian Fainelli
> > > Cc: Claudiu Beznea
> > > Cc: Harini Katakam
> > > Cc: Antoine Tenart
> > > ---
> > > drivers/net/ethernet/cadence/macb_main.c | 18 ++
> > > 1 file changed, 10 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/cadence/macb_main.c
> > > b/drivers/net/ethernet/cadence/macb_main.c
> > > index 53e81ab048ae..24c044dc7fa0 100644
> > > --- a/drivers/net/ethernet/cadence/macb_main.c
> > > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > > @@ -2817,21 +2817,23 @@ static void macb_get_wol(struct net_device
> > > *netdev, struct ethtool_wolinfo *wol)
> > > {
> > >struct macb *bp = netdev_priv(netdev);
> > >
> > > - wol->supported = 0;
> > > - wol->wolopts = 0;
> > > -
> > > - if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET)
> > > + if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) {
> > >phylink_ethtool_get_wol(bp->phylink, wol);
> > > + wol->supported |= WAKE_MAGIC;
> > > +
> > > + if (bp->wol & MACB_WOL_ENABLED)
> > > + wol->wolopts |= WAKE_MAGIC;
> > > + }
> > > }
> > >
> > > static int macb_set_wol(struct net_device *netdev, struct
> > > ethtool_wolinfo *wol)
> > > {
> > >struct macb *bp = netdev_priv(netdev);
> > > - int ret;
> > >
> > > - ret = phylink_ethtool_set_wol(bp->phylink, wol);
> > > - if (!ret)
> > > - return 0;
> > > + /* Pass the order to phylink layer.
> > > + * Don't test return value as set_wol() is often not supported.
> > > + */
> > > + phylink_ethtool_set_wol(bp->phylink, wol);
> >
> > If this returns an error, does that mean WOL works or does it not?
>
> In my use case (simple phy: "Micrel KSZ8081"), if I have the error
> "-EOPNOTSUPP", it simply means that this phy driver doesn't have the
> set_wol() function. But on the MAC side, I can perfectly wake-up on WoL
> event as the phy acts as a pass-through.
>
> > Note that if set_wol() is not supported, this will return -EOPNOTSUPP.
> > What about other errors?
>
> True, I don't manage them. But for now this patch is a fix that only reverts
> to previous behavior. In other terms, it only fixes the regression.
>
> But can I make the difference, and how, between?
> 1/ the phy doesn't support WoL and could prevent the WoL to happen on the
> MAC
> 2/ the phy doesn't implement (yet) the set_wol() function, if MAC can
> manage, it's fine
I think you need to read and understand the code, but don't worry, I'll
do it for you. There are not that many implementations in phylib, so
it doesn't take long:
m88e1318_set_wol(), dp83867_set_wol(), dp83822_set_wol(),
at803x_set_wol(), lan88xx_set_wol(), and vsc85xx_wol_set().
For case 2, phylib returns -EOPNOTSUPP.
m88e1318_set_wol() returns zero on success, or propagates an error from
the MDIO bus accessors.
dp83867_set_wol() returns zero on success, or -EINVAL if the MAC address
is invalid. No bus errors are propagated.
dp83822_set_wol() is the same as dp83867_set_wol().
at803x_set_wol() returns zero on success, or -ENODEV if there is no
netdev attached (which means you shouldn't be calling this anyway),
-EINVAL if the MAC address is invalid, or sometimes propagates an
error from the MDIO bus accessors.
lan88xx_set_wol() always returns zero, but the function does nothing
other than saving the requested state, and uses that to avoid calling
genphy_suspend() for this PHY.
vsc85xx_wol_set() returns zero on success, or propagates an error from
the MDIO bus accessors.
So, what we can tell from the return code is:
- If it returned zero, the PHY likely supports and properly configured
WoL, and you may not need to configure the MAC (depends on whether
the PHY can wake the system up on its own.)
- If it returns -EOPNOTSUPP, there is no support for WoL at the PHY,
and you need to program your MAC - assuming that the PHY is going to
stay alive.
- If it returns some other error code, there was a failure of some sort
Re: [PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink
Russell,
Thanks for the feedback.
On 13/05/2020 at 15:05, Russell King - ARM Linux admin wrote:
On Wed, May 06, 2020 at 01:37:39PM +0200, nicolas.fe...@microchip.com wrote:
From: Nicolas Ferre
Keep previous function goals and integrate phylink actions to them.
phylink_ethtool_get_wol() is not enough to figure out if Ethernet driver
supports Wake-on-Lan.
Initialization of "supported" and "wolopts" members is done in phylink
function, no need to keep them in calling function.
phylink_ethtool_set_wol() return value is not enough to determine
if WoL is enabled for the calling Ethernet driver. Call it first
but don't rely on its return value as most of simple PHY drivers
don't implement a set_wol() function.
Fixes: 7897b071ac3b ("net: macb: convert to phylink")
Signed-off-by: Nicolas Ferre
Reviewed-by: Florian Fainelli
Cc: Claudiu Beznea
Cc: Harini Katakam
Cc: Antoine Tenart
---
drivers/net/ethernet/cadence/macb_main.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c
b/drivers/net/ethernet/cadence/macb_main.c
index 53e81ab048ae..24c044dc7fa0 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2817,21 +2817,23 @@ static void macb_get_wol(struct net_device *netdev,
struct ethtool_wolinfo *wol)
{
struct macb *bp = netdev_priv(netdev);
- wol->supported = 0;
- wol->wolopts = 0;
-
- if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET)
+ if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) {
phylink_ethtool_get_wol(bp->phylink, wol);
+ wol->supported |= WAKE_MAGIC;
+
+ if (bp->wol & MACB_WOL_ENABLED)
+ wol->wolopts |= WAKE_MAGIC;
+ }
}
static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo
*wol)
{
struct macb *bp = netdev_priv(netdev);
- int ret;
- ret = phylink_ethtool_set_wol(bp->phylink, wol);
- if (!ret)
- return 0;
+ /* Pass the order to phylink layer.
+ * Don't test return value as set_wol() is often not supported.
+ */
+ phylink_ethtool_set_wol(bp->phylink, wol);
If this returns an error, does that mean WOL works or does it not?
In my use case (simple phy: "Micrel KSZ8081"), if I have the error
"-EOPNOTSUPP", it simply means that this phy driver doesn't have the
set_wol() function. But on the MAC side, I can perfectly wake-up on WoL
event as the phy acts as a pass-through.
Note that if set_wol() is not supported, this will return -EOPNOTSUPP.
What about other errors?
True, I don't manage them. But for now this patch is a fix that only
reverts to previous behavior. In other terms, it only fixes the regression.
But can I make the difference, and how, between?
1/ the phy doesn't support WoL and could prevent the WoL to happen on
the MAC
2/ the phy doesn't implement (yet) the set_wol() function, if MAC can
manage, it's fine
If you want to just ignore the case where it's not supported, then
this looks like a sledge hammer to crack a nut.
Do you suggest that I just don't call phylink_ethtool_set_wol() at all?
But what if the underlying phy does support WoL?
Best regards,
--
Nicolas Ferre
Re: [PATCH v4 3/5] net: macb: fix macb_get/set_wol() when moving to phylink
On Wed, May 06, 2020 at 01:37:39PM +0200, nicolas.fe...@microchip.com wrote:
> From: Nicolas Ferre
>
> Keep previous function goals and integrate phylink actions to them.
>
> phylink_ethtool_get_wol() is not enough to figure out if Ethernet driver
> supports Wake-on-Lan.
> Initialization of "supported" and "wolopts" members is done in phylink
> function, no need to keep them in calling function.
>
> phylink_ethtool_set_wol() return value is not enough to determine
> if WoL is enabled for the calling Ethernet driver. Call it first
> but don't rely on its return value as most of simple PHY drivers
> don't implement a set_wol() function.
>
> Fixes: 7897b071ac3b ("net: macb: convert to phylink")
> Signed-off-by: Nicolas Ferre
> Reviewed-by: Florian Fainelli
> Cc: Claudiu Beznea
> Cc: Harini Katakam
> Cc: Antoine Tenart
> ---
> drivers/net/ethernet/cadence/macb_main.c | 18 ++
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c
> b/drivers/net/ethernet/cadence/macb_main.c
> index 53e81ab048ae..24c044dc7fa0 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2817,21 +2817,23 @@ static void macb_get_wol(struct net_device *netdev,
> struct ethtool_wolinfo *wol)
> {
> struct macb *bp = netdev_priv(netdev);
>
> - wol->supported = 0;
> - wol->wolopts = 0;
> -
> - if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET)
> + if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) {
> phylink_ethtool_get_wol(bp->phylink, wol);
> + wol->supported |= WAKE_MAGIC;
> +
> + if (bp->wol & MACB_WOL_ENABLED)
> + wol->wolopts |= WAKE_MAGIC;
> + }
> }
>
> static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo
> *wol)
> {
> struct macb *bp = netdev_priv(netdev);
> - int ret;
>
> - ret = phylink_ethtool_set_wol(bp->phylink, wol);
> - if (!ret)
> - return 0;
> + /* Pass the order to phylink layer.
> + * Don't test return value as set_wol() is often not supported.
> + */
> + phylink_ethtool_set_wol(bp->phylink, wol);
If this returns an error, does that mean WOL works or does it not?
Note that if set_wol() is not supported, this will return -EOPNOTSUPP.
What about other errors?
If you want to just ignore the case where it's not supported, then
this looks like a sledge hammer to crack a nut.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
