netdev - Re: [PATCH 3/3] net: ethernet: ixp4xx: Use OF MDIO bus registration

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: <YH7Oy4F37HsTZYij@lunn.ch>
Date:   Tue, 20 Apr 2021 14:53:31 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     netdev <netdev@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Zoltan HERPAI <wigyori@...0.hu>,
        Raylynn Knight <rayknight@...com>
Subject: Re: [PATCH 3/3] net: ethernet: ixp4xx: Use OF MDIO bus registration

On Tue, Apr 20, 2021 at 10:44:16AM +0200, Linus Walleij wrote:
> On Tue, Apr 20, 2021 at 3:47 AM Andrew Lunn <andrew@...n.ch> wrote:
> 
> > > @@ -1381,25 +1382,12 @@ static struct eth_plat_info *ixp4xx_of_get_platdata(struct device *dev)
> > >       /* NPE ID 0x00, 0x10, 0x20... */
> > >       plat->npe = (val << 4);
> > >
> > > -     phy_np = of_parse_phandle(np, "phy-handle", 0);
> > > -     if (phy_np) {
> > > -             ret = of_property_read_u32(phy_np, "reg", &val);
> > > -             if (ret) {
> > > -                     dev_err(dev, "cannot find phy reg\n");
> > > -                     return NULL;
> > > -             }
> > > -             of_node_put(phy_np);
> > > -     } else {
> > > -             dev_err(dev, "cannot find phy instance\n");
> > > -             val = 0;
> > > -     }
> > > -     plat->phy = val;
> > > -
> >
> > Isn't this code you just added in the previous patch?
> 
> Yep. It's by the token of "one technical step per patch"

I don't actually seeing it being a step, since it is actually broken
and of_phy_get_and_connect() does pretty much everything it should do,
which is what you replace it with.

It is a long time since i converted a platform_data driver to DT. But
i remember trying to fill in the platform_data structure from DT was
often the wrong way to do it. They contain different data, and you
cannot easily map one to the other. So you need to make bigger changes
to the probe function. You have two intermingled code paths, one
dealing with platform_data, and the other using DT.

I've not looked in detail, but i guess my first step would be, cleanly
register the MDIO bus. Second step would be to register the PHY. And
it might need some refactoring patches just to make it easier to
understand.

> > > -     snprintf(phy_id, MII_BUS_ID_SIZE + 3, PHY_ID_FMT,
> > > -              mdio_bus->id, plat->phy);
> > > -     phydev = phy_connect(ndev, phy_id, &ixp4xx_adjust_link,
> > > -                          PHY_INTERFACE_MODE_MII);
> > > +     if (np) {
> > > +             phydev = of_phy_get_and_connect(ndev, np, ixp4xx_adjust_link);
> > > +     } else {
> > > +             snprintf(phy_id, MII_BUS_ID_SIZE + 3, PHY_ID_FMT,
> > > +                      mdio_bus->id, plat->phy);
> >
> > mdiobus_get_phy() and phy_connect_direct() might be better.
> 
> Do you mean for the legacy code path (else clause), or the
> new code path with of_phy_get_and_connect() or both?
> 
> I tried not to change the legacy code in order to not introduce
> regressions, so if I change that I suppose it should be a
> separate patch.

Yes, the legacy code. You don't often see this string parsing
method. And since you have the bus, and the index, you can directly go
to the PHY avoiding it all. A separate patch would be better.

   Andrew