Re: [PATCH v2 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 4 potential issue(s) to consider:
- [High] Failure to clear global pointers on error paths allows exported APIs to operate on dangling I/O pointers.
- [High] The Read-Modify-Write logic relies on a read-only boot status register, causing runtime overrides on other lanes to be silently discarded.
- [Medium] Direct returns and bypassed error labels in `fsl_guts_init` leak I/O memory mappings.
- [Medium] Exported APIs contradict their own zero-based indexing documentation and lack parameter bounds checking.
--

commit 2b919af199a087611275f238825912a50ae47330
Author: Ioana Ciornei <ioana.ciornei@nxp.com>
Date:   Fri Jun 12 21:08:58 2026 +0000

    soc: fsl: guts: implement the RCW override procedure
    
    Add support for the RCW override procedure which enables runtime
    reconfiguration of the protocol running on a SerDes lane. The procedure
    is done through the DCFG DCSR space which now can be defined as the
    second memory region of the guts DT node.

> diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c
> index 9f2aff07a2744..0fa2bd99d3c10 100644
> --- a/drivers/soc/fsl/guts.c
> +++ b/drivers/soc/fsl/guts.c

[ ... ]

> +/* Some fields of the Reset Configuration Word (RCW) can be overridden at
> + * runtime by writing to the RCWCRn registers contained within the DCSR space
> + * of the Device Configuration (DCFG) block. The layout of the RCWCRn registers
> + * is identical with the read-only RCWSRn from the CCSR space.
> + */
> +static void fsl_guts_rmw(int offset, u32 val, u32 mask)
> +{
> +	u32 tmp = fsl_guts_read(&soc.dcfg_ccsr->rcwsr[offset]);
> +
> +	tmp &= ~mask;
> +	tmp |= val;
> +	fsl_guts_write(&soc.dcfg_dcsr->rcwcr[offset], tmp);
> +}

[Severity: High]
Does this read-modify-write logic accidentally discard previous runtime
overrides?

Since rcwsr is a read-only boot status register, it reflects the static boot
state. If multiple lanes share the same register, reading from rcwsr to update
a subsequent lane will overwrite any dynamic configurations previously written
to the write-only rcwcr register. 

Would it be safer to use a software shadow register to track the runtime state
of rcwcr?

[ ... ]

> +/**
> + * fsl_guts_lane_init() - Notify guts module of SerDes lane configuration
> + * @serdes_idx: zero-based SerDes block index
> + * @lane: zero-based lane index within SerDes
> + *
> + * On the LS208xA SoC, the RCW override procedure needs to be aware of all link
> + * modes which are configured on a SerDes block.
> + */
> +void fsl_guts_lane_init(int serdes_idx, int lane, enum lynx_lane_mode lane_mode)
> +{
> +	soc.lane_mode[serdes_idx - 1][lane] = lane_mode;
> +}
> +EXPORT_SYMBOL_NS_GPL(fsl_guts_lane_init, "FSL_GUTS");

[Severity: Medium]
Is the documentation out of sync with the implementation?

The docstring specifies that serdes_idx is a zero-based index, but the code
subtracts 1. If a caller follows the documentation and passes 0, won't this
result in an out-of-bounds array access?

Additionally, is there a risk of out-of-bounds access if the lane parameter
exceeds MAX_NUM_LANES? An unvalidated lane value could also cause undefined
behavior later when expanded by the GENMASK macros in the override lookup
functions.

[ ... ]

> @@ -250,6 +522,8 @@ static int __init fsl_guts_init(void)
>  		of_node_put(np);
>  		return -ENOMEM;
>  	}
> +	/* DCFG_DCSR is optional */
> +	soc.dcfg_dcsr = of_iomap(np, DCFG_DCSR);
>  
>  	soc.little_endian = of_property_read_bool(np, "little-endian");
>  	soc.svr = fsl_guts_read(&soc.dcfg_ccsr->svr);
> @@ -274,7 +548,7 @@ static int __init fsl_guts_init(void)
>  	/* Register soc device */
>  	soc_dev_attr = kzalloc_obj(*soc_dev_attr);
>  	if (!soc_dev_attr)
>  		return -ENOMEM;

[Severity: Medium]
Will this direct return leak the I/O memory mapped earlier?

If kzalloc_obj() fails, the function returns immediately without unmapping
soc.dcfg_ccsr or the newly added soc.dcfg_dcsr.

[ ... ]

> @@ -296,6 +570,8 @@ static int __init fsl_guts_init(void)
>  		goto err;
>  	}

[Severity: Medium]
Does jumping to the err label skip the necessary unmapping?

If soc_device_register() fails, the code uses goto err. Because err is
placed after err_nomem, the iounmap() calls are bypassed, leaving the memory
mappings leaked.

[ ... ]

>  err_nomem:
>  	ret = -ENOMEM;
> -
> +	if (soc.dcfg_dcsr)
> +		iounmap(soc.dcfg_dcsr);
>  	iounmap(soc.dcfg_ccsr);
>  err:
>  	kfree(soc_dev_attr->family);

[Severity: High]
Should the global pointers be set to NULL after unmapping?

If fsl_guts_init() fails, it unmaps soc.dcfg_ccsr and soc.dcfg_dcsr but
leaves the global pointers dangling. Since this is a core_initcall, the boot
process will continue even if initialization fails.

If an exported API like fsl_guts_lane_set_mode() is called later, it checks
the pointer validity:

	if (!soc.dcfg_dcsr) {
		pr_debug("Device tree does not define DCFG_DCSR...");
		return -EINVAL;
	}

Since the pointer isn't NULL, this validation passes, which could lead to a
use-after-free when the driver attempts to write to the unmapped memory or
acquires the uninitialized spinlock.

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