| Message ID | 20190812095304.19030-1-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: ti: davinci-mcasp: Protect hw_params callback against race | expand |
On Mon, Aug 12, 2019 at 12:53:04PM +0300, Peter Ujfalusi wrote: > If the playback and capture of the same McASP is connected to different > dai link (non duplex PCM links, like with pcm3168a codec) then there is > a high probability of race between the two direction leaving McASP in a > confused state. > > Protect the hw_params() with a mutex to make sure that this is not > happening. This feels like something we might want to consider moving up to the core, though not every device is going to need it I guess it should be safe to do there.
On 12/08/2019 14.13, Mark Brown wrote: > On Mon, Aug 12, 2019 at 12:53:04PM +0300, Peter Ujfalusi wrote: >> If the playback and capture of the same McASP is connected to different >> dai link (non duplex PCM links, like with pcm3168a codec) then there is >> a high probability of race between the two direction leaving McASP in a >> confused state. >> >> Protect the hw_params() with a mutex to make sure that this is not >> happening. > > This feels like something we might want to consider moving up to the > core, though not every device is going to need it I guess it should be > safe to do there. soc_pcm_hw_params() is already protected by mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass); which (I think) gives protection for dai_links when they support both playback and capture. I faced with the concurrency when interfacing with pcm3168a codec, which require two dai_links. One for playback and one for capture so they don't share the same snd_soc_pcm_runtime. I believe moving the mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass); to card level could substitute (new mutex on the card for pcm operations probably) the need to handle this in drivers. We would need to change soc-core/pcm/compress for this. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Mon, Aug 12, 2019 at 02:46:33PM +0300, Peter Ujfalusi wrote: > to card level could substitute (new mutex on the card for pcm operations > probably) the need to handle this in drivers. > We would need to change soc-core/pcm/compress for this. Yeah, it'd be a reasonably substantial change.
On 12/08/2019 14.57, Mark Brown wrote: > On Mon, Aug 12, 2019 at 02:46:33PM +0300, Peter Ujfalusi wrote: > >> to card level could substitute (new mutex on the card for pcm operations >> probably) the need to handle this in drivers. > >> We would need to change soc-core/pcm/compress for this. > > Yeah, it'd be a reasonably substantial change. OK, works fine on several boards, I'll send a patch tomorrow after a bit more testing. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Mon, Aug 12, 2019 at 03:56:11PM +0300, Peter Ujfalusi wrote: > On 12/08/2019 14.57, Mark Brown wrote: > > On Mon, Aug 12, 2019 at 02:46:33PM +0300, Peter Ujfalusi wrote: > >> to card level could substitute (new mutex on the card for pcm operations > >> probably) the need to handle this in drivers. > >> We would need to change soc-core/pcm/compress for this. > > Yeah, it'd be a reasonably substantial change. > OK, works fine on several boards, I'll send a patch tomorrow after a bit > more testing. Ah, excellent, thanks for that! That was more of a "we should do this" thing than a "do this instead" thing but obviously fixing more systems is even better so please don't let me stop you. Only sending applied mails when I push things out solves one set of problems but does make for other races :/
diff --git a/sound/soc/ti/davinci-mcasp.c b/sound/soc/ti/davinci-mcasp.c index 7aa3c32e4a49..fe7a0b3572e2 100644 --- a/sound/soc/ti/davinci-mcasp.c +++ b/sound/soc/ti/davinci-mcasp.c @@ -111,6 +111,7 @@ struct davinci_mcasp { u32 channels; int max_format_width; u8 active_serializers[2]; + struct mutex mutex; #ifdef CONFIG_GPIOLIB struct gpio_chip gpio_chip; @@ -1169,6 +1170,8 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream, int period_size = params_period_size(params); int ret; + mutex_lock(&mcasp->mutex); + switch (params_format(params)) { case SNDRV_PCM_FORMAT_U8: case SNDRV_PCM_FORMAT_S8: @@ -1197,12 +1200,13 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream, default: printk(KERN_WARNING "davinci-mcasp: unsupported PCM format"); - return -EINVAL; + ret = -EINVAL; + goto out; } ret = davinci_mcasp_set_dai_fmt(cpu_dai, mcasp->dai_fmt); if (ret) - return ret; + goto out; /* * If mcasp is BCLK master, and a BCLK divider was not provided by @@ -1223,7 +1227,7 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream, ret = mcasp_common_hw_param(mcasp, substream->stream, period_size * channels, channels); if (ret) - return ret; + goto out; if (mcasp->op_mode == DAVINCI_MCASP_DIT_MODE) ret = mcasp_dit_hw_param(mcasp, params_rate(params)); @@ -1232,7 +1236,7 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream, channels); if (ret) - return ret; + goto out; davinci_config_channel_size(mcasp, word_length); @@ -1242,7 +1246,10 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream, mcasp->max_format_width = word_length; } - return 0; +out: + mutex_unlock(&mcasp->mutex); + + return ret; } static int davinci_mcasp_trigger(struct snd_pcm_substream *substream, @@ -2335,6 +2342,8 @@ static int davinci_mcasp_probe(struct platform_device *pdev) if (ret) return -EINVAL; + mutex_init(&mcasp->mutex); + ret = devm_snd_soc_register_component(&pdev->dev, &davinci_mcasp_component, &davinci_mcasp_dai[pdata->op_mode], 1);
If the playback and capture of the same McASP is connected to different dai link (non duplex PCM links, like with pcm3168a codec) then there is a high probability of race between the two direction leaving McASP in a confused state. Protect the hw_params() with a mutex to make sure that this is not happening. The concurrent execution of hw_params for capture and playback can be easily triggered with custom .asoundrc file (for pcm3168a codec): pcm.dmixed8 { type dmix ipc_key 2048 ipc_perm 0666 slave { pcm "hw:0,0" format S24_LE channels 8 rate 96000 } bindings { 0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7 } } pcm.cpb-headset-1 { type plug slave.pcm dmixed8 hint { show on description "Headset 1 jack" } ttable.0.0 1 ttable.1.4 1 } pcm.dsnooped6 { type dsnoop ipc_key 2049 ipc_perm 0666 slave { pcm "hw:0,1" format S24_LE channels 6 rate 96000 } bindings { 0 0 1 1 2 2 3 3 4 4 5 5 } } pcm.cpb-mic-1 { type plug slave.pcm "dsnooped6" hint { show on description "Microphone 1 jack" } ttable.0.0 1 ttable.1.3 1 } Then running: arecord -D cpb-mic-1 -f S24_LE -c2 -r48000 | aplay -D cpb-headset-1 -f S24_LE -c2 -r48000 Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- sound/soc/ti/davinci-mcasp.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)