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@xxxxxxx>
> Signed-off-by: Harini Katakam <harini.katakam@xxxxxxx>
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@xxxxxxx>
...
> 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
...
