Skip to contentSkip to conversation

v4l: omap4iss: Restore irq flags correctly in omap4iss_video_buffer_next()

PATCH In review Dan Carpenter 2 participants 3 messages Dec 16, 2013 – Dec 19, 2013

Subsystems (3)
Media (V4L/DVB) Staging Kernel janitors
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH v2 1/1 v4l: omap4iss: Restore irq flags correctly in omap4iss_video_buffer_next()

The spin_lock_irqsave() macro is not nestable. The second call will overwrite the first record of "flags" so the IRQs will not be enabled correctly at the end of the function.

I haven't looked at all the callers but it could be that this function is always called with interrupts disabled and the bug doesn't cause problems in real life.

Signed-off-by Dan Carpenter <[email protected]>
drivers/staging/media/omap4iss/iss_video.c +2 -2
index 766491e6a8d0..c9b71c750b15 100644
@@ -451,9 +451,9 @@ struct iss_buffer *omap4iss_video_buffer_next(struct iss_video *video)
 	}
 
 	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE && pipe->input != NULL) {
-		spin_lock_irqsave(&pipe->lock, flags);
+		spin_lock(&pipe->lock);
 		pipe->state &= ~ISS_PIPELINE_STREAM;
-		spin_unlock_irqrestore(&pipe->lock, flags);
+		spin_unlock(&pipe->lock);
 	}
 
 	buf = list_first_entry(&video->dmaqueue, struct iss_buffer,
#1 lore raw

Hi Dan,

Thank you for the patch.

Quoted 7 lines · On Monday 16 December 2013 18:06:12 Dan Carpenter wrote:

The spin_lock_irqsave() macro is not nestable. The second call will overwrite the first record of "flags" so the IRQs will not be enabled correctly at the end of the function.

I haven't looked at all the callers but it could be that this function is always called with interrupts disabled and the bug doesn't cause problems in real life.

The function is called from interrupt handlers only. Would you like to rephrase the commit message accordingly ?

Quoted 19 lines
Signed-off-by Dan Carpenter <[email protected]>
(quoted diff) +0 -0

b/drivers/staging/media/omap4iss/iss_video.c index 766491e6a8d0..c9b71c750b15 100644

drivers/staging/media/omap4iss/iss_video.c +2 -2
@@ -451,9 +451,9 @@ struct iss_buffer *omap4iss_video_buffer_next(struct
 iss_video *video) }
 
 	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE && pipe->input != NULL) {
-		spin_lock_irqsave(&pipe->lock, flags);
+		spin_lock(&pipe->lock);
 		pipe->state &= ~ISS_PIPELINE_STREAM;
-		spin_unlock_irqrestore(&pipe->lock, flags);
+		spin_unlock(&pipe->lock);
 	}
 
 	buf = list_first_entry(&video->dmaqueue, struct iss_buffer,
Signature · 3 lines
Regards,

Laurent Pinchart
Acked-by on v2 1/1
#2 lore raw
In reply to Dan Carpenter

Hi Dan,

Thank you for the patch.

Quoted 8 lines · On Wednesday 18 December 2013 10:29:24 Dan Carpenter wrote:

The spin_lock_irqsave() macro is not nestable. The second call will overwrite the first record of "flags" so the IRQs will not be enabled correctly at the end of the function.

In the current code, this function is always called from the IRQ handler so everything works fine and this fix doesn't change anything.

Signed-off-by Dan Carpenter <[email protected]>
Acked-by Laurent Pinchart <[email protected]>

and applied to my tree. I'll send a pull request shortly.

Quoted 20 lines

---
v2: Updated the change log

(quoted diff) +0 -0

b/drivers/staging/media/omap4iss/iss_video.c index 766491e6a8d0..c9b71c750b15 100644

drivers/staging/media/omap4iss/iss_video.c +2 -2
@@ -451,9 +451,9 @@ struct iss_buffer *omap4iss_video_buffer_next(struct
 iss_video *video) }
 
 	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE && pipe->input != NULL) {
-		spin_lock_irqsave(&pipe->lock, flags);
+		spin_lock(&pipe->lock);
 		pipe->state &= ~ISS_PIPELINE_STREAM;
-		spin_unlock_irqrestore(&pipe->lock, flags);
+		spin_unlock(&pipe->lock);
 	}
 
 	buf = list_first_entry(&video->dmaqueue, struct iss_buffer,
Signature · 3 lines
Regards,

Laurent Pinchart