soc: sunxi: sram: order the publication of base against sram_dev
Commit Message
From: Jaidev Shastri <jaidevshastri@vt.edu>
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.
Keep the mapped registers in a local and publish base with
smp_store_release() once sram_dev is set; read it with
smp_load_acquire() in sunxi_sram_claim(). The error value is still
stored to base so that a failed probe keeps reporting it to claimers.
Found with MBCheck, a static herd7-based memory consistency checker.
Signed-off-by: Jaidev Shastri <jaidevshastri@vt.edu>
---
drivers/soc/sunxi/sunxi_sram.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
---
base-commit: 93f51579e7df248780214094418f205253383cc5
change-id: 20260921-mb-sunxi-sram-41c3288a4660
Best regards,
--
Jaidev Shastri <jaidevshastri@vt.edu>
Comments
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);
@@ -266,10 +266,14 @@ int sunxi_sram_claim(struct device *dev)
int err;
int count = 0;
- if (IS_ERR(base))
- return PTR_ERR(base);
+ void __iomem *regs;
- if (!base)
+ /* Pairs with smp_store_release() in sunxi_sram_probe(). */
+ regs = smp_load_acquire(&base);
+ if (IS_ERR(regs))
+ return PTR_ERR(regs);
+
+ if (!regs)
return -EPROBE_DEFER;
if (!dev || !dev->of_node)
@@ -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;
@@ -420,12 +425,21 @@ static int __init sunxi_sram_probe(struct platform_device *pdev)
dev_set_drvdata(dev, (struct sunxi_sramc_variant *)variant);
- base = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(base))
- return PTR_ERR(base);
+ regs = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(regs)) {
+ base = regs;
+ return PTR_ERR(regs);
+ }
+
+ /*
+ * 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.
+ */
+ 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);
if (IS_ERR(regmap))
return PTR_ERR(regmap);