[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <616a10c5-9c72-4221-a181-6251e808b9b8@amd.com>
Date: Thu, 20 Jun 2024 21:29:01 +0530
From: "Karumanchi, Vineeth" <vineeth.karumanchi@....com>
To: Simon Horman <horms@...nel.org>
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,
andrew@...n.ch, netdev@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, git@....com
Subject: Re: [PATCH net-next v6 3/4] net: macb: Add ARP support to WOL
Hi Simon,
On 6/18/2024 4:26 PM, Simon Horman wrote:
> On Mon, Jun 17, 2024 at 12:34:12PM +0530, Vineeth Karumanchi wrote:
>> Extend wake-on LAN support with an ARP packet.
...
> ...
>
>> 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
>
OK.
>>
>> #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.
>
tmp |= MACB_BFEXT(IP, be32_to_cpu(ifa->ifa_local));
The above snippet will address above points.
Consider the ip address is : 11.11.70.78
1. ifa->ifa_local : returns be32 -> 0x4E460b0b
2. be32_to_cpu(ifa->ifa_local) : converts be32 to host byte order u32:
0x0b0b464e
There are no sparse errors as well.
I will make the change, please let me know your suggestions/thoughts.
Thanks
🙏 vineeth
...
Powered by blists - more mailing lists