netdev - Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property to disable SGMII autoneg

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: <ZiqUB0lwgw7vIozG@eichest-laptop>
Date: Thu, 25 Apr 2024 19:33:59 +0200
From: Stefan Eichenberger <eichest@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, robh@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
	lxu@...linear.com, hkallweit1@...il.com, michael@...le.cc,
	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property
 to disable SGMII autoneg

On Thu, Apr 25, 2024 at 05:30:50PM +0100, Russell King (Oracle) wrote:
> On Thu, Apr 25, 2024 at 05:51:57PM +0200, Stefan Eichenberger wrote:
> > On Thu, Apr 25, 2024 at 03:30:52PM +0100, Russell King (Oracle) wrote:
> > > On Thu, Apr 25, 2024 at 01:24:51PM +0200, Stefan Eichenberger wrote:
> > > > I think it should also work for mvneta, at least
> > > > the code looks almost the same. I get the following for the Port
> > > > Auto-Negotiation Configuration Register:
> > > > 
> > > > For 1Gbit/s it switches to SGMII and enables inband AN:
> > > > Memory mapped at address 0xffffa0112000.
> > > > Value at address 0xF2132E0C (0xffffa0112e0c): 0xB0C6
> > > 
> > > So here the link is forced up which is wrong for inband, because then
> > > we have no way to detect the link going down.
> > 
> > Yes I also saw this and didn't understand it. When I clear the force bit
> > it will be set back to 1 again when AN is enabled. I thought this might
> > be a bug of the controller.
> 
> No it isn't, the hardware never changes the value in this register.
> The difference will be because of what I explained previously.
> 
> Because the mvneta and mvpp2 hardware is "weird" in that these two
> registers provide both PCS and MAC functions, we have to deal with
> them in a way that is split between the phylink PCS and MAC
> operations.
> 
> This code was written at a time when all we had was MLO_AN_* and
> none of the PHYLINK_PCS_NEG_* stuff. PCSes got passed the MLO_AN_*
> constants which were the same as what was passed via the MAC
> operations. This made the implementation entirely consistent.
> 
> However, that lead PCS implementers to go off and do their own
> things, so we ended up with a range of different PCS behaviours
> depending on the implementation (because the way people interpreted
> MLO_AN_INBAND, interface mode, and the Autoneg bit in the advertising
> mask was all different.
> 
> So to bring some consistency, I changed the PCS interface to what we
> have now, in the belief that it would later allow us to solve the
> 2500base-X problem.
> 
> However, I'd forgotten about the mvneta/mvpp2 details, but that was
> fine at the time of this change because everything was still
> consistent - we would only ever use PHYLINK_PCS_NEG_OUTBAND or
> PHYLINK_PCS_NEG_NONE for non-MLO_AN_INBAND modes, and
> PHYLINK_PCS_NEG_INBAND_* for interface modes that support inband
> when using MLO_AN_INBAND.
> 
> Now, when trying to solve the 2500base-X problem which needs us to
> use PHYLINK_PCS_NEG_OUTBAND in some situations, this means we can
> end up with MLO_AN_INBAND + PHYLINK_PCS_NEG_OUTBAND, and this is
> what is causing me problems (the link isn't forced up.)
> 
> Conversely, I suspect you have the situation where you have MLO_AN_PHY
> or MLO_AN_FIXED being passed to the mac_config() and mac_link_up()
> operations, but the PCS is being configured for a different mode.
> 
> I am wondering whether we should at the very least move the
> configuration of the control register 0 and 2 to the pcs_config()
> method so at least that's consistent with the PHYLINK_PCS_NEG_*
> value passed to the PCS and thus the values programmed into the
> autoneg config register. However, that still leaves a big hole in
> how we handle the link forcing - which is necessary if inband AN
> is disabled (in other words, if we need to read the link status
> from the PHY as is done in MLO_AN_PHY mode.)
> 

Now I got it, thanks a lot for the explanation. So the issue is that
MLO_AN_INBAND + PHYLINK_PCS_NEG_OUTBAND is happening in my use case and
therefore the link is not forced up because for that MLO_AN_PHY would be
needed. I will also try to think about it.

Thanks,
Stefan