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: <20240416.211926.560322866915632259.fujita.tomonori@gmail.com>
Date: Tue, 16 Apr 2024 21:19:26 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: andrew@...n.ch
Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v1 5/5] net: tn40xx: add PHYLIB support

Hi,

On Mon, 15 Apr 2024 16:44:31 +0200
Andrew Lunn <andrew@...n.ch> wrote:

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

Understood, I'll split this in v2.


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

phylink is necessary if PHY is hot-pluggable, right? if so, the driver
doesn't need it. The PHYs that adapters with TN40XX use are

AMCC QT2025 PHY (SFP+)
- Tehuti TN9310
- DLink DXE-810S
- Asus XG-C100F
- Edimax EN-9320

Marvell MV88x3120 (10GBase-T)
- Tehuti TN9210

Marvell MV88X3310 (10GBase-T)
- Tehuti TN9710
- Edimax EN-9320TX-E
- Buffalo LGY-PCIE-MG
- IOI GE10
- LR-Link LREC6860BT
- QNAP PCIe Expansion Card

Marvell MV88E2010 (5GBase-T)
- Tehuti TN9710Q

TI TLK10232 (SFP+)
- Tehuti TN9610
- LR-Link LREC6860AF

Aquantia AQR105 (10GBase-T)
- Tehuti TN9510
- DLink DXE-810T
- Edimax EN-9320TX-E


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

I should not put any 'select *_PHY' here?


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

I'll


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

Yeah, I thought the same but left it alone. I'll change in v2.


>> +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().

Sure, I'll.


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

I'm not sure the adapters supports C22 or not (probably do, I
guess). But the original driver uses C45 bus protocol operations.