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

[Vineeth Karumanchi <vineeth.karumanchi@amd.com>]

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