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

On Fri, Jan 15, 2021 at 09:30:29AM -0600, George McCollister wrote:

On Thu, Jan 14, 2021 at 4:48 PM Vladimir Oltean [off-list ref] wrote:

quoted

On Thu, Jan 14, 2021 at 01:57:33PM -0600, George McCollister wrote:

[snip]

quoted

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.

quoted

+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.