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