Re: [PATCH net-next] net: pcs: lynxi: fully reconfigure if link is down

[Daniel Golle <daniel@makrotopia.org>]

Hi Russell,

On Thu, Aug 17, 2023 at 02:03:40PM +0100, Russell King (Oracle) wrote:
> On Thu, Aug 17, 2023 at 01:04:06PM +0100, Daniel Golle wrote:
> > On MT7988 When switching from 10GBase-R/5GBase-R/USXGMII to one of the
> > interface modes provided by mtk-pcs-lynxi we need to make sure to
> > always perform a full configuration of the PHYA.
> > As the idea behind not doing that was mostly to prevent an existing link
> > going down without any need for it to do so. Hence we can just always
> > perform a full confinguration in case the link is down.
> 
> And this is racy - because in the case with inband signalling, the link
> can come up between reading the status and acting on it. It could even
> be already up, but the link status indicates it is not. Lastly, reading
> the BMSR has side effects: the link status bit latches low until a read.
> 
> Basically, do not read the BMSR here, it's buggy to read it any place
> other than pcs_get_state.
> 
> I think what we need to do instead are:
> 
> 1) mtk_mac_select_pcs() returns the SGMII PCS or NULL. Presumably this
>    is the driver which supports 10GBase-R/5GBase-R/USXGMII, and thus
>    this returns NULL for 10GBase-R/5GBase-R/USXGMII.
> 
>    Phylink doesn't cater for mac_select_pcs() returning non-NULL for
>    some modes and NULL for others, mainly because the presence of a PCS
>    _used_ to cause phylink to change its behaviour (see
>    https://lore.kernel.org/netdev/YZRLQqLblRurUd4V@shell.armlinux.org.uk/).
>    That has now changed (we've got rid of the legacy stuff at last!) so
>    there is no technical reason not to now allow that.
> 
>    Vladimir did have some arguments for not allowing it when we had the
>    phylink_set_pcs() interface:
>    https://lore.kernel.org/netdev/20211123181515.qqo7e4xbuu2ntwgt@skbuf/
>    I'm assuming that your requirement now provides sufficient
>    justification for allowing this.
> 
>    There is one bug that does need fixing first:
>    phylink_change_inband_advert() checks pl->pcs->neg_mode without
>    first checking whether pl->pcs is non-NULL.
> 
>    To allow this, phylink_major_config() needs:
> 
>    	pcs_changed = pcs && pl->pcs != pcs;
> 
>    to become:
> 
>    	pcs_changed = pl->pcs != pcs;
> 
> 2) with (1) solved, there are a couple of callbacks that can be used to
>    solve this - I think pcs_disable() is the one you want, which will
>    be called when we switch to a mode where _this_ PCS will no longer
>    be used (thus you can reset mpcs->interface to _NA, ready for when
>    it is next brought into use.)
> 
> Would that work for you?

Yes, and that actually even makes things much easier.
The case of mtk_mac_select_pcs() returning NULL is not even relevant:
In case of the interface being 10GBase-R, 5GBase-R or USXGMII
mtk_mac_select_pcs() will return a pointer to the USXGMII PCS instance[1].

Hence simply implementing .pcs_disabled already resolves the issue.
I will post a patch doing that instead which replaces this patch.


Thank you for reviewing!


Daniel


[1]: https://github.com/dangowrt/linux/commit/c81d14e214c8bbbab81fd6d6d49e6f7b87015e1e#diff-6f8a141b53de471a9fe00ac68f8c82b9dda3bad057c160327d6bfe1b0b9c8b23R550