Re: [PATCH]snd-usb-usx2y 0.8.1 against alsa-kernel cvshead: OHCI doesn't crash anymore

[Takashi Iwai <tiwai@suse.de>]

At Mon, 18 Oct 2004 14:06:43 +0200,
Karsten Wiese wrote:
> 
> [1  <text/plain; iso-8859-1 (7bit)>]
> Am Montag 04 Oktober 2004 18:22 schrieb Takashi Iwai:
> > At Sun, 3 Oct 2004 00:38:04 +0200,
> >
> > Karsten Wiese wrote:
> > > > One thing I'm not sure is that the possible race of usX2Y_urbs_start()
> > > > and usX2Y_subs_startup_finish().  Better to use complete handler?
> > >
> > > did I care for any possible racing now?
> >
> > You need to call spin_lock(&usX2Y->prepare_lock) before checking
> > subs->prepared in usX2Y_urbs_start().  In this kind of implementation,
> > all the access to subs->prepared should be protected in the spinlock
> > to avoid races.
> >
> > Also, the spin_lock(&usX2Y->prepare_lock) in usX2Y_urbs_start() may
> > cause deadlock.  Use spin_lock_irq(&usX2y->prepare_lock) instead.
> >
> Hi Takashi
> 
> No spinlock no more. atomic_t instead. Please check.

I'm afraid it's still racy.
In usX2Y_urbs_start(), the complete callback may be called between
usb_submit_urb() and the loop of schedule_timeout().
So, the loop should check at first the condition before sleeping like
below:

	while (atomic_read(&subs->state) != state_PREPARED) {
		timeout = schedule_timeout(timeout);
		if (signal_pending(current))
			...
		if (! timeout)
			...
	}


Regarding the new config stuff:
Let's kill CONFIG_SND_USB_USX2Y_NRPACKS_VARIABLE option.
No one will want to disable such a module option.

Also, please avoid the Kconfig int value as much as possible if you
want to support the driver for 2.2/2.4 kernels, too.  I believe it's
ok to put a hardcoded value 4 like usbaudio.c if you describe the
module option properly in the document.


Takashi


-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl