Re: [PATCH]usx2y: prevent oops & dead keyboard on hot usb unplugging

[Takashi Iwai <tiwai@suse.de>]

At Fri, 15 Apr 2005 17:45:44 +0200 (CEST),
karsten wiese wrote:
> 
>  --- Takashi Iwai <tiwai@suse.de> schrieb: 
> > At Fri, 15 Apr 2005 17:14:40 +0200 (CEST),
> > karsten wiese wrote:
> > > 
> > > 
> > > --- Takashi Iwai <tiwai@suse.de> wrote:
> > > > At Fri, 15 Apr 2005 16:28:54 +0200 (CEST),
> > > > karsten wiese wrote:
> > > > > 
> > > > > 
> > > > > --- Takashi Iwai <tiwai@suse.de> wrote:
> > > > > > At Fri, 15 Apr 2005 15:59:25 +0200 (CEST),
> > > > > > karsten wiese wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > --- Lee Revell <rlrevell@joe-job.com> wrote:
> > > > > > > > On Thu, 2005-04-14 at 20:59 +0200, karsten
> > wiese
> > > > > > wrote:
> > > > > > > > > Hi Clemens & Takashi
> > > > > > > > > 
> > > > > > > > > Please commit the patch.
> > > > > > > > 
> > > > > > > > Is the ALSA in kernel 2.6.11 affected?  If
> > so,
> > > > this
> > > > > > > 
> > > > > > > Think so, yes. The keyboard dead effect +
> > described
> > > > > > oops
> > > > > > > has been around for quiet a long time.
> > > > > > > (There is a lkml mail from yesterday about an
> > > > > > usb-cellphone
> > > > > > > setup,
> > > > > > > where the same oops + dead keyboard happens
> > also:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> http://marc.theaimsgroup.com/?i=<20050414230621.49663f75.vsu%20()%20altlinux%20!%20ru>
> > > > > > > )
> > > > > > > There is also an alsa-bug with snd-usb-audio in
> > the
> > > > > > > bugbase.
> > > > > > > Clemens seams not to be convinced about this
> > > > aproach
> > > > > > yet.
> > > > > > > maybe 'cause the comment of
> > snd_card_free_in_thread
> > > > > > > explicitly states that its the one to be called
> > for
> > > > > > > hotplug:
> > > > > > > /**
> > > > > > >  *  snd_card_free_in_thread - call
> > snd_card_free()
> > > > in
> > > > > > > thread
> > > > > > >  *  @card: soundcard structure
> > > > > > >  *
> > > > > > >  *  This function schedules the call of
> > > > snd_card_free()
> > > > > > > function in a
> > > > > > >  *  work queue.  When all devices are released
> > > > > > (non-busy),
> > > > > > > the work
> > > > > > >  *  is woken up and calls snd_card_free().
> > > > > > >  *
> > > > > > >  *  When a card can be disconnected at any time
> > by
> > > > > > hotplug
> > > > > > > service,
> > > > > > >  *  this function should be used in disconnect
> > (or
> > > > > > detach)
> > > > > > > callback
> > > > > > >  *  instead of calling snd_card_free()
> > directly.
> > > > > > >  *  
> > > > > > >  *  Returns - zero otherwise a negative error
> > code
> > > > if
> > > > > > the
> > > > > > > start of thread failed.
> > > > > > >  */
> > > > > > > 
> > > > > > > Also I remember , that snd_card_free didn't
> > work ok
> > > > > > when i
> > > > > > > used it in the first implementations of
> > > > usx2y-module
> > > > > > some 2
> > > > > > > years ago.
> > > > > > > but that was on kernel 2.4.something.......
> > > > > > > maybe above comment is from kernel 2.4 also and
> > the
> > > > > > hotplug
> > > > > > > system has changed ? anybody?
> > > > > > 
> > > > > > Might be.  To be sure, check whether the release
> > fops
> > > > is
> > > > > > called
> > > > > > properly before you calling snd_card_free().
> > > > > 
> > > > > You mean snd_card_disconnect()?
> > > > > That's called with the same argument and before
> > > > > snd_card_free().
> > > > 
> > > > No, I mean snd_card_free(), which you changed from
> > > > snd_card_free_in_thread().  Someone has to release
> > the
> > > > files
> > > > (i.e. call release fops eventually) before clean up
> > > > everything via
> > > > snd_card_free().
> > > > 
> > > Erm, you mean the close (for
> > > snd_pcm_ops_t,snd_rawmidi_ops_t) and release (for
> > > snd_hwdep_ops_t) callbacks then?
> > 
> > I meant the "file operations".  Anyway, the callback of
> > PCM or rawmidi
> > will be called from the release callback of fops,
> > though...
> > 
> > >  They should all be called
> > > before snd_card_free()?
> > 
> > Yes.
> > 
> > > in other words:
> > > <snip>
> > > int snd_card_free_in_thread(snd_card_t * card)
> > > {
> > > 	if (card->files == NULL) {
> > > 		snd_card_free(card);
> > > 		return 0;
> > > 	}
> > > </snip>
> > > the "(card->files == NULL)" MUST be true before
> > > snd_card_free() can be called safely?
> > 
> > Yes, exactly.
> > Otherwise, snd_card_free() removes the device entry, and
> > the remainig
> > files will have no way to release themselves after that.
> 
> Just found that snd_card_free() also waits for all files
> being released:
> <snip>
> int snd_card_free(snd_card_t * card)
> {
> 	struct snd_shutdown_f_ops *s_f_ops;
> 
> 	if (card == NULL)
> 		return -EINVAL;
> 	write_lock(&snd_card_rwlock);
> 	snd_cards[card->number] = NULL;
> 	write_unlock(&snd_card_rwlock);
> 
> #ifdef CONFIG_PM
> 	wake_up(&card->power_sleep);
> #ifdef CONFIG_SND_GENERIC_PM
> 	if (card->pm_dev) {
> 		snd_generic_device_unregister(card->pm_dev);
> 		card->pm_dev = NULL;
> 	}
> #endif
> #endif
> 
> 	/* wait, until all devices are ready for the free
> operation */
> 	wait_event(card->shutdown_sleep, card->files == NULL);
> </snip>
> 
> (-: You apply my patch now?

No, not enough :)  This is exactly why snd_card_free_in_thread() was
introduced.

As I asked, please make sure that the files are all released before
snd_card_free().  Otherwise the disconnect callback hangs at this
point because of this check.

(IOW, the fact that calling snd_card_free doesn't hang implies that
 the files are released.  But it's better to check this explicitly.)


Takashi


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click