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

[Vladimir Oltean <olteanv@gmail.com>]

On Fri, Jan 15, 2021 at 09:30:29AM -0600, George McCollister wrote:
> On Thu, Jan 14, 2021 at 4:48 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > On Thu, Jan 14, 2021 at 01:57:33PM -0600, George McCollister wrote:
> [snip]
> >
> > Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> >
> > This driver is good to go, just one small nitpick below, you can fix it
> > up afterwards if you want.
> >
> > > +static void xrs700x_port_stp_state_set(struct dsa_switch *ds, int port,
> > > +                                    u8 state)
> > > +{
> > > +     struct xrs700x *priv = ds->priv;
> > > +     unsigned int bpdus = 1;
> > > +     unsigned int val;
> > > +
> > > +     switch (state) {
> > > +     case BR_STATE_DISABLED:
> > > +             bpdus = 0;
> > > +             fallthrough;
> > > +     case BR_STATE_BLOCKING:
> > > +     case BR_STATE_LISTENING:
> > > +             val = XRS_PORT_DISABLED;
> > > +             break;
> > > +     case BR_STATE_LEARNING:
> > > +             val = XRS_PORT_LEARNING;
> > > +             break;
> > > +     case BR_STATE_FORWARDING:
> > > +             val = XRS_PORT_FORWARDING;
> > > +             break;
> > > +     default:
> > > +             dev_err(ds->dev, "invalid STP state: %d\n", state);
> > > +             return;
> > > +     }
> > > +
> > > +     regmap_fields_write(priv->ps_forward, port, val);
> > > +
> > > +     /* Enable/disable inbound policy added by xrs700x_port_add_bpdu_ipf()
> > > +      * which allows BPDU forwarding to the CPU port when the front facing
> > > +      * port is in disabled/learning state.
> >                       ~~~~~~~~
> > You probably mean blocking. When the port is in BR_STATE_DISABLED, you
> > set bpdus = 1, which makes sense.
> 
> That doesn't sound quite right, let me try to explain this differently
> and you can tell me if it makes more sense. If so I'll update it, add
> the reviewed-bys and post v6:
> 
> Enable/disable inbound policy added by xrs700x_port_add_bpdu_ipf()
> which allows BPDU forwarding to the CPU port. The policy must be
> enabled when the front facing port is in BLOCKING, LISTENING and
> LEARNING BR_STATEs since the switch doesn't otherwise forward BPDUs
> when the port is set to XRS_PORT_DISABLED and XRS_PORT_LEARNING.

No, that was clear in the first place. It is even intelligible with the
comment as-is, otherwise I would have asked you what you mean. You don't
need to resent. I made a mistake when I said "you set bpdus = 1", you
obviously set bpdus = 0 for BR_STATE_DISABLED and 1 for everything else.
I think I should just get more sleep before giving review comments.