Re: [PATCH net-next v1 1/2] net: asix: ax88772: migrate to phylink

[Russell King (Oracle)]

On Sat, Jul 23, 2022 at 07:47:10PM +0200, Oleksij Rempel wrote:
> There are some exotic ax88772 based devices which may require
> functionality provide by the phylink framework. For example:
> - US100A20SFP, USB 2.0 auf LWL Converter with SFP Cage
> - AX88772B USB to 100Base-TX Ethernet (with RMII) demo board, where it
>   is possible to switch between internal PHY and external RMII based
>   connection.
> 
> So, convert this driver to phylink as soon as possible.
> 
> Tested with:
> - AX88772A + internal PHY
> - AX88772B + external DP83TD510E T1L PHY
> 
> Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>

Sorry, I missed this.

> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> index 5b5eb630c4b7..3f93bc46a7eb 100644
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -327,6 +327,12 @@ static int ax88772_reset(struct usbnet *dev)
>  	struct asix_common_private *priv = dev->driver_priv;
>  	int ret;
>  
> +	ret = phylink_connect_phy(priv->phylink, priv->phydev);
> +	if (ret) {
> +		netdev_err(dev->net, "Could not connect PHY\n");
> +		return ret;
> +	}
> +

As Paolo points out, you need to clean this up with a call to
phylink_disconnect_phy() in the error paths in this function. Sorting
that out doesn't look like it would be sufficient, however. Looking at
usbnet_open(), there are a bunch of paths after ->reset() has been
called, where failures can occur and the chip specific driver doesn't
get notified to clean up, so in that case, the PHY would remain
attached.

So, I would say that the usbnet code is not structured to allow a call
to bind the PHY at this point.

However, this is also a functional change to the code - moving the call
to connect the PHY to (presumably) the .ndo_open() stage of the network
device lifetime, rather than the probe stage. In other words, this now
happens when the device is actually opened rather than when the device
is being bound to the driver.

That should have at least been described in the commit description, but
much better would be a separate patch to make that change. However, as
I say above, I think the usbnet code would need additional work to
permit this.

> +static void ax88772_mac_link_up(struct phylink_config *config,
> +			       struct phy_device *phy,
> +			       unsigned int mode, phy_interface_t interface,
> +			       int speed, int duplex,
> +			       bool tx_pause, bool rx_pause)
> +{
> +	struct usbnet *dev = netdev_priv(to_net_dev(config->dev));
> +	u16 m = AX_MEDIUM_AC | AX_MEDIUM_RE;
> +
> +	m |= duplex ? AX_MEDIUM_FD : 0;

I'd much rather this was:

	m |= duplex == DUPLEX_FULL ? AX_MEDIUM_FD : 0;

which is much clearer, and doesn't result in other values of duplex
resulting in full duplex. If you do want that behaviour (because the
original code used to), then please use:

	m |= duplex != DUPLEX_HALF ? AX_MEDIUM_FD : 0;

Either of these make the intention here explicit.

Thanks.

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