Re: [PATCH] USB-sound: prevent kernel panic on disconnect

[Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>]

At Wed, 16 Feb 2011 15:54:22 -0800,
Sarah Sharp wrote:
> 
> On Mon, Feb 14, 2011 at 06:39:00PM +0100, Takashi Iwai wrote:
> > At Mon, 14 Feb 2011 09:54:01 +0100,
> > Clemens Ladisch wrote:
> > > 
> > > Alan Stern wrote:
> > > > On Fri, 11 Feb 2011, Sarah Sharp wrote:
> > > > > I suspect it might be the audio driver, as someone on the alsa mailing
> > > > > list suggested the USB sound driver can't handle when one isochronous
> > > > > buffer in an URB has an error, but the URB status is 0.
> > > > 
> > > > I don't think that can happen.  The URB status won't be 0 unless all 
> > > > the individual buffers have 0 status.
> > > 
> > > In any case, the driver would then just copy garbage samples out of the
> > > buffer; this wouldn't affect the driver's data structures.
> > > 
> > > > > From: Pierre-Louis Bossart <bossart.nospam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > > 
> > > > > Note that this is only a work-around, it does not address the
> > > > > root cause of this inconsistency between urbs and PCM states. The
> > > > > dmesg below shows two calls to snd_urb_complete, the substream is
> > > > > NULL and the state is either running or stopped. This doesn't make
> > > > > any sense.
> > > > > ...
> > > > > ALSA urb.c:492: frame 0 active: -84
> > > > > ALSA urb.c:197: cannot submit urb (err = -19)
> > > > > ALSA urb.c:186: NULL substream (subs->running 1) <- How is this possible?
> > > > > ALSA urb.c:186: NULL substream (subs->running 0)
> > > > 
> > > > It's most likely a matter of the device being disconnected but the
> > > > device file still being open.
> > > 
> > > subs->pcm_substream == NULL happens only when the device file _has_
> > > been closed.
> > 
> > More exactly, it's cleared the PCM close callback, so during it's
> > being closed :)
> > Looks like a race as you mentioned below, indeed.
> > 
> > > > Perhaps not everything gets cleaned up the way it should when that
> > > > happens.
> > > 
> > > There seems to be a race between snd_usb_pcm_close() and
> > > snd_usb_stream_disconnect().  I think a mutex taken by both functions
> > > should fix this; and all functions that check 'shutdown' probably need
> > > serializing.
> > 
> > Or, rather make sure that snd_usb_release_substreams() is finished
> > before PCM close; actually it should be called in hw_free callback,
> > but currently it's skipped by the shutdown flag check.  I guess this
> > check isn't right.
> > 
> > The serialization of shutdown check is a good point, but this won't
> > hit actual bugs, I suppose.
> 
> Ok.  I don't have the audio expertise to make a patch; can you or
> Clemens make one for Pierre to try out?

Well, a simple one like the follow is good to check at first.
If this works, it's indeed a race between disconnect callback and PCM
close (hw_free is always called before close).

We can refine it in a better form later once after it's confirmed :)


thanks,

Takashi

---
diff --git a/sound/usb/card.c b/sound/usb/card.c
index 800f7cb..c0f8270 100644
--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -323,6 +323,7 @@ static int snd_usb_audio_create(struct usb_device *dev, int idx,
 		return -ENOMEM;
 	}
 
+	mutex_init(&chip->shutdown_mutex);
 	chip->index = idx;
 	chip->dev = dev;
 	chip->card = card;
@@ -531,6 +532,7 @@ static void snd_usb_audio_disconnect(struct usb_device *dev, void *ptr)
 	chip = ptr;
 	card = chip->card;
 	mutex_lock(&register_mutex);
+	mutex_lock(&chip->shutdown_mutex);
 	chip->shutdown = 1;
 	chip->num_interfaces--;
 	if (chip->num_interfaces <= 0) {
@@ -548,9 +550,11 @@ static void snd_usb_audio_disconnect(struct usb_device *dev, void *ptr)
 			snd_usb_mixer_disconnect(p);
 		}
 		usb_chip[chip->index] = NULL;
+		mutex_unlock(&chip->shutdown_mutex);
 		mutex_unlock(&register_mutex);
 		snd_card_free_when_closed(card);
 	} else {
+		mutex_unlock(&chip->shutdown_mutex);
 		mutex_unlock(&register_mutex);
 	}
 }
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 4132522..ca4de49 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -385,8 +385,10 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream)
 	subs->cur_audiofmt = NULL;
 	subs->cur_rate = 0;
 	subs->period_bytes = 0;
+	mutex_lock(&subs->stream->chip->shutdown_mutex);
 	if (!subs->stream->chip->shutdown)
 		snd_usb_release_substream_urbs(subs, 0);
+	mutex_unlock(&subs->stream->chip->shutdown_mutex);
 	return snd_pcm_lib_free_vmalloc_buffer(substream);
 }
 
diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h
index db3eb21..6e66fff 100644
--- a/sound/usb/usbaudio.h
+++ b/sound/usb/usbaudio.h
@@ -36,6 +36,7 @@ struct snd_usb_audio {
 	struct snd_card *card;
 	u32 usb_id;
 	int shutdown;
+	struct mutex shutdown_mutex;
 	unsigned int txfr_quirk:1; /* Subframe boundaries on transfers */
 	int num_interfaces;
 	int num_suspended_intf;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html