netdev - Re: [PATCH net-next 1/4] net: phy: Allow loopback speed selection for PHY drivers

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] [day] [month] [year] [list]

Message-ID: <c593e14c-edde-45e7-8330-c095456af474@trager.us>
Date: Fri, 1 Nov 2024 16:21:04 -0700
From: Lee Trager <lee@...ger.us>
To: Andrew Lunn <andrew@...n.ch>,
 Gerhard Engleder <gerhard@...leder-embedded.com>
Cc: netdev@...r.kernel.org, pabeni@...hat.com, kuba@...nel.org,
 hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
 edumazet@...gle.com
Subject: Re: [PATCH net-next 1/4] net: phy: Allow loopback speed selection for
 PHY drivers

Thanks for the explanations, both make sense.

Lee

On 10/29/24 6:02 AM, Andrew Lunn wrote:
> On Tue, Oct 29, 2024 at 06:58:12AM +0100, Gerhard Engleder wrote:
>> On 29.10.24 00:23, Lee Trager wrote:
>>> On 10/28/24 1:38 PM, Gerhard Engleder wrote:
>>>> -int genphy_loopback(struct phy_device *phydev, bool enable)
>>>> +int genphy_loopback(struct phy_device *phydev, bool enable, int speed)
>>>>    {
>>>>        if (enable) {
>>>>            u16 ctl = BMCR_LOOPBACK;
>>>>            int ret, val;
>>>> +        if (speed == SPEED_10 || speed == SPEED_100 ||
>>>> +            speed == SPEED_1000)
>>>> +            phydev->speed = speed;
>>> Why is this limited to 1000? Shouldn't the max loopback speed be limited
>>> by max hardware speed? We currently have definitions going up to
>>> SPEED_800000 so some devices should support higher than 1000.
>> This generic loopback implementation only supports those three speeds
>> currently. If there is the need for higher speed, then there should be
>> PHY specific implementations in the PHY drivers.
>>
>>>    Why is speed defined as an int? It can never be negative, I normally
>>> see it defined as u32.
> https://elixir.bootlin.com/linux/v6.11.5/source/include/uapi/linux/ethtool.h#L2172
>
> #define SPEED_UNKNOWN		-1
>
> It cannot be unsigned.
>
> 	Andrew
>