netdev - Re: [PATCH net-next v1 5/5] net: tn40xx: add PHYLIB 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]

Message-ID: <7c20aefa-d93b-41e2-9a23-97782926369d@lunn.ch>
Date: Mon, 15 Apr 2024 16:44:31 +0200
From: Andrew Lunn <andrew@...n.ch>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH net-next v1 5/5] net: tn40xx: add PHYLIB support

On Mon, Apr 15, 2024 at 07:43:52PM +0900, FUJITA Tomonori wrote:
> This patch adds supports for multiple PHY hardware with PHYLIB. 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 with
> Edimax EN-9320 10G adapter.

Please split this up. Add the MDIO bus master in one patch. Then add
support for phylib in a second patch. They are logically different
things.

Are there variants of this device using SFP? It might be you actually
want to use phylink, not phylib. That is a bit messy for a PCI device,
look at drivers/net/ethernet/wangxun.

> diff --git a/drivers/net/ethernet/tehuti/Kconfig b/drivers/net/ethernet/tehuti/Kconfig
> index 4198fd59e42e..71f22471f9a0 100644
> --- a/drivers/net/ethernet/tehuti/Kconfig
> +++ b/drivers/net/ethernet/tehuti/Kconfig
> @@ -27,6 +27,7 @@ config TEHUTI_TN40
>  	tristate "Tehuti Networks TN40xx 10G Ethernet adapters"
>  	depends on PCI
>  	select FW_LOADER
> +	select AMCC_QT2025_PHY

That is pretty unusual, especially when you say there are a few
different choices.

> +static u32 bdx_mdio_get(struct bdx_priv *priv)
> +{
> +	void __iomem *regs = priv->regs;
> +
> +#define BDX_MAX_MDIO_BUSY_LOOPS 1024
> +	int tries = 0;
> +
> +	while (++tries < BDX_MAX_MDIO_BUSY_LOOPS) {
> +		u32 mdio_cmd_stat = readl(regs + REG_MDIO_CMD_STAT);
> +
> +		if (GET_MDIO_BUSY(mdio_cmd_stat) == 0)
> +			return mdio_cmd_stat;
> +	}
> +	dev_err(&priv->pdev->dev, "MDIO busy!\n");

include/linux/iopoll.h

> +	return 0xFFFFFFFF;

It is always better to use standard error codes. In this case,
-ETIMEDOUT.

> +static u16 bdx_mdio_read(struct bdx_priv *priv, int device, int port, u16 addr)
> +{
> +	void __iomem *regs = priv->regs;
> +	u32 tmp_reg, i;
> +	/* wait until MDIO is not busy */
> +	if (bdx_mdio_get(priv) == 0xFFFFFFFF)
> +		return -1;
> +
> +	i = ((device & 0x1F) | ((port & 0x1F) << 5));
> +	writel(i, regs + REG_MDIO_CMD);
> +	writel((u32)addr, regs + REG_MDIO_ADDR);
> +	tmp_reg = bdx_mdio_get(priv);
> +	if (tmp_reg == 0xFFFFFFFF)
> +		return -1;

This function has a return type of u16. So returning -1 makes no sense.

> +static int mdio_read_reg(struct mii_bus *mii_bus, int addr, int devnum, int regnum)
> +{
> +	return bdx_mdio_read(mii_bus->priv, devnum, addr, regnum);

I would probably change bdx_mdio_read() so that it takes the
parameters in the same order as mdio_read_reg().

There is also a reasonably common convention that the functions
performing C45 bus protocol operations have c45 in their name. It
appears this hardware does not support C22 at all. That makes it
unusual, and little hits like this are useful.

	 Andrew