Re: [PATCH v1 1/2] ALSA: hda: cs35l41: Add common wait_for_boot api and reset_and_boot api

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ASoC: cs35l41: false timeout in cs35l41_wait_boot_done() polling loop
--

commit 1eb324e95aeb4455ca413ace17b7aba52e402175
Author: Stefan Binding <sbinding@opensource.cirrus.com>

ALSA: hda: cs35l41: Add common wait_for_boot api and reset_and_boot api

This commit introduces common cs35l41_wait_boot_done() and
cs35l41_reset_and_boot() helpers in cs35l41-lib.c and updates the HDA
CS35L41 driver to use them during probe and system resume. The new helpers
ignore transient regmap_read() failures while the chip is booting and retry
the software reset write if it initially fails.

> diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
> index b0fa80705be7d..973713626ebd4 100644
> --- a/sound/soc/codecs/cs35l41-lib.c
> +++ b/sound/soc/codecs/cs35l41-lib.c
[ ... ]
> @@ -1586,6 +1588,71 @@ int cs35l41_exit_hibernate(struct device *dev, struct regmap *regmap)
>  }
>  EXPORT_SYMBOL_GPL(cs35l41_exit_hibernate);
>  
> +int cs35l41_wait_boot_done(struct device *dev, struct regmap *regmap)
> +{
> +	ktime_t timeout;
> +	unsigned int val = 0;
> +	int ret = 0;
> +
> +	/*
> +	 * The chip could NAK a read request while it is booting so ignore
> +	 * errors from regmap_read.
> +	 */
> +	timeout = ktime_add_us(ktime_get(), CS35L41_GLOBAL_BOOT_DONE_TIMEOUT_US);
> +	regmap_read(regmap, CS35L41_IRQ1_STATUS4, &val);
> +	while (!(val & CS35L41_OTP_BOOT_DONE) && !ktime_after(ktime_get(), timeout)) {
> +		usleep_range(CS35L41_GLOBAL_BOOT_DONE_SLEEP_US / 2,
> +			     CS35L41_GLOBAL_BOOT_DONE_SLEEP_US);
> +		regmap_read(regmap, CS35L41_IRQ1_STATUS4, &val);
> +	}
> +
> +	if (!(val & CS35L41_OTP_BOOT_DONE)) {
> +		dev_err(dev, "Timeout waiting for OTP_BOOT_DONE\n");
> +		return -ETIMEDOUT;
> +	}

[Severity: Medium]
Could this polling loop in cs35l41_wait_boot_done() return a false
-ETIMEDOUT if the task is preempted or delayed across the timeout deadline?

When cs35l41_hda_probe() or cs35l41_system_resume() calls
cs35l41_reset_and_boot() -> cs35l41_wait_boot_done(), if a regmap_read() of
CS35L41_IRQ1_STATUS4 occurs while the chip is still booting (either
returning a value without CS35L41_OTP_BOOT_DONE set or failing with a bus
NAK and leaving val at 0), and the thread is then preempted during
usleep_range() or delayed on the I2C/SPI controller lock until after
timeout expires, the loop condition !ktime_after(ktime_get(), timeout)
evaluates to false before another register read is performed. Even if the
hardware finished booting well before the deadline, val still holds the
stale pre-preemption value and cs35l41_wait_boot_done() fails with
-ETIMEDOUT.

Would it be better to use read_poll_timeout() from include/linux/iopoll.h
(similar to cs35l56_wait_for_firmware_boot() in
sound/soc/codecs/cs35l56-shared.c), which samples the timeout before
performing the read and guarantees one final read after the deadline expires
while still ignoring regmap_read() errors?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030171045.469944-1-sbinding@opensource.cirrus.com?part=1