linux-kernel - Re: [PATCH net-next v8 6/9] net: phy: c22: migrate to genphy_c45_write_eee

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 24 Feb 2023 21:02:07 +0100
From:   Oleksij Rempel <o.rempel@...gutronix.de>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Woojung Huh <woojung.huh@...rochip.com>,
        Andrew Lunn <andrew@...n.ch>, Arun.Ramadoss@...rochip.com,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, UNGLinuxDriver@...rochip.com,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>, Wei Fang <wei.fang@....com>,
        kernel@...gutronix.de, intel-wired-lan@...ts.osuosl.org,
        Jakub Kicinski <kuba@...nel.org>,
        Vladimir Oltean <olteanv@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [PATCH net-next v8 6/9] net: phy: c22: migrate to
 genphy_c45_write_eee_adv()

On Fri, Feb 24, 2023 at 11:17:24AM -0800, Guenter Roeck wrote:
> On 2/24/23 10:36, Oleksij Rempel wrote:
> > On Fri, Feb 24, 2023 at 09:41:32AM -0800, Guenter Roeck wrote:
> > > On Fri, Feb 24, 2023 at 05:52:13PM +0100, Oleksij Rempel wrote:
> > > > On Fri, Feb 24, 2023 at 08:00:57AM -0800, Guenter Roeck wrote:
> > > > > On 2/23/23 20:53, Oleksij Rempel wrote:
> > > > > > Hallo Guenter,
> > > > > > 
> > > > > > On Thu, Feb 23, 2023 at 08:16:04PM -0800, Guenter Roeck wrote:
> > > > > > > On Thu, Feb 23, 2023 at 07:55:55PM -0800, Guenter Roeck wrote:
> > > > > > > > On Sat, Feb 11, 2023 at 08:41:10AM +0100, Oleksij Rempel wrote:
> > > > > > > > > Migrate from genphy_config_eee_advert() to genphy_c45_write_eee_adv().
> > > > > > > > > 
> > > > > > > > > It should work as before except write operation to the EEE adv registers
> > > > > > > > > will be done only if some EEE abilities was detected.
> > > > > > > > > 
> > > > > > > > > If some driver will have a regression, related driver should provide own
> > > > > > > > > .get_features callback. See micrel.c:ksz9477_get_features() as example.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
> > > > > > > > > Reviewed-by: Andrew Lunn <andrew@...n.ch>
> > > > > > > > 
> > > > > > > > This patch causes network interface failures with all my xtensa qemu
> > > > > > > > emulations. Reverting it fixes the problem. Bisect log is attached
> > > > > > > > for reference.
> > > > > > > > 
> > > > > > > 
> > > > > > > Also affected are arm:cubieboard emulations, with same symptom.
> > > > > > > arm:bletchley-bmc emulations crash. In both cases, reverting this patch
> > > > > > > fixes the problem.
> > > > > > 
> > > > > > Please test this fixes:
> > > > > > https://lore.kernel.org/all/167715661799.11159.2057121677394149658.git-patchwork-notify@kernel.org/
> > > > > > 
> > > > > 
> > > > > Applied and tested
> > > > > 
> > > > > 77c39beb5efa (HEAD -> master) net: phy: c45: genphy_c45_ethtool_set_eee: validate EEE link modes
> > > > > 068a35a8d62c net: phy: do not force EEE support
> > > > > 66d358a5fac6 net: phy: c45: add genphy_c45_an_config_eee_aneg() function
> > > > > ecea1bf8b04c net: phy: c45: use "supported_eee" instead of supported for access validation
> > > > > 
> > > > > on top of
> > > > > 
> > > > > d2980d8d8265 (upstream/master, origin/master, origin/HEAD, local/master) Merge tag 'mm-nonmm-stable-2023-02-20-15-29' of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
> > > > > 
> > > > > No change for xtensa and arm:cubieboard; network interfaces still fail.
> > > > 
> > > > Huh, interesting.
> > > > 
> > > > can you please send me the kernel logs.
> > > > 
> > > There is nothing useful there, or at least I don't see anything useful.
> > > The Ethernet interfaces (sun4i-emac for cubieboard and ethoc for xtensa)
> > > just don't come up.
> > > 
> > > Sample logs:
> > > 
> > > cubieboard:
> > > 
> > > https://kerneltests.org/builders/qemu-arm-v7-master/builds/531/steps/qemubuildcommand/logs/stdio
> > > 
> > > xtensa:
> > > 
> > > https://kerneltests.org/builders/qemu-xtensa-master/builds/2177/steps/qemubuildcommand/logs/stdio
> > > 
> > > and, for completeness, bletchley-bmc:
> > > 
> > > https://kerneltests.org/builders/qemu-arm-aspeed-master/builds/531/steps/qemubuildcommand/logs/stdio
> > > 
> > > Those logs are without the above set of patches, but I don't see a
> > > difference with the patches applied for cubieboard and xtensa. I
> > > started a complete test run (for all emulations) with the patches
> > > applied; that should take about an hour to complete.
> > > I could also add some debug logging, but you'd have to give me
> > > some hints about what to add and where.
> > 
> > OK, interesting. These are emulated PHYs. QEMU seems to return 0 or
> > 0xFFFF on unsupported registers. May be I'm wrong.
> > All EEE read/write accesses depend on initial capability read
> > genphy_c45_read_eee_cap1()
> > 
> > Can you please add this trace:
> > 
> > diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> > index f595acd0a895..67dac9f0e71d 100644
> > --- a/drivers/net/phy/phy-c45.c
> > +++ b/drivers/net/phy/phy-c45.c
> > @@ -799,6 +799,7 @@ static int genphy_c45_read_eee_cap1(struct phy_device *phydev)
> >           * (Register 3.20)
> >           */
> >          val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE);
> > +       printk("MDIO_PCS_EEE_ABLE = 0x%04x", val);
> >          if (val < 0)
> >                  return val;
> > 
> 
> For cubieboard:
> 
> MDIO_PCS_EEE_ABLE = 0x0000
> 
> qemu reports attempts to access unsupported registers.
> 
> I had a look at the Allwinner mdio driver. There is no indication suggesting
> what the real hardware would return when trying to access unsupported registers,
> and the Ethernet controller datasheet is not public.

These are PHY accesses over MDIO bus. Ethernet controller should not
care about content of this operations. But on qemu side, it is implemented as
part of Ethernet controller emulation...

Since MDIO_PCS_EEE_ABLE == 0x0000, phydev->supported_eee should prevent
other EEE related operations. But may be actual phy_read_mmd() went
wrong. It is a combination of simple phy_read/write to different
registers.

> For xtensa:
> 
> MDIO_PCS_EEE_ABLE = 0x0014
> 
> I didn't try to find out what that means.

These will be interpreted as the PHY supports 1000KX and 1000T EEE modes.
Starting from this point all EEE read write operations will be allowed.

> qemu did not report attempts to access unsupported registers.

Hm. What is the best way to proceed? Remove genphy_c45_read_eee_abilities()
out of genphy_read_abilities() and let add it to PHYs known to support
it? Or go deeper and fix QEMU if needed?

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |