Re: [PATCH v4 4/7] ASoC: meson: add AUDIN driver

[Valerio Setti <vsetti@baylibre.com>]

>> +static int audin_fifo_dai_trigger(struct snd_pcm_substream *substream, int cmd,
>> +				  struct snd_soc_dai *dai)
>> +{
>> +	struct audin_fifo_dai_data *data = snd_soc_dai_dma_data_get_capture(dai);
>> +	struct snd_soc_component *component = dai->component;
>> +
>> +	switch (cmd) {
>> +	case SNDRV_PCM_TRIGGER_START:
>> +	case SNDRV_PCM_TRIGGER_RESUME:
>> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> +		snd_soc_component_update_bits(component, dai->driver->base + AUDIN_FIFO_CTRL,
>> +					      AUDIN_FIFO_CTRL_EN,
>> +					      AUDIN_FIFO_CTRL_EN);
>> +		WRITE_ONCE(data->running, true);
>> +		hrtimer_start(&data->polling_timer,
>> +			      audin_fifo_bytes_to_ns(data, data->period_bytes),
>> +			      HRTIMER_MODE_REL_SOFT);
> 
> [Severity: High]
> Does this blindly re-enqueue the timer without synchronizing with a potentially
> running callback?
> 
> If a user rapidly toggles SNDRV_PCM_IOCTL_PAUSE, PAUSE_PUSH uses a
> non-blocking hrtimer_try_to_cancel(). If the timer is running and skips
> snd_pcm_period_elapsed() (e.g. because delta < period_bytes), it won't take
> the PCM stream lock and will execute hrtimer_forward_now() concurrently with
> PAUSE_RELEASE.
> 
> PAUSE_RELEASE then blindly calls hrtimer_start(), which enqueues the timer.
> When the running callback in audin_fifo_timer_cb() reads running=true, it
> returns HRTIMER_RESTART, resulting in a double enqueue of the same timer
> which can corrupt the hrtimer rbtree.

This looks correct. I will send an update for this.

>> +static int audin_fifo_dai_remove(struct snd_soc_dai *dai)
>> +{
>> +	kfree(snd_soc_dai_dma_data_get_capture(dai));
>> +
>> +	return 0;
>> +}
> 
> [Severity: High]
> Does this free the DAI data without waiting for the polling timer to finish?
> 
> If the driver is unbound while an audio stream is open, the ALSA core triggers
> a stream stop which uses a non-blocking hrtimer_try_to_cancel(). If the timer
> callback is running concurrently, it will write to this freed memory.
> 

This and the following issue look a bit of corner case to me (i.e. 
unbouding a driver while record is ongoing), but I agree that's a weak 
point so I will fix it.

>> +int audin_fifo_sync_stop(struct snd_soc_component *component, struct snd_pcm_substream *substream)
>> +{
>> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
>> +	struct snd_soc_dai *dai = snd_soc_rtd_to_cpu(rtd, 0);
>> +	struct audin_fifo_dai_data *data = snd_soc_dai_dma_data_get_capture(dai);
>> +
>> +	hrtimer_cancel(&data->polling_timer);
> 
> [Severity: High]
> Could this deferred callback access the freed data pointer?
> 
> If the driver was unbound and audin_fifo_dai_remove() freed the DAI data,
> when the file descriptor is eventually closed this deferred sync_stop callback
> will access the freed data pointer to cancel the timer.
> 
>> +	WRITE_ONCE(data->running, false);
>> +
>> +	return 0;
>> +}
>> diff --git a/sound/soc/meson/audin.c b/sound/soc/meson/audin.c
>> new file mode 100644
>> index 0000000000000..808e7d4fd31a2
>> --- /dev/null
>> +++ b/sound/soc/meson/audin.c
> [ ... ]
>> +static const struct regmap_config audin_regmap_cfg = {
>> +	.reg_bits	= 32,
>> +	.val_bits	= 32,
>> +	.reg_stride	= 4,
>> +	.max_register	= 0x148,
>> +};
> 
> [Severity: Medium]
> Will hardware state like the channel mask and endianness be lost across
> system suspend and resume cycles?
> 
> ALSA calls the driver's prepare() callback to recover a suspended stream,
> but not hw_params(). Because this regmap lacks a cache configuration, it
> cannot automatically restore the register state configured during hw_params()
> when the system resumes.

This was already answered by Jerome [1] (thanks!) so that's out of scope 
for this series.

[1]: 
https://lore.kernel.org/all/1jbj95zz8o.fsf@starbuckisacylon.baylibre.com/

-- 
Valerio


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic