netdev - Re: [net-next PATCH 7/8] fbnic: Add SW shim for MII interface to PMA/PMD

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: <0d66967f-e75f-4723-99f4-164abfe8b9ff@lunn.ch>
Date: Tue, 28 Oct 2025 21:51:42 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Alexander Duyck <alexander.duyck@...il.com>
Cc: netdev@...r.kernel.org, kuba@...nel.org, kernel-team@...a.com,
	andrew+netdev@...n.ch, hkallweit1@...il.com, linux@...linux.org.uk,
	pabeni@...hat.com, davem@...emloft.net
Subject: Re: [net-next PATCH 7/8] fbnic: Add SW shim for MII interface to
 PMA/PMD

> +static int
> +fbnic_swmii_read_pmapmd(struct fbnic_dev *fbd, int regnum)
> +{
> +	u16 ctrl1 = 0, ctrl2 = 0;
> +	struct fbnic_net *fbn;
> +	int ret = 0;
> +	u8 aui;
> +
> +	if (fbd->netdev) {

Is that even impossible? I don't remember the core code, but usually
the priv structure is appended to the end of the net_device structure.

> +		fbn = netdev_priv(fbd->netdev);
> +		aui = fbn->aui;
> +	}
> +
> +	switch (aui) {
> +	case FBNIC_AUI_25GAUI:
> +		ctrl1 = MDIO_CTRL1_SPEED25G;
> +		ctrl2 = MDIO_PMA_CTRL2_25GBCR;
> +		break;
> +	case FBNIC_AUI_LAUI2:
> +		ctrl1 = MDIO_CTRL1_SPEED50G;
> +		ctrl2 = MDIO_PMA_CTRL2_50GBCR2;
> +		break;
> +	case FBNIC_AUI_50GAUI1:
> +		ctrl1 = MDIO_CTRL1_SPEED50G;
> +		ctrl2 = MDIO_PMA_CTRL2_50GBCR;
> +		break;
> +	case FBNIC_AUI_100GAUI2:
> +		ctrl1 = MDIO_CTRL1_SPEED100G;
> +		ctrl2 = MDIO_PMA_CTRL2_100GBCR2;
> +		break;
> +	default:
> +		break;

If it is something else, is that a bug? Would it be better to return
-EINVAL?

> +	}
> +
> +	switch (regnum) {
> +	case MDIO_CTRL1:
> +		ret = ctrl1;
> +		break;
> +	case MDIO_STAT1:
> +		ret = fbd->pmd_state == FBNIC_PMD_SEND_DATA ?
> +		      MDIO_STAT1_LSTATUS : 0;
> +		break;
> +	case MDIO_DEVS1:
> +		ret = MDIO_DEVS_PMAPMD;
> +		break;
> +	case MDIO_CTRL2:
> +		ret = ctrl2;
> +		break;
> +	case MDIO_STAT2:
> +		ret = MDIO_STAT2_DEVPRST_VAL |
> +		      MDIO_PMA_STAT2_EXTABLE;
> +		break;
> +	case MDIO_PMA_EXTABLE:
> +		ret = MDIO_PMA_EXTABLE_40_100G |
> +		      MDIO_PMA_EXTABLE_25G;
> +		break;
> +	case MDIO_PMA_40G_EXTABLE:
> +		ret = MDIO_PMA_40G_EXTABLE_50GBCR2;
> +		break;
> +	case MDIO_PMA_25G_EXTABLE:
> +		ret = MDIO_PMA_25G_EXTABLE_25GBCR;
> +		break;
> +	case MDIO_PMA_50G_EXTABLE:
> +		ret = MDIO_PMA_50G_EXTABLE_50GBCR;
> +		break;
> +	case MDIO_PMA_EXTABLE2:
> +		ret = MDIO_PMA_EXTABLE2_50G;
> +		break;
> +	case MDIO_PMA_100G_EXTABLE:
> +		ret = MDIO_PMA_100G_EXTABLE_100GBCR2;
> +		break;
> +	default:
> +		break;

So the intention here is to return 0, meaning the register does not
exist, as defined by 802.3 for C45? Maybe add a comment?

I'm just wondering how robust this is. Maybe at a dev_dbg() printing
the regnum?

> +	}
> +
> +	return ret;
> +}
> +
> +static int
> +fbnic_swmii_read_c45(struct mii_bus *bus, int addr, int devnum, int regnum)
> +{
> +	struct fbnic_dev *fbd = bus->priv;
> +
> +	if (addr != 0)
> +		return 0xffff;
> +
> +	if (devnum == MDIO_MMD_PMAPMD)
> +		return fbnic_swmii_read_pmapmd(fbd, regnum);
> +
> +	return 0xffff;

For C45 you are supposed to return 0 if the register does not exist. It says:

   45.2 MDIO Interface registers

   If a device supports the MDIO interface it shall respond to all
   possible register addresses for the device and return a value of
   zero for undefined and unsupported registers.

> +static int
> +fbnic_swmii_write_c45(struct mii_bus *bus, int addr, int devnum,
> +		      int regnum, u16 val)
> +{
> +	/* Currently PHY setup is meant to be read-only */
> +	return 0;

-EOPNOTSUPP? WARN_ON_ONCE()?

If it does happen, i assume that means your assumptions are wrong?
Don't you want to know?

	Andrew