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: <CAKgT0UcW9qhXd0JjRiJo=RVautsOa9grsWiee+OwT=1Agiib0w@mail.gmail.com>
Date: Tue, 28 Oct 2025 15:26:34 -0700
From: Alexander Duyck <alexander.duyck@...il.com>
To: Andrew Lunn <andrew@...n.ch>
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

On Tue, Oct 28, 2025 at 1:51 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> > +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.

In the case of fbnic it is possible as the fbd is appended to the
devlink structure. So it exists before the netdev does.

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

The problem with returning an error is that it causes other code to do
weird things. I figure we are better off just not returning a speed in
that case.

> > +     }
> > +
> > +     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?

I will look at adding it. Probably have it output the regnum and the value.

> > +     }
> > +
> > +     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()?

It will cause stuff to blow up. There are several writes that are done
that can be safely ignored. The issue is if I start returning an error
I get no phy at all.

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

I had based the logic on the fixed_phy approach
(https://elixir.bootlin.com/linux/v6.18-rc3/C/ident/fixed_mdio_write).
Basically it just returns 0 and doesn't do anything in response to
writes. Seems like we do the same thing for phylink assuming we aren't
passing it to an actual phy in the phylink_mii_read/write calls.