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

fs_enet is a quite old but still used Ethernet driver found on some NXP
devices. It has support for 10/100 Mbps ethernet, with half and full
duplex. Some variants of it can use RMII, while other integrations are
MII-only.

Add phylink support, thus removing custom fixed-link hanldling.

This also allows removing some internal flags such as the use_rmii flag.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 .../net/ethernet/freescale/fs_enet/Kconfig    |   2 +-
 .../ethernet/freescale/fs_enet/fs_enet-main.c | 203 +++++++++---------
 .../net/ethernet/freescale/fs_enet/fs_enet.h  |  12 +-
 .../net/ethernet/freescale/fs_enet/mac-fcc.c  |  11 +-
 .../net/ethernet/freescale/fs_enet/mac-fec.c  |   9 +-
 .../net/ethernet/freescale/fs_enet/mac-scc.c  |   5 +-
 6 files changed, 120 insertions(+), 122 deletions(-)

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?

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...

Regards,

Maxime

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.

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.

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

On Wed, Aug 28, 2024 at 11:51:02AM +0200, Maxime Chevallier wrote:
> fs_enet is a quite old but still used Ethernet driver found on some NXP
> devices. It has support for 10/100 Mbps ethernet, with half and full
> duplex. Some variants of it can use RMII, while other integrations are
> MII-only.
> 
> Add phylink support, thus removing custom fixed-link hanldling.
> 
> This also allows removing some internal flags such as the use_rmii flag.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Hi Maxime,

Some minor issues from my side: not a full review by any stretch of
the imagination.

...

> @@ -911,7 +894,7 @@ static int fs_enet_probe(struct platform_device *ofdev)
>  	if (!IS_ERR(clk)) {
>  		ret = clk_prepare_enable(clk);
>  		if (ret)
> -			goto out_deregister_fixed_link;
> +			goto out_phylink;
>  
>  		fpi->clk_per = clk;
>  	}

This goto will result in a dereference of fep.
But fep is not initialised until the following line,
which appears a little below this hunk.

	fep = netdev_priv(ndev);

This goto will also result in the function returning without
releasing clk.

Both flagged by Smatch.

> @@ -936,6 +919,26 @@ static int fs_enet_probe(struct platform_device *ofdev)
>  	fep->fpi = fpi;
>  	fep->ops = ops;
>  
> +	fep->phylink_config.dev = &ndev->dev;
> +	fep->phylink_config.type = PHYLINK_NETDEV;
> +	fep->phylink_config.mac_capabilities = MAC_10 | MAC_100;
> +
> +	__set_bit(PHY_INTERFACE_MODE_MII,
> +		  fep->phylink_config.supported_interfaces);
> +
> +	if (of_device_is_compatible(ofdev->dev.of_node, "fsl,mpc5125-fec"))
> +		__set_bit(PHY_INTERFACE_MODE_RMII,
> +			  fep->phylink_config.supported_interfaces);
> +
> +	phylink = phylink_create(&fep->phylink_config, dev_fwnode(fep->dev),
> +				 phy_mode, &fs_enet_phylink_mac_ops);
> +	if (IS_ERR(phylink)) {
> +		ret = PTR_ERR(phylink);
> +		goto out_free_fpi;

This also appears to leak clk, as well as ndev.

I didn't look for other cases.

> +	}
> +
> +	fep->phylink = phylink;
> +
>  	ret = fep->ops->setup_data(ndev);
>  	if (ret)
>  		goto out_free_dev;
> @@ -968,8 +971,6 @@ static int fs_enet_probe(struct platform_device *ofdev)
>  
>  	ndev->ethtool_ops = &fs_ethtool_ops;
>  
> -	netif_carrier_off(ndev);
> -
>  	ndev->features |= NETIF_F_SG;
>  
>  	ret = register_netdev(ndev);
> @@ -988,10 +989,8 @@ static int fs_enet_probe(struct platform_device *ofdev)
>  	free_netdev(ndev);
>  out_put:
>  	clk_disable_unprepare(fpi->clk_per);
> -out_deregister_fixed_link:
> -	of_node_put(fpi->phy_node);
> -	if (of_phy_is_fixed_link(ofdev->dev.of_node))
> -		of_phy_deregister_fixed_link(ofdev->dev.of_node);
> +out_phylink:
> +	phylink_destroy(fep->phylink);
>  out_free_fpi:
>  	kfree(fpi);
>  	return ret;

...

Hello Simon,

On Wed, 28 Aug 2024 17:32:24 +0100
Simon Horman <horms@kernel.org> wrote:

> On Wed, Aug 28, 2024 at 11:51:02AM +0200, Maxime Chevallier wrote:
> > fs_enet is a quite old but still used Ethernet driver found on some NXP
> > devices. It has support for 10/100 Mbps ethernet, with half and full
> > duplex. Some variants of it can use RMII, while other integrations are
> > MII-only.
> > 
> > Add phylink support, thus removing custom fixed-link hanldling.
> > 
> > This also allows removing some internal flags such as the use_rmii flag.
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>  
> 
> Hi Maxime,
> 
> Some minor issues from my side: not a full review by any stretch of
> the imagination.
> 
> ...
> 
> > @@ -911,7 +894,7 @@ static int fs_enet_probe(struct platform_device *ofdev)
> >  	if (!IS_ERR(clk)) {
> >  		ret = clk_prepare_enable(clk);
> >  		if (ret)
> > -			goto out_deregister_fixed_link;
> > +			goto out_phylink;
> >  
> >  		fpi->clk_per = clk;
> >  	}  
> 
> This goto will result in a dereference of fep.
> But fep is not initialised until the following line,
> which appears a little below this hunk.
> 
> 	fep = netdev_priv(ndev);

Nice catch, the goto should rather go to out_free_fpi.

> 
> This goto will also result in the function returning without
> releasing clk.

ah yes, it's never disabled_unprepared, the phylink cleanup label is at
the wrong spot. I'll include a patch in the next iteration to make use
of devm_clk_get_enabled(), that should simplify all of that.

> Both flagged by Smatch.
> 
> > @@ -936,6 +919,26 @@ static int fs_enet_probe(struct platform_device *ofdev)
> >  	fep->fpi = fpi;
> >  	fep->ops = ops;
> >  
> > +	fep->phylink_config.dev = &ndev->dev;
> > +	fep->phylink_config.type = PHYLINK_NETDEV;
> > +	fep->phylink_config.mac_capabilities = MAC_10 | MAC_100;
> > +
> > +	__set_bit(PHY_INTERFACE_MODE_MII,
> > +		  fep->phylink_config.supported_interfaces);
> > +
> > +	if (of_device_is_compatible(ofdev->dev.of_node, "fsl,mpc5125-fec"))
> > +		__set_bit(PHY_INTERFACE_MODE_RMII,
> > +			  fep->phylink_config.supported_interfaces);
> > +
> > +	phylink = phylink_create(&fep->phylink_config, dev_fwnode(fep->dev),
> > +				 phy_mode, &fs_enet_phylink_mac_ops);
> > +	if (IS_ERR(phylink)) {
> > +		ret = PTR_ERR(phylink);
> > +		goto out_free_fpi;  
> 
> This also appears to leak clk, as well as ndev.

Thanks, will be addressed in V2.

> I didn't look for other cases.

I'll go over the cleanup path, thanks for checking this !

Thanks,

Maxime