diff mbox series

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

Message ID 20240605102457.4050539-4-vineeth.karumanchi@amd.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: macb: WOL enhancements | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 922 this patch: 922
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 905 this patch: 905
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 927 this patch: 927
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 99 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-06-05--15-00 (tests: 1043)

Commit Message

Vineeth Karumanchi June 5, 2024, 10:24 a.m. UTC 
Extend wake-on LAN support with an ARP packet.

Advertise wake-on LAN supported modes by default without relying on
dt node. By default, wake-on LAN will be in disabled state.
Using ethtool users can enable/disable or choose packet types.

For wake-on LAN via ARP, ensure the IP address is assigned and
report an error otherwise.

Co-developed-by: Harini Katakam <harini.katakam@amd.com>
Signed-off-by: Harini Katakam <harini.katakam@amd.com>
Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/ethernet/cadence/macb.h      |  1 +
 drivers/net/ethernet/cadence/macb_main.c | 50 +++++++++++++++---------
 2 files changed, 32 insertions(+), 19 deletions(-)

Comments

Andrew Lunn June 6, 2024, 1:59 a.m. UTC | #1 
> @@ -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.

> -	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?

	Andrew
Vineeth Karumanchi June 7, 2024, 5:19 a.m. UTC | #2 
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.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 50cd35ef21ad..122663ff7834 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -1306,6 +1306,7 @@  struct macb {
 	unsigned int		jumbo_max_len;
 
 	u32			wol;
+	u32			wolopts;
 
 	/* holds value of rx watermark value for pbuf_rxcutthru register */
 	u32			rx_watermark;
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 4007b291526f..7eabd9c01f23 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -38,6 +38,7 @@ 
 #include <linux/ptp_classify.h>
 #include <linux/reset.h>
 #include <linux/firmware/xlnx-zynqmp.h>
+#include <linux/inetdevice.h>
 #include "macb.h"
 
 /* This structure is only used for MACB on SiFive FU540 devices */
@@ -84,8 +85,9 @@  struct sifive_fu540_macb_mgmt {
 #define GEM_MTU_MIN_SIZE	ETH_MIN_MTU
 #define MACB_NETIF_LSO		NETIF_F_TSO
 
-#define MACB_WOL_HAS_MAGIC_PACKET	(0x1 << 0)
-#define MACB_WOL_ENABLED		(0x1 << 1)
+#define MACB_WOL_ENABLED		(0x1 << 0)
+#define MACB_WOL_HAS_MAGIC_PACKET	(0x1 << 1)
+#define MACB_WOL_HAS_ARP_PACKET		(0x1 << 2)
 
 #define HS_SPEED_10000M			4
 #define MACB_SERDES_RATE_10G		1
@@ -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);
+	wol->supported |= (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) ? WAKE_MAGIC : 0;
+	wol->supported |= (bp->wol & MACB_WOL_HAS_ARP_PACKET) ? WAKE_ARP : 0;
+	/* Pass wolopts to ethtool */
+	wol->wolopts = bp->wolopts;
 }
 
 static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
@@ -3300,11 +3300,10 @@  static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
 	if (!ret || ret != -EOPNOTSUPP)
 		return ret;
 
-	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;
 
-	bp->wol = 0;
-	if (of_property_read_bool(np, "magic-packet"))
-		bp->wol |= MACB_WOL_HAS_MAGIC_PACKET;
-	device_set_wakeup_capable(&pdev->dev, bp->wol & MACB_WOL_HAS_MAGIC_PACKET);
+	bp->wol = (MACB_WOL_HAS_ARP_PACKET | MACB_WOL_HAS_MAGIC_PACKET);
+	device_set_wakeup_capable(&pdev->dev, 1);
 
 	bp->usrio = macb_config->usrio;
 
@@ -5245,6 +5242,7 @@  static int __maybe_unused macb_suspend(struct device *dev)
 	struct net_device *netdev = dev_get_drvdata(dev);
 	struct macb *bp = netdev_priv(netdev);
 	struct macb_queue *queue;
+	struct in_ifaddr *ifa;
 	unsigned long flags;
 	unsigned int q;
 	int err;
@@ -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;
+		}
 		spin_lock_irqsave(&bp->lock, flags);
 
 		/* Disable Tx and Rx engines before  disabling the queues,
@@ -5290,6 +5294,14 @@  static int __maybe_unused macb_suspend(struct device *dev)
 		macb_writel(bp, TSR, -1);
 		macb_writel(bp, RSR, -1);
 
+		tmp = (bp->wolopts & WAKE_MAGIC) ? MACB_BIT(MAG) : 0;
+		if (bp->wolopts & WAKE_ARP) {
+			tmp |= MACB_BIT(ARP);
+			/* write IP address into register */
+			tmp |= MACB_BFEXT(IP,
+					 (__force u32)(cpu_to_be32p((uint32_t *)&ifa->ifa_local)));
+		}
+
 		/* Change interrupt handler and
 		 * Enable WoL IRQ on queue 0
 		 */
@@ -5305,7 +5317,7 @@  static int __maybe_unused macb_suspend(struct device *dev)
 				return err;
 			}
 			queue_writel(bp->queues, IER, GEM_BIT(WOL));
-			gem_writel(bp, WOL, MACB_BIT(MAG));
+			gem_writel(bp, WOL, tmp);
 		} else {
 			err = devm_request_irq(dev, bp->queues[0].irq, macb_wol_interrupt,
 					       IRQF_SHARED, netdev->name, bp->queues);
@@ -5317,7 +5329,7 @@  static int __maybe_unused macb_suspend(struct device *dev)
 				return err;
 			}
 			queue_writel(bp->queues, IER, MACB_BIT(WOL));
-			macb_writel(bp, WOL, MACB_BIT(MAG));
+			macb_writel(bp, WOL, tmp);
 		}
 		spin_unlock_irqrestore(&bp->lock, flags);