| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <503E266A.1070400@gmail.com>
Date: Wed, 29 Aug 2012 16:25:46 +0200
From: Daniel Mack <zonque@...il.com>
To: Takashi Iwai <tiwai@...e.de>
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
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.
I also found some regressions caused by the recent refactoring, and will
send out a patch collection, hopefully later today.
Daniel
--
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