| Message ID | 20241107170255.1058124-5-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: freescale: ucc_geth: Phylink conversion | expand |
On Thu, Nov 07, 2024 at 06:02:51PM +0100, Maxime Chevallier wrote: > The get/set_wol ethtool ops rely on querying the PHY for its WoL > capabilities, checking for the presence of a PHY and a PHY interrupts > isn't enough. Address that by cleaning up the WoL configuration > sequence. > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > --- > .../net/ethernet/freescale/ucc_geth_ethtool.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/ucc_geth_ethtool.c b/drivers/net/ethernet/freescale/ucc_geth_ethtool.c > index fb5254d7d1ba..2a085f8f34b2 100644 > --- a/drivers/net/ethernet/freescale/ucc_geth_ethtool.c > +++ b/drivers/net/ethernet/freescale/ucc_geth_ethtool.c > @@ -346,26 +346,37 @@ static void uec_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) > struct ucc_geth_private *ugeth = netdev_priv(netdev); > struct phy_device *phydev = netdev->phydev; > > - if (phydev && phydev->irq) > - wol->supported |= WAKE_PHY; > + wol->supported = 0; > + wol->wolopts = 0; > + > + if (phydev) > + phy_ethtool_get_wol(phydev, wol); > + > if (qe_alive_during_sleep()) > wol->supported |= WAKE_MAGIC; So get WoL will return whatever methods the PHY supports, plus WAKE_MAGIC is added because i assume the MAC can do that. So depending on the PHY, it could be the full list: #define WAKE_PHY (1 << 0) #define WAKE_UCAST (1 << 1) #define WAKE_MCAST (1 << 2) #define WAKE_BCAST (1 << 3) #define WAKE_ARP (1 << 4) #define WAKE_MAGIC (1 << 5) #define WAKE_MAGICSECURE (1 << 6) /* only meaningful if WAKE_MAGIC */ #define WAKE_FILTER (1 << 7) > > - wol->wolopts = ugeth->wol_en; > + wol->wolopts |= ugeth->wol_en; > } > > static int uec_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) > { > struct ucc_geth_private *ugeth = netdev_priv(netdev); > struct phy_device *phydev = netdev->phydev; > + int ret = 0; > > if (wol->wolopts & ~(WAKE_PHY | WAKE_MAGIC)) > return -EINVAL; > - else if (wol->wolopts & WAKE_PHY && (!phydev || !phydev->irq)) > + else if ((wol->wolopts & WAKE_PHY) && !phydev) > return -EINVAL; > else if (wol->wolopts & WAKE_MAGIC && !qe_alive_during_sleep()) > return -EINVAL; > > + if (wol->wolopts & WAKE_PHY) > + ret = phy_ethtool_set_wol(phydev, wol); Here, the code only calls into the PHY for WAKE_PHY, when in fact the PHY could be handling WAKE_UCAST, WAKE_MCAST, WAKE_ARP etc. So these conditions are wrong. It could we be that X years ago when this code was added only WAKE_PHY and WAKE_MAGIC existed? Andrew
Hello Andrew, On Thu, 7 Nov 2024 18:49:00 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > > + if (phydev) > > + phy_ethtool_get_wol(phydev, wol); > > + > > if (qe_alive_during_sleep()) > > wol->supported |= WAKE_MAGIC; > > So get WoL will return whatever methods the PHY supports, plus > WAKE_MAGIC is added because i assume the MAC can do that. So depending > on the PHY, it could be the full list: > > + if (wol->wolopts & WAKE_PHY) > > + ret = phy_ethtool_set_wol(phydev, wol); > > Here, the code only calls into the PHY for WAKE_PHY, when in fact the > PHY could be handling WAKE_UCAST, WAKE_MCAST, WAKE_ARP etc. > > So these conditions are wrong. It could we be that X years ago when > this code was added only WAKE_PHY and WAKE_MAGIC existed? Ah that's right indeed. So yeah my changes aren't enough, I'll go over that patch again. Thanks for spotting this, Maxime
diff --git a/drivers/net/ethernet/freescale/ucc_geth_ethtool.c b/drivers/net/ethernet/freescale/ucc_geth_ethtool.c index fb5254d7d1ba..2a085f8f34b2 100644 --- a/drivers/net/ethernet/freescale/ucc_geth_ethtool.c +++ b/drivers/net/ethernet/freescale/ucc_geth_ethtool.c @@ -346,26 +346,37 @@ static void uec_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) struct ucc_geth_private *ugeth = netdev_priv(netdev); struct phy_device *phydev = netdev->phydev; - if (phydev && phydev->irq) - wol->supported |= WAKE_PHY; + wol->supported = 0; + wol->wolopts = 0; + + if (phydev) + phy_ethtool_get_wol(phydev, wol); + if (qe_alive_during_sleep()) wol->supported |= WAKE_MAGIC; - wol->wolopts = ugeth->wol_en; + wol->wolopts |= ugeth->wol_en; } static int uec_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol) { struct ucc_geth_private *ugeth = netdev_priv(netdev); struct phy_device *phydev = netdev->phydev; + int ret = 0; if (wol->wolopts & ~(WAKE_PHY | WAKE_MAGIC)) return -EINVAL; - else if (wol->wolopts & WAKE_PHY && (!phydev || !phydev->irq)) + else if ((wol->wolopts & WAKE_PHY) && !phydev) return -EINVAL; else if (wol->wolopts & WAKE_MAGIC && !qe_alive_during_sleep()) return -EINVAL; + if (wol->wolopts & WAKE_PHY) + ret = phy_ethtool_set_wol(phydev, wol); + + if (ret) + return ret; + ugeth->wol_en = wol->wolopts; device_set_wakeup_enable(&netdev->dev, ugeth->wol_en);
The get/set_wol ethtool ops rely on querying the PHY for its WoL capabilities, checking for the presence of a PHY and a PHY interrupts isn't enough. Address that by cleaning up the WoL configuration sequence. Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- .../net/ethernet/freescale/ucc_geth_ethtool.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)