Re: [PATCH v4 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iio: adc: ma35d1: unclocked MMIO access when disabling buffer or suspending
--

commit 42a7961767520f06d7be9ff697854a2064ddd67f
Author: Chi-Wen Weng <cwweng@nuvoton.com>

iio: adc: Add Nuvoton MA35D1 EADC driver

This commit adds an IIO ADC driver for the Enhanced Analog-to-Digital
Converter on Nuvoton MA35D1 SoCs, supporting single-ended and differential
inputs, direct conversions, and triggered buffered capture using sample
module 0.

> diff --git a/drivers/iio/adc/ma35d1_eadc.c b/drivers/iio/adc/ma35d1_eadc.c
> new file mode 100644
> index 0000000000000..b45ad3ef52645
> --- /dev/null
> +++ b/drivers/iio/adc/ma35d1_eadc.c
> @@ -0,0 +1,912 @@

[ ... ]

> +static void ma35d1_adc_hw_disable_action(void *data)
> +{
> +	struct ma35d1_adc *adc = data;
> +
> +	if (adc->clk_enabled)
> +		ma35d1_adc_hw_disable(adc);
> +}

[ ... ]

> +static int ma35d1_adc_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	guard(mutex)(&adc->lock);
> +
> +	ret = ma35d1_adc_disable_irq(adc);
> +	if (ret)
> +		return ret;
> +
> +	synchronize_irq(adc->irq);
> +
> +	return regmap_write(adc->regmap, MA35D1_EADC_STATUS2,
> +			    MA35D1_EADC_STATUS2_ADIF0);
> +}

[ ... ]

> +static int ma35d1_adc_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	struct regmap *map = adc->regmap;
> +	int ret;
> +
> +	ret = iio_device_suspend_triggering(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&adc->lock);
> +
> +	ret = ma35d1_adc_disable_irq(adc);
> +	if (ret)
> +		goto resume_triggering;
> +
> +	synchronize_irq(adc->irq);
> +
> +	ret = regmap_clear_bits(map, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCEN);
> +	if (ret) {
> +		/*
> +		 * ADCIEN0 is enabled only while a conversion is active. Holding
> +		 * adc->lock here guarantees that no conversion is in progress, so
> +		 * leave it disabled. The next conversion enables it again.
> +		 */
> +		goto resume_triggering;
> +	}
> +
> +	adc->suspended = true;
> +	clk_disable_unprepare(adc->clk);
> +	adc->clk_enabled = false;
> +	mutex_unlock(&adc->lock);
> +
> +	return 0;
> +
> +resume_triggering:
> +	mutex_unlock(&adc->lock);
> +	iio_device_resume_triggering(indio_dev);
> +
> +	return ret;
> +}
> +
> +static int ma35d1_adc_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ma35d1_adc *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&adc->lock);
> +
> +	ret = clk_prepare_enable(adc->clk);
> +	if (ret)
> +		goto unlock;
> +
> +	adc->clk_enabled = true;
> +	ret = ma35d1_adc_hw_init(adc);
> +	if (ret) {
> +		clk_disable_unprepare(adc->clk);
> +		adc->clk_enabled = false;
> +		goto unlock;
> +	}
> +
> +	adc->suspended = false;
> +	mutex_unlock(&adc->lock);
> +
> +	return iio_device_resume_triggering(indio_dev);
> +
> +unlock:
> +	mutex_unlock(&adc->lock);
> +
> +	return ret;
> +}

[Severity: High]
Can ma35d1_adc_buffer_predisable() and ma35d1_adc_suspend() access EADC MMIO
registers while adc->clk is disabled?

During system suspend, ma35d1_adc_suspend() sets adc->suspended = true, calls
clk_disable_unprepare(adc->clk), and sets adc->clk_enabled = false. If a
subsequent ma35d1_adc_resume() fails in clk_prepare_enable() or
ma35d1_adc_hw_init(), its error path leaves adc->clk_enabled = false and
adc->suspended = true while any previously enabled triggered buffer remains
active.

When userspace later disables the buffer or unbinds the driver (or if the
driver is unbound while suspended, since devm_iio_device_register() teardown
runs iio_device_unregister() -> iio_disable_all_buffers() ->
ma35d1_adc_buffer_predisable() before ma35d1_adc_hw_disable_action()),
ma35d1_adc_buffer_predisable() unconditionally calls ma35d1_adc_disable_irq()
and regmap_write(adc->regmap, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0)
without checking adc->suspended or adc->clk_enabled, accessing MMIO registers
with the peripheral clock gated.

Similarly, if another suspend cycle runs after a failed resume,
ma35d1_adc_suspend() does not check whether the device is already suspended
before accessing registers, calling clk_disable_unprepare(adc->clk) a second
time, and calling iio_device_suspend_triggering(indio_dev) again.

Should ma35d1_adc_buffer_predisable() and ma35d1_adc_suspend() check
adc->suspended (or adc->clk_enabled, as ma35d1_adc_hw_disable_action() does)
before touching the hardware registers and disabling the clock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104091136.151269-1-cwweng.linux@gmail.com?part=2