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
| ||
|
Message-ID: <ZN44tDZydQfFqZqo@makrotopia.org> Date: Thu, 17 Aug 2023 16:11:48 +0100 From: Daniel Golle <daniel@...rotopia.org> To: "Russell King (Oracle)" <linux@...linux.org.uk> Cc: Alexander Couzens <lynxis@...0.eu>, Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Matthias Brugger <matthias.bgg@...il.com>, AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org 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
Powered by blists - more mailing lists