[alsa-devel] [PATCH 0/6] ALSA: pcm: check parameter on snd_pcm_running()
Hi Takashi-san, Mark
snd_pcm_running() is using "substream" and "substream->runtime" pointer, no check. These patches adds its check in function, and removes duplicate checks from each drivers.
Not super important, but can be cleanup
Kuninori Morimoto (6): ALSA: pcm: check parameter on snd_pcm_running() ALSA: pdaudiocf: remove unneeded check for snd_pcm_running() ASoC: dwc: remove unneeded check for snd_pcm_running() ASoC: omap-hdmi-audio: remove unneeded check for snd_pcm_running() ASoC: xtfpga-i2s: remove unneeded check for snd_pcm_running() ASoC: rsnd: remove unneeded check for snd_pcm_running()
include/sound/pcm.h | 3 +++ sound/pcmcia/pdaudiocf/pdaudiocf_irq.c | 2 +- sound/soc/dwc/dwc-pcm.c | 2 +- sound/soc/omap/omap-hdmi-audio.c | 3 +-- sound/soc/sh/rcar/core.c | 5 +---- sound/soc/xtensa/xtfpga-i2s.c | 4 ++-- 6 files changed, 9 insertions(+), 10 deletions(-)
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
snd_pcm_running() is using "substream" and "substream->runtime" pointer. Let's check it before use it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/pcm.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 24febf9..a8e49f5 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -664,6 +664,9 @@ void snd_pcm_stream_unlock_irqrestore(struct snd_pcm_substream *substream, */ static inline int snd_pcm_running(struct snd_pcm_substream *substream) { + if (!substream || !substream->runtime) + return 0; + return (substream->runtime->status->state == SNDRV_PCM_STATE_RUNNING || (substream->runtime->status->state == SNDRV_PCM_STATE_DRAINING && substream->stream == SNDRV_PCM_STREAM_PLAYBACK));
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
snd_pcm_running() itself is checking parameter now. Let's remove duplicate check
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/pcmcia/pdaudiocf/pdaudiocf_irq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/pcmcia/pdaudiocf/pdaudiocf_irq.c b/sound/pcmcia/pdaudiocf/pdaudiocf_irq.c index ecf0fbd..9377505 100644 --- a/sound/pcmcia/pdaudiocf/pdaudiocf_irq.c +++ b/sound/pcmcia/pdaudiocf/pdaudiocf_irq.c @@ -265,7 +265,7 @@ irqreturn_t pdacf_threaded_irq(int irq, void *dev) if ((chip->chip_status & (PDAUDIOCF_STAT_IS_STALE|PDAUDIOCF_STAT_IS_CONFIGURED)) != PDAUDIOCF_STAT_IS_CONFIGURED) return IRQ_HANDLED; - if (chip->pcm_substream == NULL || chip->pcm_substream->runtime == NULL || !snd_pcm_running(chip->pcm_substream)) + if (!snd_pcm_running(chip->pcm_substream)) return IRQ_HANDLED;
rdp = inw(chip->port + PDAUDIOCF_REG_RDP);
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
snd_pcm_running() itself is checking parameter now. Let's remove duplicate check
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/dwc/dwc-pcm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/dwc/dwc-pcm.c b/sound/soc/dwc/dwc-pcm.c index 2cc9632..1bab46c 100644 --- a/sound/soc/dwc/dwc-pcm.c +++ b/sound/soc/dwc/dwc-pcm.c @@ -102,7 +102,7 @@ static void dw_pcm_transfer(struct dw_i2s_dev *dev, bool push) substream = rcu_dereference(dev->tx_substream); else substream = rcu_dereference(dev->rx_substream); - active = substream && snd_pcm_running(substream); + active = snd_pcm_running(substream); if (active) { unsigned int ptr; unsigned int new_ptr;
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
snd_pcm_running() itself is checking parameter now. Let's remove duplicate check
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/omap/omap-hdmi-audio.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/omap/omap-hdmi-audio.c b/sound/soc/omap/omap-hdmi-audio.c index 8eeac7c..ab2f8d7 100644 --- a/sound/soc/omap/omap-hdmi-audio.c +++ b/sound/soc/omap/omap-hdmi-audio.c @@ -58,8 +58,7 @@ static void hdmi_dai_abort(struct device *dev) struct hdmi_audio_data *ad = dev_get_drvdata(dev);
mutex_lock(&ad->current_stream_lock); - if (ad->current_stream && ad->current_stream->runtime && - snd_pcm_running(ad->current_stream)) { + if (snd_pcm_running(ad->current_stream)) { dev_err(dev, "HDMI display disabled, aborting playback\n"); snd_pcm_stream_lock_irq(ad->current_stream); snd_pcm_stop(ad->current_stream, SNDRV_PCM_STATE_DISCONNECTED);
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
snd_pcm_running() itself is checking parameter now. Let's remove duplicate check
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/xtensa/xtfpga-i2s.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/xtensa/xtfpga-i2s.c b/sound/soc/xtensa/xtfpga-i2s.c index bc3151c..bf0c7b7 100644 --- a/sound/soc/xtensa/xtfpga-i2s.c +++ b/sound/soc/xtensa/xtfpga-i2s.c @@ -163,7 +163,7 @@ static bool xtfpga_pcm_push_tx(struct xtfpga_i2s *i2s)
rcu_read_lock(); tx_substream = rcu_dereference(i2s->tx_substream); - tx_active = tx_substream && snd_pcm_running(tx_substream); + tx_active = snd_pcm_running(tx_substream); if (tx_active) { unsigned tx_ptr = ACCESS_ONCE(i2s->tx_ptr); unsigned new_tx_ptr = i2s->tx_fn(i2s, tx_substream->runtime, @@ -254,7 +254,7 @@ static irqreturn_t xtfpga_i2s_threaded_irq_handler(int irq, void *dev_id) rcu_read_lock(); tx_substream = rcu_dereference(i2s->tx_substream);
- if (tx_substream && snd_pcm_running(tx_substream)) { + if (snd_pcm_running(tx_substream)) { snd_pcm_period_elapsed(tx_substream); if (int_status & XTFPGA_I2S_INT_UNDERRUN) dev_dbg_ratelimited(i2s->dev, "%s: underrun\n",
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
snd_pcm_running() itself is checking parameter now. Let's remove duplicate check
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/rcar/core.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c index 9572e23..8cd900d 100644 --- a/sound/soc/sh/rcar/core.c +++ b/sound/soc/sh/rcar/core.c @@ -191,10 +191,7 @@ void rsnd_mod_interrupt(struct rsnd_mod *mod, int rsnd_io_is_working(struct rsnd_dai_stream *io) { /* see rsnd_dai_stream_init/quit() */ - if (io->substream) - return snd_pcm_running(io->substream); - - return 0; + return snd_pcm_running(io->substream); }
int rsnd_runtime_channel_original(struct rsnd_dai_stream *io)
On Nov 9 2017 11:11, Kuninori Morimoto wrote:
Hi Takashi-san, Mark
snd_pcm_running() is using "substream" and "substream->runtime" pointer, no check. These patches adds its check in function, and removes duplicate checks from each drivers.
Not super important, but can be cleanup
Kuninori Morimoto (6): ALSA: pcm: check parameter on snd_pcm_running() ALSA: pdaudiocf: remove unneeded check for snd_pcm_running() ASoC: dwc: remove unneeded check for snd_pcm_running() ASoC: omap-hdmi-audio: remove unneeded check for snd_pcm_running() ASoC: xtfpga-i2s: remove unneeded check for snd_pcm_running() ASoC: rsnd: remove unneeded check for snd_pcm_running()
include/sound/pcm.h | 3 +++ sound/pcmcia/pdaudiocf/pdaudiocf_irq.c | 2 +- sound/soc/dwc/dwc-pcm.c | 2 +- sound/soc/omap/omap-hdmi-audio.c | 3 +-- sound/soc/sh/rcar/core.c | 5 +---- sound/soc/xtensa/xtfpga-i2s.c | 4 ++-- 6 files changed, 9 insertions(+), 10 deletions(-)
This is a bad direction. I exactly oppose to your idea.
include/sound/pcm.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 24febf9..a8e49f5 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -664,6 +664,9 @@ void snd_pcm_stream_unlock_irqrestore(struct
snd_pcm_substream *substream,
*/ static inline int snd_pcm_running(struct snd_pcm_substream *substream) {
if (!substream || !substream->runtime)return 0;return (substream->runtime->status->state ==
SNDRV_PCM_STATE_RUNNING ||
(substream->runtime->status->state ==
SNDRV_PCM_STATE_DRAINING &&
substream->stream == SNDRV_PCM_STREAM_PLAYBACK));
In a view of 'design by contract', this function has a pre-condition that a given argument should not be NULL. Callers _should_ guarantee it to keep semantics of this function.
Your idea appends the duty of callers to this function. This causes a semantical contradiction. If it were something to bring kernel corruption such as BUG_ON(), the original design would be kept. When substream is NULL, it's a bug of drivers in adding PCM components. When runtime is NULL, it's a bug of ALSA PCM core in handling open system call.
Regards
Takashi Sakamoto
On Thu, 09 Nov 2017 03:40:22 +0100, Takashi Sakamoto wrote:
On Nov 9 2017 11:11, Kuninori Morimoto wrote:
Hi Takashi-san, Mark
snd_pcm_running() is using "substream" and "substream->runtime" pointer, no check. These patches adds its check in function, and removes duplicate checks from each drivers.
Not super important, but can be cleanup
Kuninori Morimoto (6): ALSA: pcm: check parameter on snd_pcm_running() ALSA: pdaudiocf: remove unneeded check for snd_pcm_running() ASoC: dwc: remove unneeded check for snd_pcm_running() ASoC: omap-hdmi-audio: remove unneeded check for snd_pcm_running() ASoC: xtfpga-i2s: remove unneeded check for snd_pcm_running() ASoC: rsnd: remove unneeded check for snd_pcm_running()
include/sound/pcm.h | 3 +++ sound/pcmcia/pdaudiocf/pdaudiocf_irq.c | 2 +- sound/soc/dwc/dwc-pcm.c | 2 +- sound/soc/omap/omap-hdmi-audio.c | 3 +-- sound/soc/sh/rcar/core.c | 5 +---- sound/soc/xtensa/xtfpga-i2s.c | 4 ++-- 6 files changed, 9 insertions(+), 10 deletions(-)
This is a bad direction. I exactly oppose to your idea.
include/sound/pcm.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 24febf9..a8e49f5 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -664,6 +664,9 @@ void snd_pcm_stream_unlock_irqrestore(struct
snd_pcm_substream *substream,
*/ static inline int snd_pcm_running(struct snd_pcm_substream *substream) {
if (!substream || !substream->runtime)return 0;return (substream->runtime->status->state ==SNDRV_PCM_STATE_RUNNING ||
(substream->runtime->status->state ==SNDRV_PCM_STATE_DRAINING &&
substream->stream == SNDRV_PCM_STREAM_PLAYBACK));In a view of 'design by contract', this function has a pre-condition that a given argument should not be NULL. Callers _should_ guarantee it to keep semantics of this function.
Generally I agree, but note that it depends on the exposure of the API function itself. If the API function is supposed to be an interface directly communicated with the outside, it's not seldom to allow NULL there. An implicit NULL-check would be handy and often makes coding easier (see the case of free()).
Your idea appends the duty of callers to this function. This causes a semantical contradiction. If it were something to bring kernel corruption such as BUG_ON(), the original design would be kept. When substream is NULL, it's a bug of drivers in adding PCM components. When runtime is NULL, it's a bug of ALSA PCM core in handling open system call.
When you call snd_pcm_running(), basically you're evaluating the PCM stream status, and likely a state machine. It often assumes that PCM state is consistent during the following action, and it implies the PCM stream lock was acquired. And, of course, PCM stream lock requires the non-NULL substream.
That said, if the code has a proper protection for the PCM stream consistency, the substream NULL check had to be done far before that point due to a stream lock.
Though, most codes aren't super-critical about the state change and the direct snd_pcm_running() works in most cases. But in the perfect world, stream locking is preferred around the state evaluation and the action according to it.
thanks,
Takashi
Hi Mark, Takashi-san
I wonder are these patches acceptable ? or not ? If acceptable, should I resend ?
Takashi Sakamoto wrote:
On Nov 9 2017 11:11, Kuninori Morimoto wrote:
Hi Takashi-san, Mark
snd_pcm_running() is using "substream" and "substream->runtime" pointer, no check. These patches adds its check in function, and removes duplicate checks from each drivers.
Not super important, but can be cleanup
Kuninori Morimoto (6): ALSA: pcm: check parameter on snd_pcm_running() ALSA: pdaudiocf: remove unneeded check for snd_pcm_running() ASoC: dwc: remove unneeded check for snd_pcm_running() ASoC: omap-hdmi-audio: remove unneeded check for snd_pcm_running() ASoC: xtfpga-i2s: remove unneeded check for snd_pcm_running() ASoC: rsnd: remove unneeded check for snd_pcm_running()
include/sound/pcm.h | 3 +++ sound/pcmcia/pdaudiocf/pdaudiocf_irq.c | 2 +- sound/soc/dwc/dwc-pcm.c | 2 +- sound/soc/omap/omap-hdmi-audio.c | 3 +-- sound/soc/sh/rcar/core.c | 5 +---- sound/soc/xtensa/xtfpga-i2s.c | 4 ++-- 6 files changed, 9 insertions(+), 10 deletions(-)
This is a bad direction. I exactly oppose to your idea.
include/sound/pcm.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 24febf9..a8e49f5 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -664,6 +664,9 @@ void snd_pcm_stream_unlock_irqrestore(struct
snd_pcm_substream *substream,
*/ static inline int snd_pcm_running(struct snd_pcm_substream *substream) {
if (!substream || !substream->runtime)return 0;return (substream->runtime->status->state ==SNDRV_PCM_STATE_RUNNING ||
(substream->runtime->status->state ==SNDRV_PCM_STATE_DRAINING &&
substream->stream == SNDRV_PCM_STREAM_PLAYBACK));In a view of 'design by contract', this function has a pre-condition that a given argument should not be NULL. Callers _should_ guarantee it to keep semantics of this function.
Generally I agree, but note that it depends on the exposure of the API function itself. If the API function is supposed to be an interface directly communicated with the outside, it's not seldom to allow NULL there. An implicit NULL-check would be handy and often makes coding easier (see the case of free()).
Your idea appends the duty of callers to this function. This causes a semantical contradiction. If it were something to bring kernel corruption such as BUG_ON(), the original design would be kept. When substream is NULL, it's a bug of drivers in adding PCM components. When runtime is NULL, it's a bug of ALSA PCM core in handling open system call.
When you call snd_pcm_running(), basically you're evaluating the PCM stream status, and likely a state machine. It often assumes that PCM state is consistent during the following action, and it implies the PCM stream lock was acquired. And, of course, PCM stream lock requires the non-NULL substream.
That said, if the code has a proper protection for the PCM stream consistency, the substream NULL check had to be done far before that point due to a stream lock.
Though, most codes aren't super-critical about the state change and the direct snd_pcm_running() works in most cases. But in the perfect world, stream locking is preferred around the state evaluation and the action according to it.
thanks,
Takashi
Best regards --- Kuninori Morimoto
On Tue, 28 Nov 2017 07:33:24 +0100, Kuninori Morimoto wrote:
Hi Mark, Takashi-san
I wonder are these patches acceptable ? or not ? If acceptable, should I resend ?
Since the use-case to pass NULL is limited, I'd rather avoid the change. If there were more than handful users, maybe I would rethink, but currently not convincing enough.
And, the NULL check is needed only when the caller is outside the stream lock. Such a situation already smells like a buggy code. We don't need to make it easier :)
thanks,
Takashi
Takashi Sakamoto wrote:
On Nov 9 2017 11:11, Kuninori Morimoto wrote:
Hi Takashi-san, Mark
snd_pcm_running() is using "substream" and "substream->runtime" pointer, no check. These patches adds its check in function, and removes duplicate checks from each drivers.
Not super important, but can be cleanup
Kuninori Morimoto (6): ALSA: pcm: check parameter on snd_pcm_running() ALSA: pdaudiocf: remove unneeded check for snd_pcm_running() ASoC: dwc: remove unneeded check for snd_pcm_running() ASoC: omap-hdmi-audio: remove unneeded check for snd_pcm_running() ASoC: xtfpga-i2s: remove unneeded check for snd_pcm_running() ASoC: rsnd: remove unneeded check for snd_pcm_running()
include/sound/pcm.h | 3 +++ sound/pcmcia/pdaudiocf/pdaudiocf_irq.c | 2 +- sound/soc/dwc/dwc-pcm.c | 2 +- sound/soc/omap/omap-hdmi-audio.c | 3 +-- sound/soc/sh/rcar/core.c | 5 +---- sound/soc/xtensa/xtfpga-i2s.c | 4 ++-- 6 files changed, 9 insertions(+), 10 deletions(-)
This is a bad direction. I exactly oppose to your idea.
include/sound/pcm.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 24febf9..a8e49f5 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -664,6 +664,9 @@ void snd_pcm_stream_unlock_irqrestore(struct
snd_pcm_substream *substream,
*/ static inline int snd_pcm_running(struct snd_pcm_substream *substream) {
if (!substream || !substream->runtime)return 0;return (substream->runtime->status->state ==SNDRV_PCM_STATE_RUNNING ||
(substream->runtime->status->state ==SNDRV_PCM_STATE_DRAINING &&
substream->stream == SNDRV_PCM_STREAM_PLAYBACK));In a view of 'design by contract', this function has a pre-condition that a given argument should not be NULL. Callers _should_ guarantee it to keep semantics of this function.
Generally I agree, but note that it depends on the exposure of the API function itself. If the API function is supposed to be an interface directly communicated with the outside, it's not seldom to allow NULL there. An implicit NULL-check would be handy and often makes coding easier (see the case of free()).
Your idea appends the duty of callers to this function. This causes a semantical contradiction. If it were something to bring kernel corruption such as BUG_ON(), the original design would be kept. When substream is NULL, it's a bug of drivers in adding PCM components. When runtime is NULL, it's a bug of ALSA PCM core in handling open system call.
When you call snd_pcm_running(), basically you're evaluating the PCM stream status, and likely a state machine. It often assumes that PCM state is consistent during the following action, and it implies the PCM stream lock was acquired. And, of course, PCM stream lock requires the non-NULL substream.
That said, if the code has a proper protection for the PCM stream consistency, the substream NULL check had to be done far before that point due to a stream lock.
Though, most codes aren't super-critical about the state change and the direct snd_pcm_running() works in most cases. But in the perfect world, stream locking is preferred around the state evaluation and the action according to it.
thanks,
Takashi
Best regards
Kuninori Morimoto
Hi Takashi
I wonder are these patches acceptable ? or not ? If acceptable, should I resend ?
Since the use-case to pass NULL is limited, I'd rather avoid the change. If there were more than handful users, maybe I would rethink, but currently not convincing enough.
And, the NULL check is needed only when the caller is outside the stream lock. Such a situation already smells like a buggy code. We don't need to make it easier :)
OK, I see Let's remove it so far Thanks
Best regards --- Kuninori Morimoto
participants (3)
-
Kuninori Morimoto -
Takashi Iwai -
Takashi Sakamoto