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

[Simon Horman <horms@kernel.org>]

On Mon, Jun 17, 2024 at 12:34:12PM +0530, Vineeth Karumanchi wrote:
> Extend wake-on LAN support with an ARP packet.
> 
> Currently, if PHY supports WOL, ethtool ignores the modes supported
> by MACB. This change extends the WOL modes with MACB supported modes.
> 
> 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>

...

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c

...

> @@ -84,8 +85,7 @@ 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)


nit: BIT() could be used here

>  
>  #define HS_SPEED_10000M			4
>  #define MACB_SERDES_RATE_10G		1

...

> @@ -5290,6 +5289,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)));

Hi Vineeth and Harini,

I guess I must be reading this wrong, beause I am confused
by the intent of the endeness handling above.

* ifa->ifa_local is a 32-bit big-endian value

* It's address is cast to a 32-bit host-endian pointer

  nit: I think u32 would be preferable to uint32_t; this is kernel code.

* The value at this address is then converted to a host byte order value.

  nit: Why is cpu_to_be32p() used here instead of the more commonly used
       cpu_to_be32() ?

  More importantly, why is a host byte order value being converted from
  big-endian to host byte order?

* The value returned by cpu_to_be32p, which is big-endian, because
  that is what that function does, is then cast to host-byte order.


So overall we have:

1. Cast from big endian to host byte order
2. Conversion from host byte order to big endian
   (a bytes-swap on litte endian hosts; no-op on big endian hosts)
3. Cast from big endian to host byte oder

All three of these steps seem to warrant explanation.
And the combination is confusing to say the least.


> +		}
> +
>  		/* Change interrupt handler and
>  		 * Enable WoL IRQ on queue 0

...