Skip to contentSkip to conversation

ASoC: dwc: Don't allow negative use counts

PATCH Superseded Andrew Jackson 3 participants 7 messages Dec 3, 2014 – Dec 4, 2014

Subsystems (2)
Sound ASoC

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Andrew Jackson

author
#0 lore raw

PATCH 2/5 ASoC: dwc: Don't allow negative use counts

Signed-off-by Andrew Jackson <[email protected]>
---
 sound/soc/dwc/designware_i2s.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
sound/soc/dwc/designware_i2s.c +2 -1
index 08f0229..f8946bd 100644
@@ -280,7 +280,8 @@ static int dw_i2s_trigger(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		dev->active--;
+		if (dev->active > 0)
+			dev->active--;
 		i2s_stop(dev, substream);
 		break;
 	default:
Signature · 1 line
1.7.1
maintainer
#1 lore raw

sound/soc/dwc/designware_i2s.c static int dw_i2s_trigger(struct snd_pcm_substream *substream, · line 284

Quoted 6 lines · Andrew Jackson
sound/soc/dwc/designware_i2s.c +2 -1
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		dev->active--;
+		if (dev->active > 0)
+			dev->active--;

How is this triggering - this sounds like you're papering over some other bug somewhere?

#2 lore raw
Quoted 11 lines · Mark Brown

sound/soc/dwc/designware_i2s.c static int dw_i2s_trigger(struct snd_pcm_substream *substream, · line 284

Quoted 6 lines · Andrew Jackson

case SNDRV_PCM_TRIGGER_STOP:
case SNDRV_PCM_TRIGGER_SUSPEND:
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:

sound/soc/dwc/designware_i2s.c +2 -1
-             dev->active--;
+             if (dev->active > 0)
+                     dev->active--;

How is this triggering - this sounds like you're papering over some other bug somewhere?

#3 lore raw
Quoted 10 lines · rajeev kumar
Quoted 9 lines · Mark Brown
Quoted 6 lines · Andrew Jackson

case SNDRV_PCM_TRIGGER_STOP:
case SNDRV_PCM_TRIGGER_SUSPEND:
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:

sound/soc/dwc/designware_i2s.c +2 -1
-             dev->active--;
+             if (dev->active > 0)
+                     dev->active--;
Quoted 2 lines
Quoted 2 lines

How is this triggering - this sounds like you're papering over some other bug somewhere?

This check can be removed as it is not going to triggered.

B'rgds
~Rajeev

author
#4 lore raw
Quoted 12 lines · rajeev kumar
Quoted 10 lines · rajeev kumar
Quoted 9 lines · Mark Brown
Quoted 6 lines · Andrew Jackson

case SNDRV_PCM_TRIGGER_STOP:
case SNDRV_PCM_TRIGGER_SUSPEND:
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:

sound/soc/dwc/designware_i2s.c +2 -1
-             dev->active--;
+             if (dev->active > 0)
+                     dev->active--;
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines

How is this triggering - this sounds like you're papering over some other bug somewhere?

This check can be removed as it is not going to triggered.

As I said in my email to Mark, I couldn't convince myself that STOP/SUSPEND would only be called once so it seemed like a loophole which /might/ result in the device not functioning correctly.

If this can never happen, I'll drop the patch.

Andrew

Quoted 3 lines

B'rgds
~Rajeev

author
#5 lore raw
In reply to Mark Brown
Quoted 12 lines · Mark Brown

sound/soc/dwc/designware_i2s.c static int dw_i2s_trigger(struct snd_pcm_substream *substream, · line 284

Quoted 6 lines · Andrew Jackson
sound/soc/dwc/designware_i2s.c +2 -1
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		dev->active--;
+		if (dev->active > 0)
+			dev->active--;

How is this triggering - this sounds like you're papering over some other bug somewhere?

When I looked at the code paths I couldn't convince myself that STOP wouldn't be called more than once. Then actuve would be negative and the device might not be restartable. I didn't have a problem per se, it was just that it seemed to be something of a loophole.

Andrew

maintainer
#6 lore raw
Quoted 13 lines · Andrew Jackson
Quoted 11 lines · Mark Brown

sound/soc/dwc/designware_i2s.c static int dw_i2s_trigger(struct snd_pcm_substream *substream, · line 284

Quoted 6 lines · Andrew Jackson
sound/soc/dwc/designware_i2s.c +2 -1
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		dev->active--;
+		if (dev->active > 0)
+			dev->active--;

How is this triggering - this sounds like you're papering over some other bug somewhere?

Quoted 4 lines

When I looked at the code paths I couldn't convince myself that STOP wouldn't be called more than once. Then actuve would be negative and the device might not be restartable. I didn't have a problem per se, it was just that it seemed to be something of a loophole.

If you're just adding the check on the off chance that it might fire you need to add a warning message as well - what your change does is make the code look like it's supposed to have broken reference counting since it has a check to silently fix up and ignore problems.