netdev - Re: [PATCH net-next 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]

Date:   Mon, 23 Nov 2020 14:43:01 -0600
From:   George McCollister <george.mccollister@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        "open list:OPEN FIRMWARE AND..." <devicetree@...r.kernel.org>
Subject: Re: [PATCH net-next 2/3] net: dsa: add Arrow SpeedChips XRS700x driver

On Fri, Nov 20, 2020 at 5:24 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> Hi George
>
> > > > +static void xrs700x_port_stp_state_set(struct dsa_switch *ds, int port,
> > > > +                                    u8 state)
> > > > +{
> > > > +     struct xrs700x *priv = ds->priv;
> > > > +     unsigned int val;
> > > > +
> > > > +     switch (state) {
> > > > +     case BR_STATE_DISABLED:
> > > > +             val = XRS_PORT_DISABLED;
> > > > +             break;
> > > > +     case BR_STATE_LISTENING:
> > > > +             val = XRS_PORT_DISABLED;
> > > > +             break;
> > >
> > > No listening state?
> >
> > No, just forwarding, learning and disabled. See:
> > https://www.flexibilis.com/downloads/xrs/SpeedChip_XRS7000_3000_User_Manual.pdf
> > page 82.
> >
> > >
> > > > +     case BR_STATE_LEARNING:
> > > > +             val = XRS_PORT_LEARNING;
> > > > +             break;
> > > > +     case BR_STATE_FORWARDING:
> > > > +             val = XRS_PORT_FORWARDING;
> > > > +             break;
> > > > +     case BR_STATE_BLOCKING:
> > > > +             val = XRS_PORT_DISABLED;
> > > > +             break;
> > >
> > > Hum. What exactly does XRS_PORT_DISABLED mean? When blocking, it is
> > > expected you can still send/receive BPDUs.
> >
> > Datasheet says: "Disabled. Port neither learns MAC addresses nor forwards data."
>
> I think you need to do some testing here. Put the device into a loop
> with another switch, the bridge will block a port, and see if it still
> can send/receive BPDUs on the blocked port.
>
> If it cannot send/receive BPDUs, it might get into an oscillating
> state. They see each other via BPDUs, decide there is a loop, and
> block a port. The BPDUs stop, they think the loop has been broken and
> so unblock. They see each other via BPUS, decide there is a loop,...

Yeah, this is messed up. The switch doesn't seem to pass up BPDUs in
either disabled or learning mode, only forward mode.
Can I just replace .port_stp_state_set with .port_enable (setting
switch port to forward mode) and .port_disable (setting switch port to
disabled mode)? I don't see any other way around this. It looks like
rtl8366rb.c also has no .port_stp_state_set.

>
> > > > +static int xrs700x_i2c_reg_read(void *context, unsigned int reg,
> > > > +                             unsigned int *val)
> > > > +{
> > > > +     int ret;
> > > > +     unsigned char buf[4];
> > > > +     struct device *dev = context;
> > > > +     struct i2c_client *i2c = to_i2c_client(dev);
> > > > +
> > > > +     buf[0] = reg >> 23 & 0xff;
> > > > +     buf[1] = reg >> 15 & 0xff;
> > > > +     buf[2] = reg >> 7 & 0xff;
> > > > +     buf[3] = (reg & 0x7f) << 1;
> > > > +
> > > > +     ret = i2c_master_send(i2c, buf, sizeof(buf));
> > >
> > > Are you allowed to perform transfers on stack buffers? I think any I2C
> > > bus driver using DMA is going to be unhappy.
> >
> > It should be fine. See the following file, there is a good write up about this:
> > See Documentation/i2c/dma-considerations.rst
>
> O.K, thanks for the pointer.
>
> > > > +static const struct of_device_id xrs700x_i2c_dt_ids[] = {
> > > > +     { .compatible = "arrow,xrs7003" },
> > > > +     { .compatible = "arrow,xrs7004" },
> > > > +     {},
> > >
> > > Please validate that the compatible string actually matches the switch
> > > found. Otherwise we can get into all sorts of horrible backward
> > > compatibility issues.
> >
> > Okay. What kind of compatibility issues? Do you have a hypothetical
> > example? I guess I will just use of_device_is_compatible() to check.
>
> Since it currently does not matter, you can expect 50% of the boards
> to get it wrong. Sometime later, you find some difference between the
> two, you want to add additional optional properties dependent on the
> compatible string. But that is made hard, because 50% of the boards
> are broken, and the compatible string is now worthless.
>
> 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.

okay I'll make sure an error is returned if the detected switch does
not match compatible.

>
>   Andrew