linux-kernel - Re: [PATCH net-next V2 1/5] net: lan743x: Add SFP support check flag

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: <ad0813aa-1a11-4a26-8bc7-528ef51cf0c2@lunn.ch>
Date: Thu, 12 Sep 2024 17:58:14 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Ronnie.Kunin@...rochip.com
Cc: Raju.Lakkaraju@...rochip.com, netdev@...r.kernel.org,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, Bryan.Whitehead@...rochip.com,
	UNGLinuxDriver@...rochip.com, linux@...linux.org.uk,
	maxime.chevallier@...tlin.com, rdunlap@...radead.org,
	Steen.Hegelund@...rochip.com, Daniel.Machon@...rochip.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next V2 1/5] net: lan743x: Add SFP support check flag

> > > > > +     if (adapter->is_pci11x1x && !adapter->is_sgmii_en &&
> > > > > +         adapter->is_sfp_support_en) {
> > > > > +             netif_err(adapter, drv, adapter->netdev,
> > > > > +                       "Invalid eeprom cfg: sfp enabled with sgmii disabled");
> > > > > +             return -EINVAL;
> > > >
> > > > is_sgmii_en actually means PCS? An SFP might need 1000BaseX or
> > > > SGMII,
> > >
> > > No, not really.
> > > The PCI11010/PCI1414 chip can support either an RGMII interface or an
> > > SGMII/1000Base-X/2500Base-X interface.
> > 
> > A generic name for SGMII/1000Base-X/2500Base-X would be PCS, or maybe SERDES. To me, is_sgmii_en
> > means SGMII is enabled, but in fact it actually means SGMII/1000Base-X/2500Base-X is enabled. I just
> > think this is badly named. It would be more understandable if it was is_pcs_en.
> > 
> > > According to the datasheet,
> > > the "Strap Register (STRAP)" bit 6 is described as "SGMII_EN_STRAP"
> > > Therefore, the flag is named "is_sgmii_en".
> > 
> > Just because the datasheet uses a bad name does not mean the driver has to also use it.
> > 
> >         Andrew
> 
> The hardware architect, who is a very bright guy (it's not me :-), just called the strap SGMII_EN in order not to make the name too long and to contrast it with the opposite polarity of the bit which means the interface is set to RGMII; but in the description of the strap he clearly stated what it is:
> 	SGMII_EN_STRAP
> 	0 = RGMII
> 	1 = SGMII / 1000/2500BASE-X
> 
> I don't think PCS or Serdes (both of which get used in other technologies - some of which are also included in this chip and are therefore bound to create even more confusion if used) are good choices either.

SERDES i understand, PCI itself is a SERDES. But what are the other
uses of PCS? At least in the context of networking, PCS is reasonably
well understood.

> That being said, if it makes it more we can certainly call this flag "is_sgmii_basex_en". How's that sound ?

Better. But i still think PCS is better.

But you need to look at the wider context:

> > > > > +                       "Invalid eeprom cfg: sfp enabled with sgmii disabled");

SGMII is wrong here as well. You could flip it around:

> > > > > +                       "Invalid eeprom cfg: sfp enabled with RGMII");

In terms of reviewing this code, i have to ask myself the question,
does it really mean SGMII when it says SGMII? When you are talking
about Base-T, i don't know of any 1000BaseX PHYs, so you can be sloppy
with the term SGMII. But as soon as SFPs come into the mix, SGMII vs
1000BaseX becomes important, so you want the code to really mean SGMII
when it says SGMII.

     Andrew