| 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: <s5hoblt6b0o.wl%tiwai@suse.de>
Date: Wed, 29 Aug 2012 16:14:15 +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 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...
thanks,
Takashi
>
> [2 0001-ALSA-snd-usb-Fix-URB-cancellation-at-stream-start.patch <text/x-patch (7bit)>]
> >From 42ff3e2d34427dc4c6faa09052a2531f2e71d7d6 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 | 11 +++++++++--
> sound/usb/endpoint.h | 2 +-
> sound/usb/pcm.c | 13 +++++--------
> 3 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
> index c411812..b896c55 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,11 @@ 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);
> + if (can_sleep)
> + wait_clear_urbs(ep);
> +
> ep->active_mask = 0;
> ep->unlink_mask = 0;
> ep->phase = 0;
> diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h
> index ee2723f..a8e60c1 100644
> --- a/sound/usb/endpoint.h
> +++ b/sound/usb/endpoint.h
> @@ -13,7 +13,7 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep,
> struct audioformat *fmt,
> struct snd_usb_endpoint *sync_ep);
>
> -int snd_usb_endpoint_start(struct snd_usb_endpoint *ep);
> +int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, int can_sleep);
> void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep,
> int force, int can_sleep, int wait);
> int snd_usb_endpoint_activate(struct snd_usb_endpoint *ep);
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index 62ec808..1546577 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -212,7 +212,7 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface,
> }
> }
>
> -static int start_endpoints(struct snd_usb_substream *subs)
> +static int start_endpoints(struct snd_usb_substream *subs, int can_sleep)
> {
> int err;
>
> @@ -225,7 +225,7 @@ static int start_endpoints(struct snd_usb_substream *subs)
> snd_printdd(KERN_DEBUG "Starting data EP @%p\n", ep);
>
> ep->data_subs = subs;
> - err = snd_usb_endpoint_start(ep);
> + err = snd_usb_endpoint_start(ep, can_sleep);
> if (err < 0) {
> clear_bit(SUBSTREAM_FLAG_DATA_EP_STARTED, &subs->flags);
> return err;
> @@ -239,7 +239,7 @@ static int start_endpoints(struct snd_usb_substream *subs)
> snd_printdd(KERN_DEBUG "Starting sync EP @%p\n", ep);
>
> ep->sync_slave = subs->data_endpoint;
> - err = snd_usb_endpoint_start(ep);
> + err = snd_usb_endpoint_start(ep, can_sleep);
> if (err < 0) {
> clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags);
> return err;
> @@ -544,13 +544,10 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
> subs->last_frame_number = 0;
> runtime->delay = 0;
>
> - /* clear the pending deactivation on the target EPs */
> - deactivate_endpoints(subs);
> -
> /* for playback, submit the URBs now; otherwise, the first hwptr_done
> * updates for all URBs would happen at the same time when starting */
> if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK)
> - return start_endpoints(subs);
> + return start_endpoints(subs, 1);
>
> return 0;
> }
> @@ -1175,7 +1172,7 @@ static int snd_usb_substream_capture_trigger(struct snd_pcm_substream *substream
>
> switch (cmd) {
> case SNDRV_PCM_TRIGGER_START:
> - err = start_endpoints(subs);
> + err = start_endpoints(subs, 0);
> if (err < 0)
> return err;
>
> --
> 1.7.11.4
>
--
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