Re: [PATCH] soc: sunxi: sram: order the publication of base against sram_dev

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] soc: sunxi: code and commit message mismatch regarding sram_dev dependency
--

commit 02f3c870b0e732bf484750fa2453c39fde232ed6
Author: Jaidev Shastri <jaidevshastri@vt.edu>

soc: sunxi: sram: order the publication of base against sram_dev

This commit modifies sunxi_sram_probe() to use smp_store_release() when
publishing the base pointer, and sunxi_sram_claim() to use smp_load_acquire().
The stated goal is to prevent sunxi_sram_claim() from dereferencing a null
sram_dev pointer.

>     sunxi_sram_claim() is called from the probe functions of the EMAC, the
>     video engine and other consumers. It gates on the file-scope base
>     pointer:
>     
>             if (!base)
>                     return -EPROBE_DEFER;
>     
>     and then walks sram_dev->of_node in sunxi_sram_of_parse().
>     sunxi_sram_probe() writes sram_dev and then base, both with plain
>     stores, and the claimer reads both with plain loads. Neither the two
>     stores nor the two loads are ordered, so a claimer on another CPU can
>     pass the gate and dereference sram_dev == NULL.

[Severity: Medium]
Is this description accurate?

Looking at sunxi_sram_claim() in drivers/soc/sunxi/sunxi_sram.c, it checks
the base pointer but then walks the consumer's dev->of_node rather than
sram_dev->of_node:

int sunxi_sram_claim(struct device *dev)
{
        ...
        if (!dev || !dev->of_node)
                return -EINVAL;

        of_for_each_phandle(&it, err, dev->of_node, SUNXI_SRAM_PROP,
                            NULL, SUNXI_SRAM_CELLS) {
        ...
}

It doesn't appear that sunxi_sram_claim() accesses the sram_dev pointer at all,
so the data dependency described in the commit message doesn't seem to exist.

> diff --git a/drivers/soc/sunxi/sunxi_sram.c b/drivers/soc/sunxi/sunxi_sram.c
> index 2ccaeacf8c706..0e0b86ad3fd37 100644
> --- a/drivers/soc/sunxi/sunxi_sram.c
> +++ b/drivers/soc/sunxi/sunxi_sram.c
[ ... ]
> @@ -410,6 +414,7 @@ static int __init sunxi_sram_probe(struct platform_device *pdev)
>  	const struct sunxi_sramc_variant *variant;
>  	struct device *dev = &pdev->dev;
>  	struct regmap *regmap;
> +	void __iomem *regs;
>  	int ret;
>  
>  	sram_dev = &pdev->dev;
[ ... ]
> +	/*
> +	 * base is the gate that sunxi_sram_claim() tests before it walks
> +	 * sram_dev->of_node. Publish it after sram_dev so that a claimer on
> +	 * another CPU that passes the gate also sees sram_dev.
> +	 */

[Severity: Medium]
Does this comment need to be updated?

Since sunxi_sram_claim() iterates over dev->of_node rather than
sram_dev->of_node, the rationale for this memory barrier seems incorrect.

> +	smp_store_release(&base, regs);
>  
>  	if (variant->num_emac_clocks || variant->has_ldo_ctrl) {
> -		regmap = devm_regmap_init_mmio(dev, base, &sunxi_sram_regmap_config);
> +		regmap = devm_regmap_init_mmio(dev, regs, &sunxi_sram_regmap_config);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-mb-sunxi-sram-v1-1-ad7817f0ee7d@vt.edu?part=1