Re: [PATCH net-next 6/6] net: ethernet: fs_enet: phylink conversion

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hello Russell,

On Wed, 28 Aug 2024 14:54:57 +0100
"Russell King (Oracle)" <linux@armlinux.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@armlinux.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