netdev - Re: [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver

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: <20210114183243.4kse75ksw3u7h4uz@skbuf>
Date:   Thu, 14 Jan 2021 20:32:43 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     George McCollister <george.mccollister@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Rob Herring <robh@...nel.org>,
        "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        "open list:OPEN FIRMWARE AND..." <devicetree@...r.kernel.org>
Subject: Re: [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x
 driver

On Thu, Jan 14, 2021 at 10:53:16AM -0600, George McCollister wrote:
> On Wed, Jan 13, 2021 at 7:57 PM Vladimir Oltean <olteanv@...il.com> wrote:
> >
> > What PHY interface types does the switch support as of this patch?
> > No RGMII delay configuration needed?
> >
> 
> Port 0: RMII
> Port 1-3: RGMII
> 
> For RGMII the documentation states:
> "PCB is required to add 1.5 ns to 2.0 ns more delay to the clock line
> than the other lines, unless the other end (PHY) has configurable RX
> clock delay."

Ok, didn't notice that.

> > I've been there too, not the smartest of decisions in the long run. See
> > commit 0b0e299720bb ("net: dsa: sja1105: use detected device id instead
> > of DT one on mismatch") if you want a sneak preview of how this is going
> > to feel two years from now. If you can detect the device id you're
> > probably better off with a single compatible string.
> 
> Previously Andrew said:
> "Either you need to verify the compatible from day one so it is not
> wrong, or you just use a single compatible "arrow,xrs700x", which
> cannot be wrong."
> 
> I did it the first way he suggested, if you would have replied at that
> time to use a single that's the way I would have done it that way.
> 
> If you two can agree I should change it to a single string I'd be
> happy to do so. In my case I need 3 RGMII and only one of the package
> types will fit on the board so there's no risk of changing to one of
> the existing parts. Perhaps this could be an issue if a new part is
> added in the future or on someone else's design.

Ok, if the parts are not pin-compatible I guess the range of potential
issues to deal with may be lower. Don't get me wrong, I don't have a
strong opinion. I'm fine if you ignore this comment and keep the
specific compatibles, I think this is what the Open Firmware document
recommends anyway.

> > > +static int xrs700x_alloc_port_mib(struct xrs700x *dev, int port)
> > > +{
> > > +     struct xrs700x_port *p = &dev->ports[port];
> > > +     size_t mib_size = sizeof(*p->mib_data) * ARRAY_SIZE(xrs700x_mibs);
> >
> > Reverse Christmas tree ordering... sorry.
> 
> The second line uses p so that won't work. I'll change the function to
> use devm_kcalloc like you recommended below and just get rid of
> mib_size.

Yes, if you can get rid of it, that works.
Generally when somebody says "reverse xmas tree" and it's obvious that
there are data dependencies between variables, what they mean to request
is:

	struct xrs700x_port *p = &dev->ports[port];
	size_t mib_size;

	mib_size = sizeof(*p->mib_data) * ARRAY_SIZE(xrs700x_mibs);

> > > diff --git a/drivers/net/dsa/xrs700x/xrs700x_mdio.c b/drivers/net/dsa/xrs700x/xrs700x_mdio.c
> > > new file mode 100644
> > > index 000000000000..4fa6cc8f871c
> > > --- /dev/null
> > > +++ b/drivers/net/dsa/xrs700x/xrs700x_mdio.c
> > > +static int xrs700x_mdio_reg_read(void *context, unsigned int reg,
> > > +                              unsigned int *val)
> > > +{
> > > +     struct mdio_device *mdiodev = context;
> > > +     struct device *dev = &mdiodev->dev;
> > > +     u16 uval;
> > > +     int ret;
> > > +
> > > +     uval = (u16)FIELD_GET(GENMASK(31, 16), reg);
> > > +
> > > +     ret = mdiobus_write(mdiodev->bus, mdiodev->addr, XRS_MDIO_IBA1, uval);
> > > +     if (ret < 0) {
> > > +             dev_err(dev, "xrs mdiobus_write returned %d\n", ret);
> > > +             return ret;
> > > +     }
> > > +
> > > +     uval = (u16)((reg & GENMASK(15, 1)) | XRS_IB_READ);
> >
> > What happened to bit 0 of "reg"?
> 
> From the datasheet:
> "Bits 15-1 of the address on the internal bus to where data is written
> or from where data is read. Address bit 0 is always 0 (because of 16
> bit registers)."
> 
> reg_stride is set to 2.
> "The register address stride. Valid register addresses are a multiple
> of this value."

Ok, clear now.

> > May boil down to preference too, but I don't believe "dev" is a happy
> > name to give to a driver private data structure.
> 
> There are other drivers in the subsystem that do this. If there was a
> consistent pattern followed in the subsystem I would have followed it.
> Trust me I was a bit frustrated with home much time I spent going
> through multiple drivers trying to determine the best practices for
> organization, naming, etc.
> If it's a big let me know and I'll change it.

Funny that you are complaining about consistency in other drivers,
because if I count correctly, out of a total of 22 occurrences of
struct xrs700x variables in yours, 13 are named priv and 9 are named
dev. So you are not even consistent with yourself. But it's not a major
issue either way.