ASoC: ti: davinci-mcasp: Protect hw_params callback against race
diff mbox series

Message ID 20190812095304.19030-1-peter.ujfalusi@ti.com
State New
Headers show
Series
  • ASoC: ti: davinci-mcasp: Protect hw_params callback against race
Related show

Commit Message

Peter Ujfalusi Aug. 12, 2019, 9:53 a.m. UTC
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(-)

Comments

Mark Brown Aug. 12, 2019, 11:13 a.m. UTC | #1
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.
Peter Ujfalusi Aug. 12, 2019, 11:46 a.m. UTC | #2
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
Mark Brown Aug. 12, 2019, 11:57 a.m. UTC | #3
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.
Peter Ujfalusi Aug. 12, 2019, 12:56 p.m. UTC | #4
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
Mark Brown Aug. 12, 2019, 1:01 p.m. UTC | #5
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 :/

Patch
diff mbox series

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);