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

Message-ID: <9f6262bf-b559-48e0-97f0-9d83b3c9c5f8@lunn.ch>
Date: Tue, 29 Oct 2024 14:02:46 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Gerhard Engleder <gerhard@...leder-embedded.com>
Cc: Lee Trager <lee@...ger.us>, 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

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