netdev - Re: [PATCH net-next 6/6] net: ethernet: fs

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: <20240828163151.485b2907@device-28.home>
Date: Wed, 28 Aug 2024 16:31:51 +0200
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: davem@...emloft.net, Pantelis Antoniou <pantelis.antoniou@...il.com>,
 Andrew Lunn <andrew@...n.ch>, Jakub Kicinski <kuba@...nel.org>, Eric
 Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Christophe
 Leroy <christophe.leroy@...roup.eu>, Florian Fainelli
 <f.fainelli@...il.com>, Heiner Kallweit <hkallweit1@...il.com>,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 thomas.petazzoni@...tlin.com, Herve Codina <herve.codina@...tlin.com>,
 linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH net-next 6/6] net: ethernet: fs_enet: phylink conversion

Hello Russell,

On Wed, 28 Aug 2024 14:54:57 +0100
"Russell King (Oracle)" <linux@...linux.org.uk> wrote:

> On Wed, Aug 28, 2024 at 01:44:13PM +0200, Maxime Chevallier wrote:
> > Hi Russell,
> > 
> > On Wed, 28 Aug 2024 11:38:31 +0100
> > "Russell King (Oracle)" <linux@...linux.org.uk> wrote:
> >   
> > > On Wed, Aug 28, 2024 at 11:51:02AM +0200, Maxime Chevallier wrote:  
> > > > +static int fs_eth_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> > > > +{
> > > > +	struct fs_enet_private *fep = netdev_priv(dev);
> > > > +
> > > > +	if (!netif_running(dev))
> > > > +		return -EINVAL;    
> > > 
> > > Why do you need this check?
> > >   
> > 
> > I included it as the original ioctl was phy_do_ioctl_running(), which
> > includes that check.
> > 
> > Is this check irrelevant with phylink ? I could only find macb and
> > xilinx_axienet that do the same check in their ioctl.
> > 
> > I can't tell you why that check is there in the first place in that
> > driver, a quick grep search leads back from a major driver rework in
> > 2011, at which point the check was already there...  
> 
> int phylink_mii_ioctl(struct phylink *pl, struct ifreq *ifr, int cmd)
> {
> 	if (pl->phydev) {
> 		... do PHY based access / pass on to phylib ...
> 	} else {
> 		... for reads:
> 		...  return emulated fixed-phy state if in fixed mode
> 		...  return emulated inband state if in inband mode
> 		... for writes:
> 		...  ignore writes in fixed and inband modes
> 		... otherwise return -EOPNOTSUPP
> 	}
> }
> 
> So, if a driver decides to connect the PHY during probe, the PHY will
> always be accessible.
> 
> If a driver decides to connect the PHY during .ndo_open, the PHY will
> only be accessible while the netdev is open, otherwise -EOPNOTSUPP
> will be returned.

That makes sense, so there's no point keeping that check then.

I'll update the patch, thanks for this clarification.

[...]

> At this point... this email has eaten up a substantial amount of time,
> and I can't spend anymore time in front of the screen today so that's
> the end of my contributions for today. Sorry.

I've been in the same rabbit-hole today debating in my head whether or
not to add this check, I'm sorry that I dragged you in there... With
what you stressed-out, I have a good enough justification to drop the
check in the current patch.

As for the current situation with the ioctl return codes, there indeed
room for lots of improvements. For drivers that simply forward the
ioctl to phylib/phylink, I think we could pretty easily add some
consistency on the return code, provided we agree on the proper one to
return.

Thanks for your time,

Maxime