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: <d6cfce0d7210762bf0c4130ea4d59470cbabfb56.camel@redhat.com> Date: Tue, 23 Aug 2022 18:38:00 +0200 From: Paolo Abeni <pabeni@...hat.com> To: Alexander 'lynxis' Couzens <lynxis@...0.eu> Cc: Felix Fietkau <nbd@....name>, John Crispin <john@...ozen.org>, Sean Wang <sean.wang@...iatek.com>, Mark Lee <Mark-MC.Lee@...iatek.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>, Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org, linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org, Daniel Golle <daniel@...rotopia.org> Subject: Re: [PATCH 2/4] net: mediatek: sgmii: ensure the SGMII PHY is powered down on configuration On Tue, 2022-08-23 at 16:17 +0200, Alexander 'lynxis' Couzens wrote: > On Tue, 23 Aug 2022 15:18:31 +0200 > Paolo Abeni <pabeni@...hat.com> wrote: > > > On Sun, 2022-08-21 at 00:45 +0200, Alexander Couzens wrote: > > > The code expect the PHY to be in power down which is only true > > > after reset. Allow changes of the SGMII parameters more than once. > > > > > > Signed-off-by: Alexander Couzens <lynxis@...0.eu> > > > --- > > > drivers/net/ethernet/mediatek/mtk_sgmii.c | 16 +++++++++++++++- > > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c > > > b/drivers/net/ethernet/mediatek/mtk_sgmii.c index > > > a01bb20ea957..782812434367 100644 --- > > > a/drivers/net/ethernet/mediatek/mtk_sgmii.c +++ > > > b/drivers/net/ethernet/mediatek/mtk_sgmii.c @@ -7,6 +7,7 @@ > > > * > > > */ > > > > > > +#include <linux/delay.h> > > > #include <linux/mfd/syscon.h> > > > #include <linux/of.h> > > > #include <linux/phylink.h> > > > @@ -24,6 +25,9 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs > > > *mpcs) { > > > unsigned int val; > > > > > > + /* PHYA power down */ > > > + regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, > > > SGMII_PHYA_PWD); > > > > in mtk_pcs_setup_mode_an() and in mtk_pcs_setup_mode_force() the code > > carefully flips only the SGMII_PHYA_PWD bit. Is it safe to overwrite > > the full register contents? > > I've read out the register without my patch and it's 0x0. The old driver > worked as long the engine came out of reset. > When writing the single bit SGMII_PHYA_PWD (0x10), the register might > end up containing 0x19 and as long 0x9 is in the register the link > doesn't work. > > I've tested the driver with a mt7622 and Daniel Golle tested it with a > mt7986. > > > > > > > + > > > /* Setup the link timer and QPHY power up inside SGMIISYS > > > */ regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER, > > > SGMII_LINK_TIMER_DEFAULT); > > > @@ -36,6 +40,10 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs > > > *mpcs) val |= SGMII_AN_RESTART; > > > regmap_write(mpcs->regmap, SGMSYS_PCS_CONTROL_1, val); > > > > > > + /* Release PHYA power down state > > > + * unknown how much the QPHY needs but it is racy without > > > a sleep > > > + */ > > > + usleep_range(50, 100); > > > > Ouch, this looks fragile, without any related H/W specification. > > The datasheet [1] doesn't say anything about it. I'ven't found a > mediatek SDK which adds a usleep(). It seems they always expect the > SGMII came out of reset and don't change after initial configured. > But without it, it's racy. > > [1] MT7622 Reference Manual, v1.0, 2018-12-19, 1972 pages I see. Since it looks like a new revision of this patchset will be needed - as per Russell comments - I suggest to extend/replace the comments with something more alike this longer description, it will make the future mainteinance simpler. Thanks, Paolo
Powered by blists - more mailing lists