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@xxxxxxxxx> wrote:
> >
> > On Thu, Jan 14, 2021 at 01:57:33PM -0600, George McCollister wrote:
> [snip]
> >
> > Reviewed-by: Vladimir Oltean <olteanv@xxxxxxxxx>
> >
> > 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.
