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.
>
> > + */
> > + regmap_update_bits(priv->regmap, XRS_ETH_ADDR_CFG(port, 0), 1, bpdus);
> > +
> > + dev_dbg_ratelimited(priv->dev, "%s - port: %d, state: %u, val: 0x%x\n",
> > + __func__, port, state, val);
> > +}
