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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Read-modify-write on shared TSN_PORT_STATE_CTRL_OFFSET fails to clear other ports' self-clearing commit bits
--

--- Patch [11]: [PATCH 11/20] net: dsa: xilinx: implement port_stp_state_set ---
commit a493a3188672bc98a878692c597aa8f604b4be1d
Author: Nagadheeraj Rottela <[email protected]>
Subject: net: dsa: xilinx: implement port_stp_state_set

This commit implements the port_stp_state_set callback for the Xilinx TSN
DSA switch. It maps bridge STP states to hardware encodings and applies
them via the Switch Port State Control register.

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

[ ... ]

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

[ ... ]

> +	/* 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);

[Severity: High]
Could this read-modify-write sequence accidentally re-trigger state changes
on other ports?

The register read stores the state of all ports in `val`. Since the
commit bits (EP_PORT_STATUS_CHG_BIT, MAC1_PORT_STATUS_CHG_BIT, and
MAC2_PORT_STATUS_CHG_BIT) are self-clearing, it is possible for another
port's commit bit to be read as 1.

If the hardware asynchronously clears that other port's bit to 0 just
before this write back, does writing the 1 back cause an unintended
0-to-1 transition and a spurious state machine commit on the unaffected
port?

Should the commit bits for all other ports be masked out from `val`
before writing the value back to TSN_PORT_STATE_CTRL_OFFSET?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11