netdev - Re: [PATCH net-next v5 5/6] net: tn40xx: add mdio bus support

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]

Date: Thu, 9 May 2024 11:52:46 +0200
From: Hans-Frieder Vogt <hfdevel@....net>
To: Andrew Lunn <andrew@...n.ch>, FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, horms@...nel.org, kuba@...nel.org,
 jiri@...nulli.us, pabeni@...hat.com
Subject: Re: [PATCH net-next v5 5/6] net: tn40xx: add mdio bus support

On 08.05.2024 21.30, Hans-Frieder Vogt wrote:

> On 08.05.2024 20.25, Andrew Lunn wrote:
>>>> +    writel(((1 << 15) | i), regs + TN40_REG_MDIO_CMD);
>>> similarly here:
>>>
>>> writel((MDIO_PHY_ID_C45 | i), regs + TN40_REG_MDIO_CMD);
>> This one i don't agree with. It happens to work, but there is no
>> reason to think the hardware has been designed around how Linux
>> combines the different parts of a C45 address into one word, using the
>> top bit to indicate it is actually a C45 address, not a C22.
>>
>> I would much prefer a TN40_ define is added for this bit.
> OK, yes, very valid point.

A small addition here:
digging through an old Tehuti linux river for the TN30xx (revision
7.33.5.1) I found revealing comments:
in bdx_mdio_read:
         /* Write read command */
         writel(MDIO_CMD_STAT_VAL(1, device, port), regs +
regMDIO_CMD_STAT);
in bdx_mdio_write:
         /* Write write command */
         writel(MDIO_CMD_STAT_VAL(0, device, port), regs +
regMDIO_CMD_STAT);

The CMD register has a different layout in the TN40xx, but the logic is
similar.
Therefore, I conclude now that the value (1 << 15)  is in fact a read
flag. Maybe it could be defined like:

#define TN40_MDIO_READ    BIT(15)

>>
>>>> +    writel(((device & 0x1F) | ((port & 0x1F) << 5)),
>>> and also here, similarly:
>>>
>>> writel((device & MDIO_PHY_ID_DEVAD) | ((port << 5) &
>>> MDIO_PHY_ID_PRTAD),
>> Similarly here, this happens to work, but that is just because the
>> hardware matches a software construct Linux uses. It would be better
>> to add TN40_ macros to describe the hardware.
> agreed, I assume I just interpreted too much into the constants.
>>
>>     Andrew
>
> Thanks!
> Hans
>
Thanks for challenging my initial assumptions,
Hans