From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 0/6] ALSA: pcm: check parameter on snd_pcm_running() Date: Tue, 28 Nov 2017 07:42:48 +0100 Message-ID: References: <87r2t8p5tz.wl%kuninori.morimoto.gx@renesas.com> <0a642561-7c16-c54d-185b-3f39c5560738@sakamocchi.jp> <87a7z6udif.wl%kuninori.morimoto.gx@renesas.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id A69EA267252 for ; Tue, 28 Nov 2017 07:42:50 +0100 (CET) In-Reply-To: <87a7z6udif.wl%kuninori.morimoto.gx@renesas.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Kuninori Morimoto Cc: Linux-ALSA , Mark Brown , Simon , Takashi Sakamoto List-Id: alsa-devel@alsa-project.org 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 >