Message-ID: <s5hipc168ud.wl%tiwai@suse.de>
Date:	Wed, 29 Aug 2012 17:01:14 +0200
From:	Takashi Iwai <tiwai@...e.de>
To:	Daniel Mack <zonque@...il.com>
Cc:	Josh Boyer <jwboyer@...hat.com>, Bruno Wolff III <bruno@...ff.to>,
	Jaroslav Kysela <perex@...ex.cz>, alsa-devel@...a-project.org,
	linux-kernel@...r.kernel.org
Subject: Re: Logitech USB headset not working in 3.6-rc3

At Wed, 29 Aug 2012 16:25:46 +0200,
Daniel Mack wrote:
> 
> On 29.08.2012 16:14, Takashi Iwai wrote:
> > At Wed, 29 Aug 2012 15:32:34 +0200,
> > Daniel Mack wrote:
> >>
> >> [1  <text/plain; ISO-8859-1 (7bit)>]
> >> On 29.08.2012 15:29, Takashi Iwai wrote:
> >>> At Wed, 29 Aug 2012 13:26:25 +0200,
> >>> Daniel Mack wrote:
> >>>>
> >>>> [1  <text/plain; ISO-8859-1 (7bit)>]
> >>>> On 25.08.2012 14:17, Josh Boyer wrote:
> >>>>> On Sat, Aug 25, 2012 at 02:13:58PM +0200, Daniel Mack wrote:
> >>>>>> On 25.08.2012 14:07, Bruno Wolff III wrote:
> >>>>>>> On Sat, Aug 25, 2012 at 14:02:51 +0200,
> >>>>>>>    Daniel Mack <zonque@...il.com> wrote:
> >>>>>>>>
> >>>>>>>> Can you revert commit e9ba389c5 ("ALSA: usb-audio: Fix
> >>>>>>>> scheduling-while-atomic bug in PCM capture stream") and see if that
> >>>>>>>
> >>>>>>> I can try that, but it takes a long time to build a new kernel on my 
> >>>>>>> old hardware.
> >>>>>>>
> >>>>>>>> helps? If not, can you summarize again which kernels still work for you
> >>>>>>>> and which don't?
> >>>>>>>
> >>>>>>> The latest kernel that works is 3.6.0-0.rc2.git1.2.fc18. The earliest that 
> >>>>>>> doesn't work is 3.6.0-0.rc2.git2.1.fc18.
> >>>>>>>
> >>>>>>
> >>>>>> The report you sent doesn't look like it could be caused by e9ba389c5.
> >>>>>> It fixes a kernel Ooops. But as it is the only relevant patch in that
> >>>>>> area, it would be interesting if reverting it fixes anything.
> >>>>>
> >>>>> Yep, agreed.  If this revert kernel doesn't work, we're likely down to a
> >>>>> git bisect, Bruno.
> >>>>>
> >>>>>> Btw - thanks a lot for testing -rc kernels, much appreciated!
> >>>>>
> >>>>> Indeed!
> >>>>
> >>>> Could you please try this patch on top of Takashi's? Thanks again!
> >>>>
> >>>>
> >>>> Daniel
> >>>>
> >>>> [2 0001-ALSA-snd-usb-Fix-URB-cancellation-at-stream-start.patch <text/x-patch (7bit)>]
> >>>> >From 6617bb2463ae3fad21390b59cc2a71f96b9e9ca8 Mon Sep 17 00:00:00 2001
> >>>> From: Daniel Mack <zonque@...il.com>
> >>>> Date: Wed, 29 Aug 2012 13:17:05 +0200
> >>>> Subject: [PATCH] ALSA: snd-usb: Fix URB cancellation at stream start
> >>>>
> >>>> Commit e9ba389c5 ("ALSA: usb-audio: Fix scheduling-while-atomic bug in
> >>>> PCM capture stream") fixed a scheduling-while-atomic bug that happened
> >>>> when snd_usb_endpoint_start was called from the trigger callback, which
> >>>> is an atmic context. However, the patch breaks the idea of the endpoints
> >>>> reference counting, which is the reason why the driver has been
> >>>> refactored lately.
> >>>>
> >>>> Revert that commit and let snd_usb_endpoint_start() take care of the URB
> >>>> cancellation again. As this function is called from both atomic and
> >>>> non-atomic context, add a flag to denote whether the function may sleep.
> >>>>
> >>>> Signed-off-by: Daniel Mack <zonque@...il.com>
> >>>> Cc: stable@...nel.org [3.5+]
> >>>> ---
> >>>>  sound/usb/endpoint.c | 10 ++++++++--
> >>>>  sound/usb/endpoint.h |  2 +-
> >>>>  sound/usb/pcm.c      | 13 +++++--------
> >>>>  3 files changed, 14 insertions(+), 11 deletions(-)
> >>>>
> >>>> diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
> >>>> index c411812..678456c 100644
> >>>> --- a/sound/usb/endpoint.c
> >>>> +++ b/sound/usb/endpoint.c
> >>>> @@ -799,7 +799,9 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep,
> >>>>  /**
> >>>>   * snd_usb_endpoint_start: start an snd_usb_endpoint
> >>>>   *
> >>>> - * @ep: the endpoint to start
> >>>> + * @ep:		the endpoint to start
> >>>> + * @can_sleep:	flag indicating whether the operation is executed in
> >>>> + * 		non-atomic context
> >>>>   *
> >>>>   * A call to this function will increment the use count of the endpoint.
> >>>>   * In case it is not already running, the URBs for this endpoint will be
> >>>> @@ -809,7 +811,7 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep,
> >>>>   *
> >>>>   * Returns an error if the URB submission failed, 0 in all other cases.
> >>>>   */
> >>>> -int snd_usb_endpoint_start(struct snd_usb_endpoint *ep)
> >>>> +int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, int can_sleep)
> >>>>  {
> >>>>  	int err;
> >>>>  	unsigned int i;
> >>>> @@ -821,6 +823,10 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep)
> >>>>  	if (++ep->use_count != 1)
> >>>>  		return 0;
> >>>>  
> >>>> +	/* just to be sure */
> >>>> +	deactivate_urbs(ep, 0, can_sleep);
> >>>> +	wait_clear_urbs(ep);
> >>>
> >>> It'd be safer to protect the call of wait_clear_urbs() when
> >>> can_sleep=0.
> >>
> >> Right. New patch attached.
> > 
> > Thanks.  This makes me thinking whether we really need to call
> > deactivate_urbs() at snd_usb_endpoint_start().  deactivate_endpoints()
> > is called already in prepare (at the beginning).  Which possibility is
> > considered?  The comment "just to be sure" implies that my original
> > code before your stream model change was simply optional.  Now I'm not
> > quite sure whether we can drop it or not...
> 
> Yes, we can most probably drop it, but I would clearly do that in
> another patch for 3.6 - I'll prepare one.

Yeah, that's fine for post 3.6.  I just wondered.

> I also found some regressions caused by the recent refactoring, and will
> send out a patch collection, hopefully later today.

OK, thanks.


Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists