| Message ID | 1373533573-12272-46-git-send-email-ming.lei@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11-07-2013 13:06, Ming Lei wrote: > Complete() will be run with interrupt enabled, so change to > spin_lock_irqsave(). Changelog doesn't match the patch. > Cc: Jaroslav Kysela <perex@perex.cz> > Cc: Takashi Iwai <tiwai@suse.de> > Cc: alsa-devel@alsa-project.org > Signed-off-by: Ming Lei <ming.lei@canonical.com> > --- > sound/usb/usx2y/usbusx2yaudio.c | 4 ++++ > 1 file changed, 4 insertions(+) > diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c > index 4967fe9..e2ee893 100644 > --- a/sound/usb/usx2y/usbusx2yaudio.c > +++ b/sound/usb/usx2y/usbusx2yaudio.c > @@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev *usX2Y) > struct snd_usX2Y_substream *subs = usX2Y->subs[s]; > if (subs) { > if (atomic_read(&subs->state) >= state_PRERUNNING) { > + unsigned long flags; > + > + local_irq_save(flags); > snd_pcm_stop(subs->pcm_substream, SNDRV_PCM_STATE_XRUN); > + local_irq_restore(flags); > } WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At Thu, 11 Jul 2013 17:08:30 +0400, Sergei Shtylyov wrote: > > On 11-07-2013 13:06, Ming Lei wrote: > > > Complete() will be run with interrupt enabled, so change to > > spin_lock_irqsave(). > > Changelog doesn't match the patch. Yep, but moreover... > > Cc: Jaroslav Kysela <perex@perex.cz> > > Cc: Takashi Iwai <tiwai@suse.de> > > Cc: alsa-devel@alsa-project.org > > Signed-off-by: Ming Lei <ming.lei@canonical.com> > > --- > > sound/usb/usx2y/usbusx2yaudio.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c > > index 4967fe9..e2ee893 100644 > > --- a/sound/usb/usx2y/usbusx2yaudio.c > > +++ b/sound/usb/usx2y/usbusx2yaudio.c > > @@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev *usX2Y) > > struct snd_usX2Y_substream *subs = usX2Y->subs[s]; > > if (subs) { > > if (atomic_read(&subs->state) >= state_PRERUNNING) { > > + unsigned long flags; > > + > > + local_irq_save(flags); > > snd_pcm_stop(subs->pcm_substream, SNDRV_PCM_STATE_XRUN); > > + local_irq_restore(flags); > > } ... actually this snd_pcm_stop() call should have been covered by snd_pcm_stream_lock(). Maybe it'd be enough to have a single patch together with the change, i.e. wrapping with snd_pcm_stream_lock_irqsave(). I'll prepare the patch for 3.11 independently from your patch series, so please drop this one. BTW, the word "cleanup" in the subject is inappropriate. This is rather a fix together with the core change. And, I wonder whether we can take a safer approach. When the caller condition changed, we often introduced either a different ops (e.g. ioctl case) or a flag for the new condition, at least during the transition period. Last but not least, is a conversion to tasklet really preferred? tasklet is rather an obsoleted infrastructure nowadays, and people don't recommend to use it any longer, AFAIK. thanks, Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 11, 2013 at 9:50 PM, Takashi Iwai <tiwai@suse.de> wrote: > At Thu, 11 Jul 2013 17:08:30 +0400, > Sergei Shtylyov wrote: >> >> On 11-07-2013 13:06, Ming Lei wrote: >> >> > Complete() will be run with interrupt enabled, so change to >> > spin_lock_irqsave(). >> >> Changelog doesn't match the patch. > > Yep, but moreover... > >> > Cc: Jaroslav Kysela <perex@perex.cz> >> > Cc: Takashi Iwai <tiwai@suse.de> >> > Cc: alsa-devel@alsa-project.org >> > Signed-off-by: Ming Lei <ming.lei@canonical.com> >> > --- >> > sound/usb/usx2y/usbusx2yaudio.c | 4 ++++ >> > 1 file changed, 4 insertions(+) >> >> > diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c >> > index 4967fe9..e2ee893 100644 >> > --- a/sound/usb/usx2y/usbusx2yaudio.c >> > +++ b/sound/usb/usx2y/usbusx2yaudio.c >> > @@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev *usX2Y) >> > struct snd_usX2Y_substream *subs = usX2Y->subs[s]; >> > if (subs) { >> > if (atomic_read(&subs->state) >= state_PRERUNNING) { >> > + unsigned long flags; >> > + >> > + local_irq_save(flags); >> > snd_pcm_stop(subs->pcm_substream, SNDRV_PCM_STATE_XRUN); >> > + local_irq_restore(flags); >> > } > > ... actually this snd_pcm_stop() call should have been covered by > snd_pcm_stream_lock(). Maybe it'd be enough to have a single patch > together with the change, i.e. wrapping with > snd_pcm_stream_lock_irqsave(). Please use snd_pcm_stream_lock_irqsave() so that I can avoid sending out similar patch later, :-) > > I'll prepare the patch for 3.11 independently from your patch series, > so please drop this one. OK, thanks for dealing with that. > > > BTW, the word "cleanup" in the subject is inappropriate. This is > rather a fix together with the core change. It is a cleanup since the patchset only addresses lock problem which is caused by the tasklet conversion. > > And, I wonder whether we can take a safer approach. When the caller > condition changed, we often introduced either a different ops > (e.g. ioctl case) or a flag for the new condition, at least during the > transition period. Interrupt is't enabled until all current drivers are cleaned up, so it is really safe, please see patch [2]. > > Last but not least, is a conversion to tasklet really preferred? > tasklet is rather an obsoleted infrastructure nowadays, and people > don't recommend to use it any longer, AFAIK. We discussed the problem in the below link previously[1], Steven and Thomas suggested to use threaded irq handler, but which may degrade USB mass storage performance, so we have to take tasklet now until we rewrite transport part of USB mass storage driver. Also the conversion[2] has avoided the tasklet spin lock problem already. [1], http://marc.info/?t=137079119200001&r=1&w=2 [2], http://marc.info/?l=linux-usb&m=137286326726326&w=2 Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At Thu, 11 Jul 2013 22:13:35 +0800, Ming Lei wrote: > > On Thu, Jul 11, 2013 at 9:50 PM, Takashi Iwai <tiwai@suse.de> wrote: > > At Thu, 11 Jul 2013 17:08:30 +0400, > > Sergei Shtylyov wrote: > >> > >> On 11-07-2013 13:06, Ming Lei wrote: > >> > >> > Complete() will be run with interrupt enabled, so change to > >> > spin_lock_irqsave(). > >> > >> Changelog doesn't match the patch. > > > > Yep, but moreover... > > > >> > Cc: Jaroslav Kysela <perex@perex.cz> > >> > Cc: Takashi Iwai <tiwai@suse.de> > >> > Cc: alsa-devel@alsa-project.org > >> > Signed-off-by: Ming Lei <ming.lei@canonical.com> > >> > --- > >> > sound/usb/usx2y/usbusx2yaudio.c | 4 ++++ > >> > 1 file changed, 4 insertions(+) > >> > >> > diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c > >> > index 4967fe9..e2ee893 100644 > >> > --- a/sound/usb/usx2y/usbusx2yaudio.c > >> > +++ b/sound/usb/usx2y/usbusx2yaudio.c > >> > @@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev *usX2Y) > >> > struct snd_usX2Y_substream *subs = usX2Y->subs[s]; > >> > if (subs) { > >> > if (atomic_read(&subs->state) >= state_PRERUNNING) { > >> > + unsigned long flags; > >> > + > >> > + local_irq_save(flags); > >> > snd_pcm_stop(subs->pcm_substream, SNDRV_PCM_STATE_XRUN); > >> > + local_irq_restore(flags); > >> > } > > > > ... actually this snd_pcm_stop() call should have been covered by > > snd_pcm_stream_lock(). Maybe it'd be enough to have a single patch > > together with the change, i.e. wrapping with > > snd_pcm_stream_lock_irqsave(). > > Please use snd_pcm_stream_lock_irqsave() so that I can avoid sending > out similar patch later, :-) > > > > > I'll prepare the patch for 3.11 independently from your patch series, > > so please drop this one. > > OK, thanks for dealing with that. > > > > > > > BTW, the word "cleanup" in the subject is inappropriate. This is > > rather a fix together with the core change. > > It is a cleanup since the patchset only addresses lock problem which > is caused by the tasklet conversion. Well, the conversion to irqsave() is needed for the future drop of irq_save() in the caller, right? Then this isn't a cleanup but a preparation for movement ahead. > > And, I wonder whether we can take a safer approach. When the caller > > condition changed, we often introduced either a different ops > > (e.g. ioctl case) or a flag for the new condition, at least during the > > transition period. > > Interrupt is't enabled until all current drivers are cleaned up, so it is really > safe, please see patch [2]. OK. > > Last but not least, is a conversion to tasklet really preferred? > > tasklet is rather an obsoleted infrastructure nowadays, and people > > don't recommend to use it any longer, AFAIK. > > We discussed the problem in the below link previously[1], Steven > and Thomas suggested to use threaded irq handler, but which > may degrade USB mass storage performance, so we have to > take tasklet now until we rewrite transport part of USB mass storage > driver. > > Also the conversion[2] has avoided the tasklet spin lock problem > already. OK, good to know. thanks, Takashi > [1], http://marc.info/?t=137079119200001&r=1&w=2 > [2], http://marc.info/?l=linux-usb&m=137286326726326&w=2 > > Thanks, > -- > Ming Lei > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 11, 2013 at 10:34 PM, Takashi Iwai <tiwai@suse.de> wrote: > At Thu, 11 Jul 2013 22:13:35 +0800, > Ming Lei wrote: >> >> On Thu, Jul 11, 2013 at 9:50 PM, Takashi Iwai <tiwai@suse.de> wrote: >> > At Thu, 11 Jul 2013 17:08:30 +0400, >> > Sergei Shtylyov wrote: >> >> >> >> On 11-07-2013 13:06, Ming Lei wrote: >> >> >> >> > Complete() will be run with interrupt enabled, so change to >> >> > spin_lock_irqsave(). >> >> >> >> Changelog doesn't match the patch. >> > >> > Yep, but moreover... >> > >> >> > Cc: Jaroslav Kysela <perex@perex.cz> >> >> > Cc: Takashi Iwai <tiwai@suse.de> >> >> > Cc: alsa-devel@alsa-project.org >> >> > Signed-off-by: Ming Lei <ming.lei@canonical.com> >> >> > --- >> >> > sound/usb/usx2y/usbusx2yaudio.c | 4 ++++ >> >> > 1 file changed, 4 insertions(+) >> >> >> >> > diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c >> >> > index 4967fe9..e2ee893 100644 >> >> > --- a/sound/usb/usx2y/usbusx2yaudio.c >> >> > +++ b/sound/usb/usx2y/usbusx2yaudio.c >> >> > @@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev *usX2Y) >> >> > struct snd_usX2Y_substream *subs = usX2Y->subs[s]; >> >> > if (subs) { >> >> > if (atomic_read(&subs->state) >= state_PRERUNNING) { >> >> > + unsigned long flags; >> >> > + >> >> > + local_irq_save(flags); >> >> > snd_pcm_stop(subs->pcm_substream, SNDRV_PCM_STATE_XRUN); >> >> > + local_irq_restore(flags); >> >> > } >> > >> > ... actually this snd_pcm_stop() call should have been covered by >> > snd_pcm_stream_lock(). Maybe it'd be enough to have a single patch >> > together with the change, i.e. wrapping with >> > snd_pcm_stream_lock_irqsave(). >> >> Please use snd_pcm_stream_lock_irqsave() so that I can avoid sending >> out similar patch later, :-) >> >> > >> > I'll prepare the patch for 3.11 independently from your patch series, >> > so please drop this one. >> >> OK, thanks for dealing with that. >> >> > >> > >> > BTW, the word "cleanup" in the subject is inappropriate. This is >> > rather a fix together with the core change. >> >> It is a cleanup since the patchset only addresses lock problem which >> is caused by the tasklet conversion. > > Well, the conversion to irqsave() is needed for the future drop of > irq_save() in the caller, right? Then this isn't a cleanup but a > preparation for movement ahead. Sounds more accurate, and I will change the title in next round, :-) Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c index 4967fe9..e2ee893 100644 --- a/sound/usb/usx2y/usbusx2yaudio.c +++ b/sound/usb/usx2y/usbusx2yaudio.c @@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev *usX2Y) struct snd_usX2Y_substream *subs = usX2Y->subs[s]; if (subs) { if (atomic_read(&subs->state) >= state_PRERUNNING) { + unsigned long flags; + + local_irq_save(flags); snd_pcm_stop(subs->pcm_substream, SNDRV_PCM_STATE_XRUN); + local_irq_restore(flags); } for (u = 0; u < NRURBS; u++) { struct urb *urb = subs->urb[u];
Complete() will be run with interrupt enabled, so change to spin_lock_irqsave(). Cc: Jaroslav Kysela <perex@perex.cz> Cc: Takashi Iwai <tiwai@suse.de> Cc: alsa-devel@alsa-project.org Signed-off-by: Ming Lei <ming.lei@canonical.com> --- sound/usb/usx2y/usbusx2yaudio.c | 4 ++++ 1 file changed, 4 insertions(+)