Re: [PATCH net-next v3 3/4] net: macb: Add ARP support to WOL

From: Vineeth Karumanchi
Date: Fri Jun 07 2024 - 01:19:53 EST


Hi Andrew,

On 06/06/24 7:29 am, Andrew Lunn wrote:
@@ -3278,13 +3280,11 @@ static void macb_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
{
struct macb *bp = netdev_priv(netdev);
- 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;
- }
+ phylink_ethtool_get_wol(bp->phylink, wol);

So you ask the PHY what it supports, and what it currently has
enabled.

+ wol->supported |= (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) ? WAKE_MAGIC : 0;
+ wol->supported |= (bp->wol & MACB_WOL_HAS_ARP_PACKET) ? WAKE_ARP : 0;

You mask in what the MAC supports.

+ /* Pass wolopts to ethtool */
+ wol->wolopts = bp->wolopts;

And then you overwrite what the PHY is currently doing with
bp->wolopts.

Now, if we look at what macb_set_wol does:

@@ -3300,11 +3300,10 @@ static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
if (!ret || ret != -EOPNOTSUPP)
return ret;


We are a little bit short of context here. This is checking the return
value of:

ret = phylink_ethtool_set_wol(bp->phylink, wol);

So if there is no error, or an error which is not EOPNOTSUPP, it
returns here. So if the PHY supports WAKE_MAGIC and/or WAKE_ARP, there
is nothing for the MAC to do. Importantly, the code below which sets
bp->wolopts is not reached.

So your get_wol looks wrong.


yes, with PHY supporting WOL the if/return logic needs changes.

Consider the scenario of phy supporting a,b,c and macb
supporting c,d modes. For a,b,c phy should handle and for "d"
mode the handle should be at mac.

I will make the changes accordingly.
please let me know your thoughts or suggestions.


- if (!(bp->wol & MACB_WOL_HAS_MAGIC_PACKET) ||
- (wol->wolopts & ~WAKE_MAGIC))
- return -EOPNOTSUPP;
+ bp->wolopts = (wol->wolopts & WAKE_MAGIC) ? WAKE_MAGIC : 0;
+ bp->wolopts |= (wol->wolopts & WAKE_ARP) ? WAKE_ARP : 0;
- if (wol->wolopts & WAKE_MAGIC)
+ if (bp->wolopts)
bp->wol |= MACB_WOL_ENABLED;
else
bp->wol &= ~MACB_WOL_ENABLED;
@@ -5085,10 +5084,8 @@ static int macb_probe(struct platform_device *pdev)
else
bp->max_tx_length = GEM_MAX_TX_LEN;

@@ -5257,6 +5255,12 @@ static int __maybe_unused macb_suspend(struct device *dev)
return 0;
if (bp->wol & MACB_WOL_ENABLED) {
+ /* Check for IP address in WOL ARP mode */
+ ifa = rcu_dereference(__in_dev_get_rcu(bp->dev)->ifa_list);
+ if ((bp->wolopts & WAKE_ARP) && !ifa) {
+ netdev_err(netdev, "IP address not assigned\n");
+ return -EOPNOTSUPP;
+ }

I don't know suspend too well. Is returning an error enough abort the
suspend?


yes, it will abort suspend.

🙏 vineeth

Andrew