Re: [PATCH v5 09/12] pinctrl: ambarella: add CV75 pin controller

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Race condition between PM suspend/resume callbacks and concurrent pinmux hardware accesses.
--

--- Patch [9]: [PATCH v5 09/12] pinctrl: ambarella: add CV75 pin controller ---
commit d6bbc0d8c89a402f676e8c68b67c6a6866af650a
Author: Long Zhao <[email protected]>

pinctrl: ambarella: add CV75 pin controller
    
Add an Ambarella pinmux-only pinctrl driver with CV75 function/group
tables. GPIO is handled by the PL061 driver.

> diff --git a/drivers/pinctrl/pinctrl-ambarella.c b/drivers/pinctrl/pinctrl-ambarella.c
> new file mode 100644
> index 0000000000000..f9146d365c3c5
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-ambarella.c

[ ... ]

> +static int amb_pinctrl_suspend(struct device *dev)
> +{
> +	struct amb_pinctrl_soc_data *soc = dev_get_drvdata(dev);
> +	u32 bank, dedicated = soc->data->clk_au_dedicated_pin;
> +
> +	for (bank = 0; bank < soc->data->nr_banks; bank++) {

[ ... ]

> +		if (soc->data->have_ds2)
> +			regmap_read(soc->ds_regmap, soc->data->ds2[bank],
> +				    &soc->pm[bank].ds[2]);
> +
> +		amb_iomux_save_bank(soc, bank);

[Severity: High]
Does this read the IOMUX hardware state into PM structures without holding
soc->lock?

If another CPU is modifying the pins concurrently, could this result in a
torn read?

> +	}
> +
> +	if (dedicated >= soc->data->nr_banks * 32)
> +		amb_iomux_save_bank(soc, PINID_TO_BANK(dedicated));
> +
> +	return 0;
> +}
> +
> +static int amb_pinctrl_resume(struct device *dev)
> +{
> +	struct amb_pinctrl_soc_data *soc = dev_get_drvdata(dev);
> +	u32 bank, dedicated = soc->data->clk_au_dedicated_pin;
> +
> +	for (bank = 0; bank < soc->data->nr_banks; bank++)
> +		amb_iomux_restore_bank(soc, bank);
> +
> +	if (dedicated >= soc->data->nr_banks * 32)
> +		amb_iomux_restore_bank(soc, PINID_TO_BANK(dedicated));
> +
> +	/* Ensure restored IOMUX writes land before the commit strobe. */
> +	wmb();
> +	amb_iomux_commit(soc);

[Severity: High]
Does this write directly to shared IOMUX hardware registers without holding
soc->lock?

If a concurrent pinmux or GPIO configuration request executes while the system
is resuming, the pinctrl core will invoke amb_pinmux_set_mux() and acquire
soc->lock.

However, since amb_pinctrl_resume() does not hold the lock here, could it
silently overwrite the concurrent pinmux updates with stale suspended state?

> +
> +	for (bank = 0; bank < soc->data->nr_banks; bank++) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9