Re: [PATCH 11/20] net: dsa: xilinx: implement port_stp_state_set

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: xilinx: implement port_stp_state_set

This adds a .port_stp_state_set callback that maps the five bridge STP
states onto the 3-bit per-port fields of the Switch Port State Control
register at +0x004C, using a self-clearing commit bit plus
readl_poll_timeout() for the acknowledge. setup() puts the CPU port in
FORWARDING and the user ports in DISABLED, and teardown() puts the ports
back to DISABLED.

A few questions on the new code below.

First, on the commit message itself:

> The bridge STP machine does not manage the CPU port. Its forwarding
> state does not change after setup().

[Severity: Low]
Is this accurate?  The teardown() added by this same patch writes DISABLED
to the CPU port:

	xlnx_tsn_set_port_state(sw, XLNX_TSN_CPU_PORT, TSN_PORT_STATE_DISABLED);

And because the driver now provides .port_stp_state_set, the DSA core also
drives the CPU port state on its own:

  dsa_port_setup()   [DSA_PORT_TYPE_CPU] -> dsa_port_enable(dp, NULL)
    -> dsa_port_enable_rt() -> dsa_port_set_state_now(dp, BR_STATE_FORWARDING, false)

  dsa_port_teardown() [DSA_PORT_TYPE_CPU] -> dsa_port_disable(dp)
    -> dsa_port_disable_rt() -> dsa_port_set_state_now(dp, BR_STATE_DISABLED, false)

So the CPU port is written FORWARDING twice at bring-up (making the
explicit write in setup() redundant) and DISABLED from two paths at
unbind.  Could the changelog be reworded to say the core drives the CPU
port state too, and that teardown disables it?

> diff --git a/drivers/net/dsa/xilinx/xilinx_tsn.c b/drivers/net/dsa/xilinx/xilinx_tsn.c
> index 6b285c528724..020b1622670c 100644
> --- a/drivers/net/dsa/xilinx/xilinx_tsn.c
> +++ b/drivers/net/dsa/xilinx/xilinx_tsn.c

[ ... ]

> @@ -11,6 +15,86 @@
> +static int xlnx_tsn_set_port_state(struct xlnx_tsn *sw, int port,
> +				   enum tsn_port_state state)
> +{
> +	u32 chg_bit, mask, val, reg;
> +	int err;
> +
> +	err = xlnx_tsn_port_state_bits(port, &mask, &chg_bit);
> +	if (err)
> +		return err;
> +
> +	err = xlnx_tsn_switch_status_ready(sw);
> +	if (err) {
> +		dev_err(sw->dev, "port %d: switch not ready for state change\n",
> +			port);
> +		return err;
> +	}
> +
> +	/* Bit won't re-arm if a previous change never cleared it. */
> +	val = sw_ior(sw, TSN_PORT_STATE_CTRL_OFFSET);
> +	if (val & chg_bit) {
> +		dev_err(sw->dev, "port %d: previous state change still pending\n",
> +			port);
> +		return -EBUSY;
> +	}
> +
> +	val &= ~mask;
> +	val |= (state << __ffs(mask)) & mask;
> +	val |= chg_bit;
> +	sw_iow(sw, TSN_PORT_STATE_CTRL_OFFSET, val);
> +
> +	err = readl_poll_timeout(sw->sw_base + TSN_PORT_STATE_CTRL_OFFSET, reg,
> +				 !(reg & chg_bit), TSN_SW_POLL_DELAY_US,
> +				 TSN_SW_POLL_TIMEOUT_US);
> +	if (err) {
> +		dev_err(sw->dev, "port %d: state change ack timed out\n", port);
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Can this timeout path wedge the port permanently?  When the 5 ms ack poll
expires, xlnx_tsn_set_port_state() returns -ETIMEDOUT with chg_bit still
set in TSN_PORT_STATE_CTRL, and it neither clears the bit nor re-drives
the transfer.  Per the comment right above the precondition check, the bit
will not re-arm, so every later call for that port hits:

	val = sw_ior(sw, TSN_PORT_STATE_CTRL_OFFSET);
	if (val & chg_bit) {
		dev_err(sw->dev, "port %d: previous state change still pending\n",
			port);
		return -EBUSY;
	}

That includes a subsequent probe attempt, since xlnx_tsn_setup()
propagates the error and aborts registration.  Meanwhile the state field
and the 0->1 commit edge have already been written, so the hardware may
still latch the new encoding.

Would it make sense to write the commit bit back to 0 (or otherwise
re-drive it) on the timeout path, and to clear a stale commit bit once
before the first programming attempt in setup()?

The exact behaviour of the commit bit is not visible from the tree, so
this rests on the assumption stated in the comment.

> @@ -18,19 +102,83 @@ static enum dsa_tag_protocol xlnx_tsn_get_tag_protocol(struct dsa_switch *ds,
> +static void xlnx_tsn_port_stp_state_set(struct dsa_switch *ds, int port,
> +					u8 state)
> +{
> +	struct xlnx_tsn *sw = ds->priv;
> +	enum tsn_port_state hw_state;
> +
> +	switch (state) {
> +	case BR_STATE_DISABLED:
> +		hw_state = TSN_PORT_STATE_DISABLED;
> +		break;
> +	case BR_STATE_BLOCKING:
> +		hw_state = TSN_PORT_STATE_BLOCKING;
> +		break;
> +	case BR_STATE_LISTENING:
> +		hw_state = TSN_PORT_STATE_LISTENING;
> +		break;
> +	case BR_STATE_LEARNING:
> +		hw_state = TSN_PORT_STATE_LEARNING;
> +		break;
> +	case BR_STATE_FORWARDING:
> +		hw_state = TSN_PORT_STATE_FORWARDING;
> +		break;
> +	default:
> +		dev_warn(sw->dev, "port %d: unsupported STP state %u\n",
> +			 port, state);
> +		return;
> +	}
> +
> +	xlnx_tsn_set_port_state(sw, port, hw_state);
> +}

[Severity: High]
Does this bridge MAC1 and MAC2 in hardware even when the two netdevs are
not bridged?  The DSA core requests FORWARDING for any user port that is
not bridge-offloaded, on every ndo_open:

net/dsa/port.c:dsa_port_enable_rt() {
	...
	if (!dp->bridge)
		dsa_port_set_state_now(dp, BR_STATE_FORWARDING, false);
	...
}

That reaches the BR_STATE_FORWARDING case above and programs
TSN_PORT_STATE_FORWARDING on the MAC port.  The ops table below adds no
port_bridge_join/leave and the driver programs no forwarding vector,
port-based VLAN, or any other isolation register (the only registers
touched are TSN_SW_STATUS_OFFSET and TSN_PORT_STATE_CTRL_OFFSET).

So after:

	ip link set <mac1port> up
	ip link set <mac2port> up

both switch ports sit in FORWARDING in the same flooding domain, and
frames received on one wire port are forwarded out the other in hardware
without reaching the host stack.

Documentation/networking/dsa/dsa.rst says of setup():

  "This function is also expected to properly configure the switch to
   separate all network interfaces from each other, that is, they should be
   isolated by the switch hardware itself"

Should setup() program a per-port forwarding domain (for example a
port-based VLAN allowing only the CPU port and that port) so standalone
ports stay isolated?

[Severity: Medium]
The return value of xlnx_tsn_set_port_state() is discarded here.  Since
.port_stp_state_set is a void callback, an -EBUSY or -ETIMEDOUT failure is
invisible to the bridge, which then records e.g. BLOCKING while the
hardware can still be FORWARDING.  Would at least a dev_err() on failure
here help make that divergence visible?

>  static int xlnx_tsn_setup(struct dsa_switch *ds)
>  {
> +	struct xlnx_tsn *sw = ds->priv;
> +	struct dsa_port *dp;
> +	int ret;
> +
> +	if (!dsa_is_user_port(ds, XLNX_TSN_PORT_MAC1) ||
> +	    !dsa_is_user_port(ds, XLNX_TSN_PORT_MAC2))
> +		return dev_err_probe(sw->dev, -EINVAL,
> +				     "both MAC1 and MAC2 must be enabled as switch ports\n");

[Severity: Low]
This adds a probe-time topology requirement that isn't mentioned in the
changelog and isn't expressed in the binding.  In
Documentation/devicetree/bindings/net/xlnx,tsn-endpoint-ethernet-mac.yaml
the wire ports are described only as:

	patternProperties:
	  "^port@[1-2]$":

with no "required" entry, so a device tree describing only port@0 and
port@1 validates against the schema but fails dsa_register_switch() with
-EINVAL, taking down the whole DSA tree for that conduit.

Should the binding mark port@1 and port@2 as required, or should the
driver tolerate a single wire port?

> +
> +	/* CPU port stays in FORWARDING so host traffic always flows.
> +	 * User ports start in DISABLED and transition from there under
> +	 * bridge STP control.
> +	 */

[Severity: Low]
Is the second half of this comment correct?  The DSA core replaces the
DISABLED encoding on the first ifup, with no bridge STP event involved:

net/dsa/port.c:dsa_port_enable_rt() {
	if (!dp->bridge)
		dsa_port_set_state_now(dp, BR_STATE_FORWARDING, false);
}

net/dsa/port.c:dsa_port_disable_rt() {
	if (!dp->bridge)
		dsa_port_set_state_now(dp, BR_STATE_DISABLED, false);
}

> +	ret = xlnx_tsn_set_port_state(sw, XLNX_TSN_CPU_PORT,
> +				      TSN_PORT_STATE_FORWARDING);
> +	if (ret)
> +		return ret;
> +
> +	dsa_switch_for_each_user_port(dp, ds) {
> +		ret = xlnx_tsn_set_port_state(sw, dp->index,
> +					      TSN_PORT_STATE_DISABLED);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }

[Severity: Low]
Should these error returns undo the CPU port write?  The endpoint port has
already been committed to FORWARDING when the user-port loop runs, and if
xlnx_tsn_set_port_state() returns -EBUSY or -ETIMEDOUT for a user port the
fabric is left half-programmed.

The core does not call teardown() when setup() fails:

net/dsa/dsa.c:dsa_switch_setup() {
	...
	err = ds->ops->setup(ds);
	if (err < 0)
		goto unregister_notifier;
	...
}

so probe fails, devm releases the register windows and frees struct
xlnx_tsn, but the endpoint port stays in FORWARDING with no driver bound.
Mirroring xlnx_tsn_teardown() on setup()'s error paths would keep this
symmetric.

>  static void xlnx_tsn_teardown(struct dsa_switch *ds)
>  {
> +	struct xlnx_tsn *sw = ds->priv;
> +	struct dsa_port *dp;
> +
> +	dsa_switch_for_each_user_port(dp, ds)
> +		xlnx_tsn_set_port_state(sw, dp->index, TSN_PORT_STATE_DISABLED);
> +
> +	xlnx_tsn_set_port_state(sw, XLNX_TSN_CPU_PORT, TSN_PORT_STATE_DISABLED);
>  }
>  
>  static const struct dsa_switch_ops xlnx_tsn_switch_ops = {
>  	.get_tag_protocol	= xlnx_tsn_get_tag_protocol,
>  	.setup			= xlnx_tsn_setup,
>  	.teardown		= xlnx_tsn_teardown,
> +	.port_stp_state_set	= xlnx_tsn_port_stp_state_set,
>  };

[ ... ]