netdev - Re: [PATCH net-next v7 6/6] net: tn40xx: add phylink support

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]

Date: Tue, 28 May 2024 13:02:42 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, andrew@...n.ch, horms@...nel.org,
	kuba@...nel.org, jiri@...nulli.us, pabeni@...hat.com,
	hfdevel@....net, naveenm@...vell.com, jdamato@...tly.com
Subject: Re: [PATCH net-next v7 6/6] net: tn40xx: add phylink support

On Tue, May 28, 2024 at 05:39:28AM +0900, FUJITA Tomonori wrote:
> This patch adds supports for multiple PHY hardware with phylink. The
> adapters with TN40xx chips use multiple PHY hardware; AMCC QT2025, TI
> TLK10232, Aqrate AQR105, and Marvell 88X3120, 88X3310, and MV88E2010.
> 
> For now, the PCI ID table of this driver enables adapters using only
> QT2025 PHY. I've tested this driver and the QT2025 PHY driver (SFP+
> 10G SR) with Edimax EN-9320 10G adapter.
> 
i> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>

A few comments - I don't recall seeing previous versions of these
patches, despite it being at version 7.

> @@ -1082,6 +1083,10 @@ static void tn40_link_changed(struct tn40_priv *priv)
>  				 TN40_REG_MAC_LNK_STAT) & TN40_MAC_LINK_STAT;
>  
>  	netdev_dbg(priv->ndev, "link changed %u\n", link);
> +	if (link)
> +		phylink_mac_change(priv->phylink, true);
> +	else
> +		phylink_mac_change(priv->phylink, false);

This is only useful if you have a PCS, and I don't see anything in the
driver that suggests you do. What link is this referring to?

In any case, you could eliminate the if() and just pass !!link if it's
not already boolean in nature (the if() suggests it is, so passing just
"link" would also work.)

> @@ -1381,10 +1389,17 @@ static int tn40_open(struct net_device *dev)
>  	struct tn40_priv *priv = netdev_priv(dev);
>  	int ret;
>  
> +	ret = phylink_connect_phy(priv->phylink, priv->phydev);
> +	if (ret)
> +		return ret;
> +
>  	tn40_sw_reset(priv);
> +	phylink_start(priv->phylink);

At this point, the link could have come up (mac_link_up() could well
be called.) Is the driver prepared to cope with that happening right
_now_ in the tn40_open sequence? If not, you will need to move this
to a point where the driver is ready to begin operation.

phylink_stop() is its opposite, and you need to call that at the
point where you want the link to be taken down (iow, where you want
.mac_link_down() to be guaranteed to have been called if the link
was already up.)

> @@ -143,6 +143,9 @@ struct tn40_priv {
>  	char *b0_va; /* Virtual address of buffer */
>  
>  	struct mii_bus *mdio;
> +	struct phy_device *phydev;
> +	struct phylink *phylink;
> +	struct phylink_config phylink_config;

So phylink_config is embedded in tn40_priv - that's fine. What this does
mean is you can trivially go from the phylink_config pointer to a
pointer to tn40_priv *without* multiple dereferences:

static inline struct tn40_priv *
config_to_tn40_priv(struct phylink_config *config)
{
	return container_of(config, struct tn40_priv, phylink_config);
}

> +static void tn40_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 net_device *ndev = to_net_dev(config->dev);
> +	struct tn40_priv *priv = netdev_priv(ndev);
> +
> +	tn40_set_link_speed(priv, speed);
> +	netif_wake_queue(priv->ndev);
> +}
> +
> +static void tn40_link_down(struct phylink_config *config, unsigned int mode,
> +			   phy_interface_t interface)
> +{
> +	struct net_device *ndev = to_net_dev(config->dev);
> +	struct tn40_priv *priv = netdev_priv(ndev);
> +
> +	tn40_set_link_speed(priv, 0);
> +	netif_stop_queue(priv->ndev);

Shouldn't the queue be stopped first?

> +}
> +
> +static void tn40_mac_config(struct phylink_config *config, unsigned int mode,
> +			    const struct phylink_link_state *state)
> +{
> +}

Nothing needs to be done here?

> +
> +static const struct phylink_mac_ops tn40_mac_ops = {
> +	.mac_config = tn40_mac_config,
> +	.mac_link_up = tn40_link_up,
> +	.mac_link_down = tn40_link_down,
> +};
> +
> +int tn40_phy_register(struct tn40_priv *priv)
> +{
> +	struct phylink_config *config;
> +	struct phy_device *phydev;
> +	struct phylink *phylink;
> +
> +	phydev = phy_find_first(priv->mdio);
> +	if (!phydev) {
> +		dev_err(&priv->pdev->dev, "PHY isn't found\n");
> +		return -1;
> +	}
> +
> +	config = &priv->phylink_config;
> +	config->dev = &priv->ndev->dev;
> +	config->type = PHYLINK_NETDEV;
> +	config->mac_capabilities = MAC_10000FD | MLO_AN_PHY;

MLO_AN_PHY is not a MAC capability, it shouldn't be here.

Thanks.

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