Received-SPF: pass (google.com: domain of linux-kernel+bounces-133536-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 5 Apr 2024 19:45:56 +0000
From: Daniel Machon <daniel.machon@microchip.com>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
CC: "David S. Miller" <davem@davemloft.net>, Eric Dumazet
	<edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
	<pabeni@redhat.com>, Lars Povlsen <lars.povlsen@microchip.com>, "Steen
 Hegelund" <Steen.Hegelund@microchip.com>, <UNGLinuxDriver@microchip.com>,
	Bjarni Jonasson <bjarni.jonasson@microchip.com>, <netdev@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net] net: sparx5: fix reconfiguration of PCS on link mode
 change
Message-ID: <20240405194556.ngto6ddgf5r2br33@DEN-DL-M70577>
References: <20240405-link-mode-reconfiguration-fix-v1-1-c1480bc2346a@microchip.com>
 <Zg/Qf4P5gAzRUYuK@shell.armlinux.org.uk>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Zg/Qf4P5gAzRUYuK@shell.armlinux.org.uk>

Hi Russel,

> > It was observed that the PCS would be misconfigured on link mode change,
> > if the negotiated link mode went from no-inband capabilities to in-band
> > capabilities. This bug appeared after the neg_mode change of phylink [1],
> > but is really due to the wrong config being used when reconfiguring the PCS.
> 
> I don't see how the change you point to could have changed the
> behaviour. Old code:
> 
>         conf.inband = phylink_autoneg_inband(mode);
>         conf.autoneg = phylink_test(advertising, Autoneg);
> 
> New code:
> 
>         conf.inband = neg_mode == PHYLINK_PCS_NEG_INBAND_DISABLED ||
>                       neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED;
>         conf.autoneg = neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED;
> 
> where, for SGMII or 802.3z modes, neg_mode will be one of
> PHYLINK_PCS_NEG_INBAND_DISABLED or PHYLINK_PCS_NEG_INBAND_ENABLED if
> phylink_autoneg_inband(mode) is true, or PHYLINK_PCS_NEG_OUTBAND if
> not.

For inband/10GBase-R, conf.inband would be true, prior to the
phylink change. This is fine since conf.inband is only used when
configuring low speed devices.

After the change, conf.inband will be false when opting in through
phylink_pcs.net_mode = true. This causes the SGMII to be misconfigured,
since the inband parameter from the old config is used, and that
parameter is actually used for configuring SGMII..

TBH the commit description is inadequate at best. I will revise it in a
v2.

> 
> It does change conf.autoneg slightly in that this will always be true
> for SGMII, but will only be true for Autoneg + 802.3z modes.
> 
> As far as your code change goes, it looks correct to me, but I think
> it's fixing a bug that goes back long before the commit you have
> identified.

Just to be clear - by commit do you mean the phylink change or the commit
referenced in the fixes tag?

> 
> However, I think there's another issue here which is more relevant to
> the problem you describe in your commit message. If you look at
> port_conf_has_changed(), you will notice that it fails to compare
> conf.inband, and thus fails to notice any change in the setting of
> that configuration item. This will result in sparx5_port_pcs_set()
> not even being called if only conf.inband changes state.
> 
> Thus, changing from in-band to out-of-band or vice versa won't have
> any effect if this is the only change that occurs, and this also
> exists prior to my change.

Yes. I agree this is an issue that deserves a fix - separate from this
one I would think.

> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!