On Fri, Nov 20, 2020 at 5:24 PM Andrew Lunn <andrew@xxxxxxx> 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
