[PATCH 0/6] ALSA: pcm: check parameter on snd_pcm_running()

[PATCH 0/6] ALSA: pcm: check parameter on snd_pcm_running()

[Kuninori Morimoto]

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(-)

-- 
1.9.1

[PATCH 1/6] ALSA: pcm: check parameter on snd_pcm_running()

[Kuninori Morimoto]

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));
-- 
1.9.1

[PATCH 2/6] ALSA: pdaudiocf: remove unneeded check for snd_pcm_running()

[Kuninori Morimoto]

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);
-- 
1.9.1

[PATCH 3/6] ASoC: dwc: remove unneeded check for snd_pcm_running()

[Kuninori Morimoto]

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;
-- 
1.9.1

[PATCH 4/6] ASoC: omap-hdmi-audio: remove unneeded check for snd_pcm_running()

[Kuninori Morimoto]

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);
-- 
1.9.1

[PATCH 5/6] ASoC: xtfpga-i2s: remove unneeded check for snd_pcm_running()

[Kuninori Morimoto]

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",
-- 
1.9.1

[PATCH 6/6] ASoC: rsnd: remove unneeded check for snd_pcm_running()

[Kuninori Morimoto]

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)
-- 
1.9.1

Re: [PATCH 0/6] ALSA: pcm: check parameter on snd_pcm_running()

[Takashi Sakamoto]

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

Re: [PATCH 0/6] ALSA: pcm: check parameter on snd_pcm_running()

[Takashi Iwai]

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

Re: [PATCH 0/6] ALSA: pcm: check parameter on snd_pcm_running()

[Kuninori Morimoto]

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

Re: [PATCH 0/6] ALSA: pcm: check parameter on snd_pcm_running()

[Takashi Iwai]

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
> 

Re: [PATCH 0/6] ALSA: pcm: check parameter on snd_pcm_running()

[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