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@xxxxxxxxx>
> > > > >
> > > > > 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(®ister_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(®ister_mutex);
snd_card_free_when_closed(card);
} else {
+ mutex_unlock(&chip->shutdown_mutex);
mutex_unlock(®ister_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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
