Re: [PATCH v2 phy-next 2/8] soc: fsl: guts: add a global structure to hold state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The global `soc.dcfg_ccsr` pointer is not cleared after being unmapped on the error path, resulting in a dangling pointer.
- [Medium] The globally mapped `soc.dcfg_ccsr` is leaked on multiple error paths during initialization.
--

commit 28927a6c50db9adc7ddb8b131d5e08a366298fb3
Author: Ioana Ciornei <ioana.ciornei@nxp.com>

soc: fsl: guts: add a global structure to hold state

Add the fsl_soc_guts structure in order to pass information like base
addresses, endianness etc between the init time and the runtime
operations. There is no point in mapping and unmapping the DCFG CCSR
space every time we need to make a read, just map it once and keep its
reference in this new global structure.

> diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c
> index f87ee47c15035..a0a52a5603a5f 100644
> --- a/drivers/soc/fsl/guts.c
> +++ b/drivers/soc/fsl/guts.c

[ ... ]

> @@ -199,18 +202,17 @@ static int __init fsl_guts_init(void)
>  		return 0;
>  	soc_data = match->data;
>  
> -	regs = of_iomap(np, DCFG_CCSR);
> -	if (!regs) {
> +	soc.dcfg_ccsr = of_iomap(np, DCFG_CCSR);
> +	if (!soc.dcfg_ccsr) {
>  		of_node_put(np);
>  		return -ENOMEM;
>  	}
>  
> -	little_endian = of_property_read_bool(np, "little-endian");
> -	if (little_endian)
> -		svr = ioread32(&regs->svr);
> +	soc.little_endian = of_property_read_bool(np, "little-endian");
> +	if (soc.little_endian)
> +		svr = ioread32(&soc.dcfg_ccsr->svr);
>  	else
> -		svr = ioread32be(&regs->svr);
> -	iounmap(regs);
> +		svr = ioread32be(&soc.dcfg_ccsr->svr);
>  	of_node_put(np);
>  
>  	/* Register soc device */

[Severity: Medium]
Since the iounmap() was removed here to keep soc.dcfg_ccsr mapped, what
happens if the subsequent soc_dev_attr allocation fails?

fsl_guts_init()
	/* Register soc device */
	soc_dev_attr = kzalloc_obj(*soc_dev_attr);
	if (!soc_dev_attr)
		return -ENOMEM;

Does this direct return leak the soc.dcfg_ccsr mapping?

Also, if soc_device_register() fails later in this function, the code does a
goto err:

	soc_dev = soc_device_register(soc_dev_attr);
	if (IS_ERR(soc_dev)) {
		ret = PTR_ERR(soc_dev);
		goto err;
	}

Because this bypasses the err_nomem label, does it also leak the
soc.dcfg_ccsr mapping?

> @@ -263,6 +265,8 @@ static int __init fsl_guts_init(void)
>  
>  err_nomem:
>  	ret = -ENOMEM;
> +
> +	iounmap(soc.dcfg_ccsr);

[Severity: High]
Since soc is a static global structure, does calling iounmap() here leave
soc.dcfg_ccsr with a dangling, non-NULL pointer?

If a client driver later attempts to use the API and checks this pointer
after an initialization failure, could it lead to a kernel oops?

>  err:
>  	kfree(soc_dev_attr->family);
>  	kfree(soc_dev_attr->soc_id);

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