netdev - 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: <AM6PR08MB4376669AFBED3CF8E75097D1FFC19@AM6PR08MB4376.eurprd08.prod.outlook.com>
Date:   Mon, 16 Jan 2023 09:51:20 +0000
From:   Pierluigi Passaro <pierluigi.p@...iscite.com>
To:     Lars-Peter Clausen <lars@...afoo.de>, Andrew Lunn <andrew@...n.ch>
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 3:41 AM Lars-Peter Clausen <lars@...afoo.de> wrote:
> On 1/15/23 15:55, Andrew Lunn 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?
> >
> > To some extent, the ID is only used to find the driver. A C45 device
> > has a lot of ID register, 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;
> >
> I think part of the problem is that for C45 there are a few other fields
> that get populated by the ID detection, such as devices_in_package and
> mmds_present. Is this something we can do after running the PHY drivers
> probe function? Or is it too late at that point?
>
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