From: Takashi Sakamoto <o-takashi@sakamocchi.jp> To: Takashi Iwai <tiwai@suse.de> Cc: perex@perex.cz, anna-maria@linutronix.de, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, peterz@infradead.org, mingo@redhat.com, hch@lst.org, keescook@chromium.org, john.stultz@linaro.org, tglx@linutronix.de Subject: Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer Date: Sat, 2 Sep 2017 10:19:45 +0900 [thread overview] Message-ID: <19775981-a3f2-5f65-e934-bfe25db62a43@sakamocchi.jp> (raw) In-Reply-To: <s5hbmmutytt.wl-tiwai@suse.de> On p 1 2017 20:58, Takashi Iwai wrote: >> >From 07d61ba2a1c0e06e914443225e194d99f2d8c58d Mon Sep 17 00:00:00 2001 >> From: Takashi Sakamoto <o-takashi@sakamocchi.jp> >> Date: Fri, 1 Sep 2017 19:10:18 +0900 >> Subject: [PATCH] ALSA: dummy: avoid stall due to a call of hrtimer_cancel() on >> a callback of hrtimer >> >> A call of 'htrimer_cancel()' on a callback of hrtimer brings endless loop >> because 'struct hrtimer_clock_base.running' is not NULL on the callback. >> In hrtimer subsystem, this member is used to indicate the instance of >> hrtimer gets callbacks and there's a helper function, >> 'hrtimer_callback_running()' to check it. >> >> ALSA dummy driver uses hrtimer to emulate hardware interrupt per period >> of PCM buffer. When XRUN occurs on PCM substream, in a call of >> 'snd_pcm_period_elapsed()', 'struct snd_pcm_ops.stop()' is called to >> stop the substream. In current implementation, 'hrtimer_cancel()' is >> used to wait for cancellation of hrtimer. However, as described, this >> brings endless loop. > > It's not only about XRUN. When the stream finishes the draining, it > stops the stream gracefully -- that is the very normal operation. I overlooked it. Thanks for your indication. >> For this problem, this commit uses 'hrtimer_callback_running()' to >> detect whether to be on a callback of hrtimer or not, then skip >> cancellation of hrtimer in hrtimer callbacks. Furthermore, at a case of >> XRUN, hrtimer callback returns HRTIMER_NORESTART after a call of >> 'snd_pcm_period_elapsed()' to discontinue hrtimr because cancellation is >> skipped. >> >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > > It's better to fold the fix into the original patch instead of > introducing a bug and fixing it. Yep. I request the authors to include this fix. Well, in sound subsystem, there're a few drivers which uses hrtimer: - snd-pcsp - snd-sh-dac-audio - snd-soc-imx-pcm-fiq As a quick glance, 'snd-sh-dac-audio' includes the same bug, too. Additionally, 'snd-soc-imx-pcm-fiq' maintains hrtimer with loose manner in a point of state of PCM substream and it shall gain the same bug if improved. Later, I posted some patches for them. Thanks Takashi Sakamoto
WARNING: multiple messages have this Message-ID (diff)
From: Takashi Sakamoto <o-takashi@sakamocchi.jp> To: Takashi Iwai <tiwai@suse.de> Cc: alsa-devel@alsa-project.org, keescook@chromium.org, peterz@infradead.org, linux-kernel@vger.kernel.org, mingo@redhat.com, john.stultz@linaro.org, tglx@linutronix.de, hch@lst.org, anna-maria@linutronix.de Subject: Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer Date: Sat, 2 Sep 2017 10:19:45 +0900 [thread overview] Message-ID: <19775981-a3f2-5f65-e934-bfe25db62a43@sakamocchi.jp> (raw) In-Reply-To: <s5hbmmutytt.wl-tiwai@suse.de> On p 1 2017 20:58, Takashi Iwai wrote: >> >From 07d61ba2a1c0e06e914443225e194d99f2d8c58d Mon Sep 17 00:00:00 2001 >> From: Takashi Sakamoto <o-takashi@sakamocchi.jp> >> Date: Fri, 1 Sep 2017 19:10:18 +0900 >> Subject: [PATCH] ALSA: dummy: avoid stall due to a call of hrtimer_cancel() on >> a callback of hrtimer >> >> A call of 'htrimer_cancel()' on a callback of hrtimer brings endless loop >> because 'struct hrtimer_clock_base.running' is not NULL on the callback. >> In hrtimer subsystem, this member is used to indicate the instance of >> hrtimer gets callbacks and there's a helper function, >> 'hrtimer_callback_running()' to check it. >> >> ALSA dummy driver uses hrtimer to emulate hardware interrupt per period >> of PCM buffer. When XRUN occurs on PCM substream, in a call of >> 'snd_pcm_period_elapsed()', 'struct snd_pcm_ops.stop()' is called to >> stop the substream. In current implementation, 'hrtimer_cancel()' is >> used to wait for cancellation of hrtimer. However, as described, this >> brings endless loop. > > It's not only about XRUN. When the stream finishes the draining, it > stops the stream gracefully -- that is the very normal operation. I overlooked it. Thanks for your indication. >> For this problem, this commit uses 'hrtimer_callback_running()' to >> detect whether to be on a callback of hrtimer or not, then skip >> cancellation of hrtimer in hrtimer callbacks. Furthermore, at a case of >> XRUN, hrtimer callback returns HRTIMER_NORESTART after a call of >> 'snd_pcm_period_elapsed()' to discontinue hrtimr because cancellation is >> skipped. >> >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > > It's better to fold the fix into the original patch instead of > introducing a bug and fixing it. Yep. I request the authors to include this fix. Well, in sound subsystem, there're a few drivers which uses hrtimer: - snd-pcsp - snd-sh-dac-audio - snd-soc-imx-pcm-fiq As a quick glance, 'snd-sh-dac-audio' includes the same bug, too. Additionally, 'snd-soc-imx-pcm-fiq' maintains hrtimer with loose manner in a point of state of PCM substream and it shall gain the same bug if improved. Later, I posted some patches for them. Thanks Takashi Sakamoto
next prev parent reply other threads:[~2017-09-02 1:19 UTC|newest] Thread overview: 86+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-08-31 12:23 [PATCH 00/25] hrtimer: Provide softirq context hrtimers Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 01/25] hrtimer: Use predefined function for updating next_timer Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 02/25] hrtimer: Correct blantanly wrong comment Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 03/25] hrtimer: Fix kerneldoc for struct hrtimer_cpu_base Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 04/25] hrtimer: Cleanup clock argument in schedule_hrtimeout_range_clock() Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 05/25] hrtimer: Switch for loop to _ffs() evaluation Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 07/25] hrtimer: Reduce conditional code (hres_active) Anna-Maria Gleixner 2017-09-25 13:55 ` Peter Zijlstra 2017-09-25 14:28 ` Thomas Gleixner 2017-08-31 12:23 ` [PATCH 06/25] hrtimer: Store running timer in hrtimer_clock_base Anna-Maria Gleixner 2017-09-25 14:44 ` Peter Zijlstra 2017-09-28 12:45 ` Thomas Gleixner 2017-08-31 12:23 ` [PATCH 08/25] hrtimer: Reduce conditional code (expires_next, next_timer) Anna-Maria Gleixner 2017-09-26 12:10 ` Peter Zijlstra 2017-09-28 8:07 ` Thomas Gleixner 2017-08-31 12:23 ` [PATCH 09/25] hrtimer: Reduce conditional code (hrtimer_reprogram()) Anna-Maria Gleixner 2017-09-26 12:07 ` Peter Zijlstra 2017-08-31 12:23 ` [PATCH 10/25] hrtimer: Make handling of hrtimer reprogramming and enqueuing not conditional Anna-Maria Gleixner 2017-09-26 12:14 ` Peter Zijlstra 2017-09-28 8:09 ` Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 11/25] hrtimer: Allow remote hrtimer enqueue with "expires_next" as expiry time Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 13/25] hrtimer: Split out code from hrtimer_start_range_ns() for reuse Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 12/25] hrtimer: Simplify hrtimer_reprogram() call Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 14/25] hrtimer: Split out code from __hrtimer_get_next_event() for reuse Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 15/25] hrtimer: Add clock bases for soft irq context Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 16/25] hrtimer: Allow function reuse for softirq based hrtimer Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 18/25] hrtimer: Enable soft and hard hrtimer Anna-Maria Gleixner 2017-09-26 12:52 ` Peter Zijlstra 2017-09-27 14:18 ` Anna-Maria Gleixner 2017-09-27 15:54 ` Thomas Gleixner 2017-09-27 16:47 ` Peter Zijlstra 2017-08-31 12:23 ` [PATCH 17/25] hrtimer: Implementation of softirq hrtimer handling Anna-Maria Gleixner 2017-09-26 12:40 ` Peter Zijlstra 2017-09-26 15:03 ` Peter Zijlstra 2017-09-27 14:22 ` Anna-Maria Gleixner 2017-09-27 16:46 ` Peter Zijlstra 2017-09-27 16:40 ` Peter Zijlstra 2017-09-28 7:59 ` Thomas Gleixner 2017-09-28 8:15 ` Peter Zijlstra 2017-12-19 8:58 ` Sebastian Andrzej Siewior 2017-12-19 13:21 ` Peter Zijlstra 2017-12-20 8:44 ` Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 20/25] mac80211_hwsim: Replace hrtimer tasklet with softirq hrtimer Anna-Maria Gleixner 2017-08-31 12:23 ` Anna-Maria Gleixner 2017-09-05 7:03 ` Johannes Berg 2017-09-05 8:49 ` Thomas Gleixner 2017-09-05 8:51 ` Johannes Berg 2017-08-31 12:23 ` [PATCH 19/25] can/bcm: Replace hrtimer_tasklet with softirq based hrtimer Anna-Maria Gleixner 2017-09-01 15:49 ` Oliver Hartkopp 2017-09-01 15:56 ` Thomas Gleixner 2017-09-01 17:02 ` Oliver Hartkopp 2017-09-02 17:56 ` Oliver Hartkopp 2017-08-31 12:23 ` [PATCH 21/25] xfrm: Replace hrtimer tasklet with softirq hrtimer Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 23/25] ALSA/dummy: Replace " Anna-Maria Gleixner 2017-08-31 14:21 ` Takashi Sakamoto 2017-08-31 14:21 ` Takashi Sakamoto 2017-08-31 14:26 ` Takashi Iwai 2017-08-31 14:26 ` Takashi Iwai 2017-08-31 15:36 ` Takashi Iwai 2017-08-31 15:36 ` Takashi Iwai 2017-09-01 10:25 ` Takashi Sakamoto 2017-09-01 10:25 ` Takashi Sakamoto 2017-09-01 11:58 ` Takashi Iwai 2017-09-01 11:58 ` Takashi Iwai 2017-09-02 1:19 ` Takashi Sakamoto [this message] 2017-09-02 1:19 ` Takashi Sakamoto 2017-09-04 12:45 ` Takashi Sakamoto 2017-09-05 15:53 ` [PATCH 23/25 v2] " Sebastian Andrzej Siewior 2017-09-05 15:53 ` Sebastian Andrzej Siewior 2017-09-05 16:02 ` Takashi Iwai 2017-09-05 16:02 ` Takashi Iwai 2017-09-05 16:05 ` Takashi Sakamoto 2017-09-05 16:18 ` [PATCH 23/25 v3] " Sebastian Andrzej Siewior 2017-09-05 16:18 ` Sebastian Andrzej Siewior 2017-09-06 4:30 ` Takashi Sakamoto 2017-09-06 4:30 ` Takashi Sakamoto 2017-09-08 8:28 ` [alsa-devel] " Takashi Iwai 2017-09-08 8:28 ` Takashi Iwai 2017-08-31 12:23 ` [PATCH 22/25] softirq: Remove tasklet_hrtimer Anna-Maria Gleixner 2017-08-31 12:23 ` [PATCH 25/25] usb/gadget/NCM: Replace tasklet with softirq hrtimer Anna-Maria Gleixner 2017-10-24 9:45 ` Felipe Balbi 2017-08-31 12:23 ` [PATCH 24/25] net/cdc_ncm: " Anna-Maria Gleixner 2017-08-31 13:33 ` Greg Kroah-Hartman 2017-08-31 13:57 ` Bjørn Mork 2017-09-05 15:42 ` [PATCH 24/25 v2] " Sebastian Andrzej Siewior 2017-08-31 12:36 ` [PATCH 00/25] hrtimer: Provide softirq context hrtimers Anna-Maria Gleixner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=19775981-a3f2-5f65-e934-bfe25db62a43@sakamocchi.jp \ --to=o-takashi@sakamocchi.jp \ --cc=alsa-devel@alsa-project.org \ --cc=anna-maria@linutronix.de \ --cc=hch@lst.org \ --cc=john.stultz@linaro.org \ --cc=keescook@chromium.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mingo@redhat.com \ --cc=perex@perex.cz \ --cc=peterz@infradead.org \ --cc=tglx@linutronix.de \ --cc=tiwai@suse.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.