netdev - Re: [PATCH net-next 2/2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

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: <700c326c-d154-4d21-b9d4-d8abf8f2bf33@lunn.ch>
Date: Tue, 12 Nov 2024 14:50:16 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Tristram.Ha@...rochip.com
Cc: Woojung.Huh@...rochip.com, olteanv@...il.com, robh@...nel.org,
	krzk+dt@...nel.org, conor+dt@...nel.org, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	marex@...x.de, UNGLinuxDriver@...rochip.com,
	devicetree@...r.kernel.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] net: dsa: microchip: Add SGMII port support
 to KSZ9477 switch

On Tue, Nov 12, 2024 at 02:55:29AM +0000, Tristram.Ha@...rochip.com wrote:
> > On Fri, Nov 08, 2024 at 05:56:33PM -0800, Tristram.Ha@...rochip.com wrote:
> > > From: Tristram Ha <tristram.ha@...rochip.com>
> > >
> > > The SGMII module of KSZ9477 switch can be setup in 3 ways: 0 for direct
> > > connect, 1 for 1000BaseT/1000BaseX SFP, and 2 for 10/100/1000BaseT SFP.
> > 
> > This naming is rather odd. First off, i would drop 'SFP'. It does not
> > have to be an SFP on the other end, it could be another switch for
> > example. 1 is PHY_INTERFACE_MODE_1000BASEX and 2 is
> > PHY_INTERFACE_MODE_SGMII.
> > 
> > > SFP is typically used so the default is 1.  The driver can detect
> > > 10/100/1000BaseT SFP and change the mode to 2.
> > 
> > phylink will tell you want mode to use. I would ignore what the
> > hardware detects, so this driver is just the same as every other
> > driver, making it easier to maintain.
> 
> There are some issues I found that will need your advises.
> 
> The phylink SFP code categorizes SFP using fiber cable as
> PHY_INTERFACE_MODE_1000BASEX and SFP using a regular RJ45 connector as 
> PHY_INTERFACE_MODE_SGMII, which has a PHY that can be accessed through
> I2C connection with a PHY driver.

Not quite correct, i think. If MDIO over I2C does not work, it will
still decide on 1000BaseX vs SGMII from the SFP eeprom contents. There
are some SFPs where the PHY is not accessible, and we have to live
with however it is configured.

> Now when SGMII SFP is used the phylink
> cannot be created because it fails the validation in
> phylink_sfp_config_phy().

Please stop using 'SGMII SFP'. It should just be SGMII. The MAC should
not care what is on the other end, it could be a PHY, and SFP, or a
switch, all using Cisco SGMII.

> The reason is the phydev has empty supported
> and advertising data fields as it is just created.

Do you mean the phydev for the PHY in the SFP? Or do you have a second
phydev here? I'm confused.

> I mentioned the SGMII module operates differently for two types of SFP:
> SGMII and 1000BASEX.  The 1000Base-T SFP operates the same as 1000Base-SX
> fiber SFP, and the driver would like it to be assigned
> PHY_INTERFACE_MODE_1000BASEX, but it is always assigned
> PHY_INTERFACE_MODE_SGMII in sfp_select_interface because 1000baseT_Full
> is compared before 1000baseX_Full.
> 
> Now I am not sure if those SFPs I tested have correct EEPROM.  Some
> no-brand ones return 0xff value when the PHY driver reads the link status
> from them and so that driver cannot tell when the link is down.  Other
> than that those SFPs operate correctly in forwarding traffic.

There is no standardisation of how you access the PHY in an SFP. So
each manufacture can do their own thing. However, there are a small
number of PHYs actually used inside SFPs, and we have support for
those common ones.

> It seems there is no way to assign an interupt to that PHY and so polling
> is always used.

Correct, the interface between the SFP and the SFP cage does not have
an interrupt pin the PHY can use.

> The SFP using fiber cable does not have the above issues but has its own
> issue.  The SFP cage can detect the cable is being plugged in as the
> light is detected.  The PCS driver is then consulted to confirm the link.
> However as the cable is not completely plugged in the driver can report
> the link is not up.  After two calls are done the port can be left into
> unconnected state forever even after the cable is plugged in.  The driver
> can detect there is something different about the link value and can try
> to read it later to get a firm reading.  This just means the driver needs
> to poll anyway.  But if interrupt is used and the driver uses it to
> report link this issue is moot.

Have you set pcs.poll? phylink will then poll the PCS every
second. You can report PCS status any time.

> I just feel the SFP code offered in the phylink model does not help to
> operate SFP well in the KSZ9477 switch, and it does not need that code to
> provide basic SGMII port connection.

We need to understand what is different about the KSZ9477 switch that
phylink does not work for it, yet phylink does work for mv88e6xxx,
sja1105, rzn1, qca, ocelot, and other MAC drivers.

	Andrew