linux-kernel - Re: [PATCH] net: mdio: force deassert MDIO reset signal

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]

Message-ID: <AM6PR08MB43767C522EDAAF962B3AA73AFFC19@AM6PR08MB4376.eurprd08.prod.outlook.com>
Date:   Mon, 16 Jan 2023 08:39:31 +0000
From:   Pierluigi Passaro <pierluigi.p@...iscite.com>
To:     Andrew Lunn <andrew@...n.ch>, Lars-Peter Clausen <lars@...afoo.de>
CC:     Pierluigi Passaro <pierluigi.passaro@...il.com>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Eran Matityahu <eran.m@...iscite.com>,
        Nate Drude <Nate.D@...iscite.com>,
        Francesco Ferraro <francesco.f@...iscite.com>
Subject: Re: [PATCH] net: mdio: force deassert MDIO reset signal

On Mon, Jan 16, 2023 at 12:55 AM Andrew Lunn <andrew@...n.ch> wrote:
> > Specifying the ID as part of the compatible string works for clause 22 PHYs,
> > but for clause 45 PHYs it does not work. The code always wants to read the
> > ID from the PHY itself. But I do not understand things well enough to tell
> > whether that's a fundamental restriction of C45 or just our implementation
> > and the implementation can be changed to fix it.
> >
> > Do you have some thoughts on this?
>
> Do you have more details about what goes wrong? Which PHY driver is
> it? What compatibles do you put into DT for the PHY?
>
We use both AR8033 and ADIN1300: these are 10/100/1000 PHYs.
They are both probed after the MDIO bus, but skipped if the reset was
asserted while probing the MDIO bus.
However, for iMX6UL and iMX7 we use C22, not C45.
>
> To some extent, the ID is only used to find the driver. A C45 device
> has a lot of ID registers, and all of them are used by phy_bus_match()
> to see if a driver matches. So you need to be careful which ID you
> pick, it needs to match the driver.
>
> It is the driver which decides to use C22 or C45 to talk to the PHY.
> However, we do have:
>
> static int phy_probe(struct device *dev)
> {
> ...
>         else if (phydev->is_c45)
>                 err = genphy_c45_pma_read_abilities(phydev);
>         else
>                 err = genphy_read_abilities(phydev);
>
> so it could be a C45 PHY probed using an ID does not have
> phydev->is_c45 set, and so it looks in the wrong place for the
> capabilities. Make sure you also have the compatible
> "ethernet-phy-ieee802.3-c45" which i think should cause is_c45 to be
> set.
>
> There is no fundamental restriction that i know of here, it probably
> just needs somebody to debug it and find where it goes wrong.
>
> Ah!
>
> int fwnode_mdiobus_register_phy(struct mii_bus *bus,
>                                 struct fwnode_handle *child, u32 addr)
> {
> ...
>         rc = fwnode_property_match_string(child, "compatible",
>                                           "ethernet-phy-ieee802.3-c45");
>         if (rc >= 0)
>                 is_c45 = true;
>
>         if (is_c45 || fwnode_get_phy_id(child, &phy_id))
>                 phy = get_phy_device(bus, addr, is_c45);
>         else
>                 phy = phy_device_create(bus, addr, phy_id, 0, NULL);
>
>
> So compatible "ethernet-phy-ieee802.3-c45" results in is_c45 being set
> true. The if (is_c45 || is then true, so it does not need to call
> fwnode_get_phy_id(child, &phy_id) so ignores whatever ID is in DT and
> asks the PHY.
>
> Try this, totally untested:
>
> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> index b782c35c4ac1..13be23f8ac97 100644
> --- a/drivers/net/mdio/fwnode_mdio.c
> +++ b/drivers/net/mdio/fwnode_mdio.c
> @@ -134,10 +134,10 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
>         if (rc >= 0)
>                 is_c45 = true;
>
> -       if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> +       if (fwnode_get_phy_id (child, &phy_id))
>                 phy = get_phy_device(bus, addr, is_c45);
>         else
> -               phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> +               phy = phy_device_create(bus, addr, phy_id, is_c45, NULL);
>         if (IS_ERR(phy)) {
>                 rc = PTR_ERR(phy);
>                 goto clean_mii_ts;
>
>
>      Andrew
Unfortunately the above doesn't change the condition: this problem is not C45 specific.
The call fwnode_get_phy_id just parses the device tree and always passes.
This is a sample device tree
https://github.com/varigit/linux-imx/blob/5.15-2.0.x-imx_var01/arch/arm64/boot/dts/freescale/imx8qm-var-spear.dtsi#L168-L219