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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a358234a-8521-468e-8531-c8be0bbc619d@amd.com>
Date: Fri, 7 Jun 2024 10:49:22 +0530
From: Vineeth Karumanchi <vineeth.karumanchi@....com>
To: Andrew Lunn <andrew@...n.ch>
Cc: nicolas.ferre@...rochip.com, claudiu.beznea@...on.dev,
 davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
 conor+dt@...nel.org, linux@...linux.org.uk, vadim.fedorenko@...ux.dev,
 netdev@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, git@....com
Subject: Re: [PATCH net-next v3 3/4] net: macb: Add ARP support to WOL

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