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

From: "Russell King (Oracle)" <[email-masked]>
Date: Wed, 28 Aug 2024 14:54:57 +0100

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)" <[email-masked]> 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.

What do ethernet drivers return when they're not open or not having a
PHY? The answer is pretty random error codes.

drivers/net/ethernet/renesas/ravb_main.c-       if (!netif_running(ndev))
drivers/net/ethernet/renesas/ravb_main.c-               return -EINVAL;
drivers/net/ethernet/renesas/ravb_main.c-
drivers/net/ethernet/renesas/ravb_main.c-       if (!phydev)
drivers/net/ethernet/renesas/ravb_main.c-               return -ENODEV;
...
drivers/net/ethernet/renesas/ravb_main.c:       return phy_mii_ioctl(phydev, req, cmd);

drivers/net/ethernet/renesas/rswitch.c- if (!netif_running(ndev))
drivers/net/ethernet/renesas/rswitch.c-         return -EINVAL;

drivers/net/ethernet/8390/ax88796.c-    if (!netif_running(dev))
drivers/net/ethernet/8390/ax88796.c-            return -EINVAL;
drivers/net/ethernet/8390/ax88796.c-
drivers/net/ethernet/8390/ax88796.c-    if (!phy_dev)
drivers/net/ethernet/8390/ax88796.c-            return -ENODEV;

drivers/net/ethernet/marvell/mv643xx_eth.c-     if (!dev->phydev)
drivers/net/ethernet/marvell/mv643xx_eth.c-             return -ENOTSUPP;

drivers/net/ethernet/xilinx/xilinx_emaclite.c-  if (!dev->phydev || !netif_running(dev))
drivers/net/ethernet/xilinx/xilinx_emaclite.c-          return -EINVAL;

drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c-     if (!(netif_running(netdev)))
drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c-             return -EINVAL;

drivers/net/ethernet/actions/owl-emac.c-        if (!netif_running(netdev))
drivers/net/ethernet/actions/owl-emac.c-                return -EINVAL;

drivers/net/ethernet/engleder/tsnep_main.c-     if (!netif_running(netdev))
drivers/net/ethernet/engleder/tsnep_main.c-             return -EINVAL;

drivers/net/ethernet/ti/davinci_emac.c- if (!(netif_running(ndev)))
drivers/net/ethernet/ti/davinci_emac.c-         return -EINVAL;
drivers/net/ethernet/ti/davinci_emac.c- if (ndev->phydev)
drivers/net/ethernet/ti/davinci_emac.c:         return phy_mii_ioctl(ndev->phydev, ifrq, cmd);
drivers/net/ethernet/ti/davinci_emac.c- else
drivers/net/ethernet/ti/davinci_emac.c-         return -EOPNOTSUPP;

drivers/net/ethernet/ti/netcp_ethss.c-  if (phy)
drivers/net/ethernet/ti/netcp_ethss.c:          return phy_mii_ioctl(phy, req, cmd);
drivers/net/ethernet/ti/netcp_ethss.c-
drivers/net/ethernet/ti/netcp_ethss.c-  return -EOPNOTSUPP;

drivers/net/ethernet/ti/cpsw_priv.c-    if (phy)
drivers/net/ethernet/ti/cpsw_priv.c:            return phy_mii_ioctl(phy, req, cmd);
drivers/net/ethernet/ti/cpsw_priv.c-
drivers/net/ethernet/ti/cpsw_priv.c-    return -EOPNOTSUPP;

drivers/net/ethernet/xscale/ixp4xx_eth.c-       if (!netif_running(dev))
drivers/net/ethernet/xscale/ixp4xx_eth.c-               return -EINVAL;

drivers/net/ethernet/freescale/gianfar.c-       if (!netif_running(dev))
drivers/net/ethernet/freescale/gianfar.c-               return -EINVAL;
...
drivers/net/ethernet/freescale/gianfar.c-       if (!phydev)
drivers/net/ethernet/freescale/gianfar.c-               return -ENODEV;
drivers/net/ethernet/freescale/gianfar.c-
drivers/net/ethernet/freescale/gianfar.c:       return phy_mii_ioctl(phydev, rq, cmd);

drivers/net/ethernet/freescale/ucc_geth.c-      if (!netif_running(dev))
drivers/net/ethernet/freescale/ucc_geth.c-              return -EINVAL;
drivers/net/ethernet/freescale/ucc_geth.c-
drivers/net/ethernet/freescale/ucc_geth.c-      if (!ugeth->phydev)
drivers/net/ethernet/freescale/ucc_geth.c-              return -ENODEV;

drivers/net/ethernet/mediatek/mtk_star_emac.c-  if (!netif_running(ndev))
drivers/net/ethernet/mediatek/mtk_star_emac.c-          return -EINVAL;

drivers/net/ethernet/microchip/lan743x_main.c-  if (!netif_running(netdev))
drivers/net/ethernet/microchip/lan743x_main.c-          return -EINVAL;

drivers/net/ethernet/ethoc.c-   if (!netif_running(dev))
drivers/net/ethernet/ethoc.c-           return -EINVAL;

drivers/net/ethernet/broadcom/b44.c-    int err = -EINVAL;
drivers/net/ethernet/broadcom/b44.c-
drivers/net/ethernet/broadcom/b44.c-    if (!netif_running(dev))
drivers/net/ethernet/broadcom/b44.c-            goto out;

drivers/net/ethernet/broadcom/sb1250-mac.c-     if (!netif_running(dev) || !sc->phy_dev)
drivers/net/ethernet/broadcom/sb1250-mac.c-             return -EINVAL;

drivers/net/usb/smsc95xx.c-     if (!netif_running(netdev))
drivers/net/usb/smsc95xx.c-             return -EINVAL;

Of 28 drivers that call phy_mii_ioctl():
 - 17 drivers return EINVAL if !netif_running().
 - 11 drivers do not check netif_running().
and if there's no phydev:
 - 4 drivers return ENODEV
 - 3 drivers return EOPNOTSUPP (plus all drivers converted to phylink)
 - 2 drivers return EINVAL
 - 1 driver returns ENOTSUPP

Phylib itself doesn't want NULL passed to phy_mii_ioctl(), so its up to
the driver to trap this if its calling phy_mii_ioctl(). However, phylib
also provides phy_do_ioctl() for hooking directly into .ndo_eth_ioctl,
which is:

int phy_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
{
        if (!dev->phydev)
                return -ENODEV;

        return phy_mii_ioctl(dev->phydev, ifr, cmd);
}

then there's (as you point out) phy_do_ioctl_running(), which is:

int phy_do_ioctl_running(struct net_device *dev, struct ifreq *ifr, int cmd)
{
        if (!netif_running(dev))
                return -ENODEV;

        return phy_do_ioctl(dev, ifr, cmd);
}

and this returns ENODEV if the netif isn't running, not EINVAL which
the majority of net drivers that manually check are doing. Maybe phylib
has the error code wrong?

In any case, I think it's pretty clear that there's no single "right"
error code for these cases, everyone just puts up on a piece of paper
with a donkey the names of various error codes, and while blindfolded
sticks a pin in to find the "right" error code to use! So, I don't see
any point in debating what is the "One True Right Error Code" for these
conditions.

Now, the question is... why do drivers do this netif_running() check?
Is it because "other drivers do" or is it because there's a valid
reason. There's no way to know, no one ever documents why the check
is there - and based on your response, it's "because other drivers do".

Without looking at the history, I wouldn't make any assumption about
using phy_do_ioctl_running() - that could be the result of a drive-by
cleanup patch converting code to use that helper.

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.

-- 
*** please note that I probably will only be occasionally responsive
*** for an unknown period of time due to recent eye surgery making
*** reading quite difficult.

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!