Re: [PATCH resend] ALSA: firewire-speakers: fix locking

Re: [PATCH resend] ALSA: firewire-speakers: fix locking

[Takashi Iwai]

At Sat, 1 Oct 2011 15:57:46 +0200,
Stefan Richter wrote:
> 
> Date: Sat, 27 Aug 2011 16:45:28 +0200
> 
> There is a lock inversion between fwspk->mutex and pcm->open_mutex
> reported by lockdep when fwspk_hw_free is called.
> 
> Fixed by copying the fix from the same former issue in the isight
> sound driver (commit f3f7c1837f6bcae3601fc535b339426868bf1549
> "ALSA: isight: fix locking").
> 
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

We should put cc to stable kernel, right?


thanks,

Takashi

> ---
>  sound/firewire/speakers.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Index: b/sound/firewire/speakers.c
> ===================================================================
> --- a/sound/firewire/speakers.c
> +++ b/sound/firewire/speakers.c
> @@ -772,9 +772,10 @@ static int __devexit fwspk_remove(struct
>  {
>  	struct fwspk *fwspk = dev_get_drvdata(dev);
>  
> -	mutex_lock(&fwspk->mutex);
>  	amdtp_out_stream_pcm_abort(&fwspk->stream);
>  	snd_card_disconnect(fwspk->card);
> +
> +	mutex_lock(&fwspk->mutex);
>  	fwspk_stop_stream(fwspk);
>  	mutex_unlock(&fwspk->mutex);
>  
> @@ -790,8 +791,8 @@ static void fwspk_bus_reset(struct fw_un
>  	fcp_bus_reset(fwspk->unit);
>  
>  	if (cmp_connection_update(&fwspk->connection) < 0) {
> -		mutex_lock(&fwspk->mutex);
>  		amdtp_out_stream_pcm_abort(&fwspk->stream);
> +		mutex_lock(&fwspk->mutex);
>  		fwspk_stop_stream(fwspk);
>  		mutex_unlock(&fwspk->mutex);
>  		return;
> 
> 
> -- 
> Stefan Richter
> -=====-==-== =-=- ----=
> http://arcgraph.de/sr/
> 

Re: [PATCH resend] ALSA: firewire-speakers: fix locking

[Clemens Ladisch]

Stefan Richter wrote:
> On Oct 03 Takashi Iwai wrote:
>> Stefan Richter wrote:
>>> There is a lock inversion between fwspk->mutex and pcm->open_mutex
>>> reported by lockdep when fwspk_hw_free is called.
>>
>> We should put cc to stable kernel, right?
>
> I am not sure.  While I could reproduce the lockdep report reliably at
> device unplugs, I never encountered a deadlock.

The reported scenario requires that the same PCM device is freed twice
at the same time, which is prevented by the file reference counting.

Going through the entire dependency chain would require the device being
unplugged, being independently close()d, and a ioctl being called with
a swapped-out data buffer all happening at the same time.

This doesn't look like an actual problem.


Regards,
Clemens