Re: [PATCH v3 phy-next 7/8] soc: fsl: guts: implement the RCW override procedure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] SerDes block index validation incorrectly uses `>=` instead of `>`, rejecting the highest valid SerDes index and breaking configuration.
- [High] SoC-specific GENMASK macros execute undefined behavior via negative shifts when `lane >= 4`.
- [High] Bulk RCW override initialization clobbers active configurations on uninitialized lanes.
- [Medium] Non-atomic read-modify-write on shared state causes lost lane initializations.
- [Medium] Extremely short 1 microsecond timeout for hardware register state reflection causes spurious failures.

Pre-existing issues:
- [High] Dereferencing `soc_dev_attr` on the error path causes a kernel panic.
--

--- Patch [7]: [PATCH v3 phy-next 7/8] soc: fsl: guts: implement the RCW override procedure ---
commit a1f1c62bf1e452bc97a7f348215d8401bd095833
Author: Ioana Ciornei <ioana.ciornei@nxp.com>
soc: fsl: guts: implement the RCW override procedure

This commit adds support for the RCW override procedure to enable runtime
reconfiguration of SerDes lane protocols through the DCFG DCSR space.
It exports functions to notify boot time modes, validate protocol changes,
and execute the RCW procedure on specific SoCs.

> diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c
> index b9973ca3a440d..4d49d1b953007 100644
> --- a/drivers/soc/fsl/guts.c
> +++ b/drivers/soc/fsl/guts.c
[ ... ]
> @@ -210,6 +210,13 @@ static int fsl_guts_rcw_rmw(int offset, u32 val, u32 mask)
>  {
>  	u32 rcwcr, rcwsr = fsl_guts_read(&soc.dcfg_ccsr->rcwsr[offset]);
>  
>  	rcwcr = rcwsr & ~mask;
>  	rcwcr |= val;
>  	fsl_guts_write(&soc.dcfg_dcsr->rcwcr[offset], rcwcr);
>  
>  	/* Updates to RCWCR should be visible back in RCWSR */
>  	return read_poll_timeout_atomic(fsl_guts_read, rcwsr, rcwsr == rcwcr,
>  					0, RCW_TIMEOUT_US, false,
>  					&soc.dcfg_ccsr->rcwsr[offset]);
>  }

[Severity: Medium]
Is a 1 microsecond timeout sufficient for cross-domain hardware reflections?

The atomic polling loop strictly uses RCW_TIMEOUT_US set to 1, which might be
too brief for the hardware to propagate the state across clock domains. This
could lead to spurious -ETIMEDOUT failures when configuring SerDes lane modes.

[ ... ]
> +static int
> +fsl_guts_serdes_get_rcw_override(int serdes_idx, int lane,
> +				 enum lynx_lane_mode lane_mode,
> +				 struct fsl_soc_serdes_rcw_override *override)
> +{
> +	const struct fsl_soc_data *soc_data = soc.data;
> +
> +	if (!soc_data)
> +		return -ENODEV;
> +
> +	if (serdes_idx >= soc_data->num_serdes || serdes_idx <= 0)
> +		return -ERANGE;

[Severity: High]
Does this bounds check incorrectly reject the highest valid SerDes index?

If soc_data->num_serdes is 2, a client driver attempting to initialize
serdes_idx 2 will trigger this return path because 2 >= 2 evaluates to true.
Should this use > instead of >= to allow configuring the last SerDes block?

> +
> +	if (lane >= MAX_NUM_LANES || lane < 0)
> +		return -ERANGE;

[Severity: High]
Can this loose bounds check lead to undefined behavior in the SoC-specific
macros?

MAX_NUM_LANES is 8, but macros like LS1088A_RCWSR30_SRDS_PRTCL_S2_LNn
assume a maximum of 4 lanes:

GENMASK(3 + 4 * (3 - (lane)), 4 * (3 - (lane)))

If a PHY driver requests a lane index of 4 or higher on LS1088A, this macro
evaluates to GENMASK(-1, -4), which causes negative bitwise shifts.

[ ... ]
> +int fsl_guts_lane_init(int serdes_idx, int lane, enum lynx_lane_mode lane_mode)
> +{
> +	int err;
> +
> +	err = fsl_guts_lane_validate(serdes_idx, lane, lane_mode);
> +	if (err)
> +		return err;
> +
> +	soc.lane_mode[serdes_idx - 1][lane] = lane_mode;
> +	soc.lanes_initialized[serdes_idx - 1] |= BIT(lane);

[Severity: Medium]
Could concurrent PHY probes cause lost lane initialization status here?

This performs a non-atomic read-modify-write on the shared global state array
without holding a lock. If two PHY instances on the same SerDes controller
probe asynchronously, one update to soc.lanes_initialized might overwrite the
other.

[ ... ]
> +static int ls2088a_serdes_init_rcwcr(int serdes_idx)
> +{
> +	int i, err;
> +	u32 reg;
[ ... ]
> +	/* We need to configure the initial state of all lanes for
> +	 * the SerDes block #1
> +	 */
> +	for_each_set_bit(i, &soc.lanes_initialized[serdes_idx - 1], MAX_NUM_LANES)
> +		if (lynx_lane_mode_uses_xgmii_mac(soc.lane_mode[serdes_idx - 1][i]))
> +			reg |= LS2088A_RCWSR30_SRDS_CLK_SEL_XGMII_Ln_S1(i);
> +
> +	err = fsl_guts_rcw_rmw(29, reg,
> +			       LS2088A_RCWSR30_SRDS_CLK_EN_SEL_XGMII_S1 |
> +			       LS2088A_RCWSR30_SRDS_CLK_SEL_MSK);

[Severity: High]
Does this bulk RCW override initialization clobber active configurations on
uninitialized lanes?

If a client driver for lane A calls fsl_guts_lane_set_mode() before sibling
lane B has initialized, this read-modify-write uses a mask spanning all
lanes (LS2088A_RCWSR30_SRDS_CLK_SEL_MSK). This forcefully writes zeroes
to lane B's configuration because it relies purely on the software state
in soc.lanes_initialized, potentially discarding its bootloader-configured
active state.

[ ... ]
>  static int __init fsl_guts_init(void)
>  {
>  	struct soc_device_attribute *soc_dev_attr = NULL;
[ ... ]
>  err_nomem:
>  	ret = -ENOMEM;
>  err:
>  	kfree(soc_dev_attr->family);
>  	kfree(soc_dev_attr->soc_id);
>  	kfree(soc_dev_attr->revision);
>  	kfree(soc_dev_attr->serial_number);
>  	kfree(soc_dev_attr);

[Severity: High]
This is a pre-existing issue, but can dereferencing soc_dev_attr here cause
a kernel panic on the error path?

If earlier initialization steps fail, such as of_iomap() failing and jumping
to err_nomem before soc_dev_attr is allocated, soc_dev_attr will remain NULL.
The unconditional access to member variables like soc_dev_attr->family will
result in a NULL pointer dereference.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720133642.136324-1-vladimir.oltean@nxp.com?part=7