Messages in this thread | | | Date | Thu, 17 Aug 2023 16:11:48 +0100 | From | Daniel Golle <> | Subject | Re: [PATCH net-next] net: pcs: lynxi: fully reconfigure if link is down |
| |
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
| |