Re: [PATCH 2/2] ASoC: Add LPASS VA CSR heartbeat pulse clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Uninitialized stack variable `val` is evaluated if `regmap_read()` fails in `heartbeat_pulse_is_enabled()`.
- [High] The clock enable/disable callbacks use a standard regmap and Maple Tree cache, leading to lock nesting violations and 'sleeping in atomic context' panics on PREEMPT_RT.
- [Medium] The return values of `regmap_write` and `regmap_update_bits` are silently ignored in `heartbeat_pulse_enable`.
--

--- Patch [2]: [PATCH 2/2] ASoC: Add LPASS VA CSR heartbeat pulse clock ---
commit 7fe84986850c91283daded46e6a43a50beacfb9f
Author: Sarath Ganapathiraju <sarath.ganapathiraju@oss.qualcomm.com>

ASoC: Add LPASS VA CSR heartbeat pulse clock

This commit adds the LPASS VA CSR driver to model the rate generator
as a clock provider. This enables automatic enable and disable via
the clock framework during runtime PM resume and suspend.

> diff --git a/sound/soc/codecs/lpass-va-csr.c b/sound/soc/codecs/lpass-va-csr.c
> new file mode 100644
> index 0000000000000..14aec8f941008
> --- /dev/null
> +++ b/sound/soc/codecs/lpass-va-csr.c
[ ... ]
> +static int heartbeat_pulse_enable(struct clk_hw *hw)
> +{
> +	struct lpass_va_csr *csr = to_lpass_va_csr(hw);
> +
> +	regmap_write(csr->regmap, LPASS_RATE_GEN_COUNTER_0, csr->data->counter_0);
> +	regmap_write(csr->regmap, LPASS_RATE_GEN_DELAY, csr->data->delay);
> +	regmap_update_bits(csr->regmap, LPASS_RATE_GEN_CTRL,
> +			   LPASS_RG_CTRL_EN, LPASS_RG_CTRL_EN);
> +
> +	return 0;

[Severity: Medium]
Does this function unintentionally mask hardware or cache errors? By ignoring
the return values from regmap_write() and regmap_update_bits() and
unconditionally returning 0, the clock framework might assume the clock is
enabled even if the hardware write or cache allocation failed.

> +}
[ ... ]
> +static int heartbeat_pulse_is_enabled(struct clk_hw *hw)
> +{
> +	struct lpass_va_csr *csr = to_lpass_va_csr(hw);
> +	unsigned int val;
> +
> +	regmap_read(csr->regmap, LPASS_RATE_GEN_CTRL, &val);
> +
> +	return !!(val & LPASS_RG_CTRL_EN);

[Severity: High]
Could this evaluate uninitialized memory if the regmap read fails? If
regmap_read() returns an error, the stack variable val is left untouched,
which might result in the clock framework registering an incorrect initial
hardware state.

> +}
> +
> +static const struct clk_ops heartbeat_pulse_ops = {
> +	.enable = heartbeat_pulse_enable,
> +	.disable = heartbeat_pulse_disable,
> +	.is_enabled = heartbeat_pulse_is_enabled,
> +};

[Severity: High]
Is it safe to use a standard regmap and maple tree cache in atomic clock
callbacks? The common clock framework invokes .enable, .disable, and
.is_enabled callbacks under the enable_lock raw spinlock with interrupts
disabled.

However, the regmap configuration omits use_raw_spinlock = true:

static const struct regmap_config lpass_rate_gen_regmap_config = {
	...
	.cache_type = REGCACHE_MAPLE,
};

This means the regmap operations rely on a standard spinlock. On preempt-rt
kernels, a standard spinlock is a sleepable rt_mutex, which could cause lock
nesting violations and kernel panics when acquired inside an atomic context.
Additionally, the REGCACHE_MAPLE cache type allocates memory and uses its own
standard spinlock.

Should these callbacks be moved to .prepare and .unprepare instead, which run
under a sleepable mutex context?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-master-v1-0-1ebe5993225e@oss.qualcomm.com?part=2