Skip to contentSkip to conversation

mx3-camera: locking typo in mx3_videobuf_queue()

PATCH Superseded Dan Carpenter 2 participants 5 messages Aug 23, 2013 – Aug 29, 2013

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

Dan Carpenter

author
#0 lore raw

PATCH 1/1 mx3-camera: locking typo in mx3_videobuf_queue()

There is a return in the middle where we haven't restored the IRQs to their original state.

Signed-off-by Dan Carpenter <[email protected]>
drivers/media/platform/soc_camera/mx3_camera.c +2 -2
index 1047e3e..4bae910 100644
@@ -334,7 +334,7 @@ static void mx3_videobuf_queue(struct vb2_buffer *vb)
 	if (!mx3_cam->active)
 		mx3_cam->active = buf;
 
-	spin_unlock_irq(&mx3_cam->lock);
+	spin_unlock_irqrestore(&mx3_cam->lock, flags);
 
 	cookie = txd->tx_submit(txd);
 	dev_dbg(icd->parent, "Submitted cookie %d DMA 0x%08x\n",

@@ -343,7 +343,7 @@ static void mx3_videobuf_queue(struct vb2_buffer *vb)
 	if (cookie >= 0)
 		return;
 
-	spin_lock_irq(&mx3_cam->lock);
+	spin_lock_irqsave(&mx3_cam->lock, flags);
 
 	/* Submit error */
 	list_del_init(&buf->queue);
#1 lore raw

Hi Dan,

Quoted 10 lines · Dan Carpenter

There is a return in the middle where we haven't restored the IRQs to their original state.

Signed-off-by Dan Carpenter <[email protected]>
drivers/media/platform/soc_camera/mx3_camera.c +0 -0
index 1047e3e..4bae910 100644

drivers/media/platform/soc_camera/mx3_camera.c static void mx3_videobuf_queue(struct vb2_buffer *vb) · line 340

Quoted 8 lines
drivers/media/platform/soc_camera/mx3_camera.c +1 -1
@@ -334,7 +334,7 @@ static void mx3_videobuf_queue(struct vb2_buffer *vb)
 	if (!mx3_cam->active)
 		mx3_cam->active = buf;
 
-	spin_unlock_irq(&mx3_cam->lock);
+	spin_unlock_irqrestore(&mx3_cam->lock, flags);
 
 	cookie = txd->tx_submit(txd);
 	dev_dbg(icd->parent, "Submitted cookie %d DMA 0x%08x\n",

Please, wait with this. The above doesn't seem quite right to me. IIRC, the purpose of unlock_irq(), i.e. of the unconditionally enabling IRQs was to make sure ->tx_submit() is called with interrupts enabled. I'm currently on holiday with very scarce internet access. Either please double-check this yourself or I'll have another look at it when back home next week.

Thanks
Guennadi

drivers/media/platform/soc_camera/mx3_camera.c static void mx3_videobuf_queue(struct vb2_buffer *vb) · line 349

Quoted 10 lines
drivers/media/platform/soc_camera/mx3_camera.c +1 -1
@@ -343,7 +343,7 @@ static void mx3_videobuf_queue(struct vb2_buffer *vb)
 	if (cookie >= 0)
 		return;
 
-	spin_lock_irq(&mx3_cam->lock);
+	spin_lock_irqsave(&mx3_cam->lock, flags);
 
 	/* Submit error */
 	list_del_init(&buf->queue);

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

author
#2 lore raw
Quoted 20 lines · Guennadi Liakhovetski

Hi Dan,

Quoted 16 lines · Dan Carpenter

There is a return in the middle where we haven't restored the IRQs to their original state.

Signed-off-by Dan Carpenter <[email protected]>
drivers/media/platform/soc_camera/mx3_camera.c +1 -1
index 1047e3e..4bae910 100644
@@ -334,7 +334,7 @@ static void mx3_videobuf_queue(struct vb2_buffer *vb)
 	if (!mx3_cam->active)
 		mx3_cam->active = buf;
 
-	spin_unlock_irq(&mx3_cam->lock);
+	spin_unlock_irqrestore(&mx3_cam->lock, flags);
Quoted 10 lines

drivers/media/platform/soc_camera/mx3_camera.c static void mx3_videobuf_queue(struct vb2_buffer *vb) · line 340

Quoted 2 lines
drivers/media/platform/soc_camera/mx3_camera.c +0 -0
 	cookie = txd->tx_submit(txd);
 	dev_dbg(icd->parent, "Submitted cookie %d DMA 0x%08x\n",

Please, wait with this. The above doesn't seem quite right to me. IIRC, the purpose of unlock_irq(), i.e. of the unconditionally enabling IRQs was to make sure ->tx_submit() is called with interrupts enabled. I'm currently on holiday with very scarce internet access. Either please double-check this yourself or I'll have another look at it when back home next week.

No problem. This is static checker stuff. The currect code is
definitely wrong because the return in the middle. When the function returns we don't know if IRQs are enabled or not.

Quoted 3 lines

drivers/media/platform/soc_camera/mx3_camera.c static void mx3_videobuf_queue(struct vb2_buffer *vb) · line 344

Quoted 3 lines
drivers/media/platform/soc_camera/mx3_camera.c +0 -0
@@ -343,7 +343,7 @@ static void mx3_videobuf_queue(struct vb2_buffer *vb)
 	if (cookie >= 0)
 		return;

            ^^^^^^^^^^^
Quoted 3 lines

drivers/media/platform/soc_camera/mx3_camera.c static void mx3_videobuf_queue(struct vb2_buffer *vb) · line 346

Quoted 3 lines
drivers/media/platform/soc_camera/mx3_camera.c +1 -1
-	spin_lock_irq(&mx3_cam->lock);
+	spin_lock_irqsave(&mx3_cam->lock, flags);

I thought about using local_irq_restore(flags) but it seemed like an obvious oversight.

regards,
dan carpenter

#3 lore raw
Quoted 32 lines · Dan Carpenter
Quoted 20 lines · Guennadi Liakhovetski

Hi Dan,

Quoted 16 lines · Dan Carpenter

There is a return in the middle where we haven't restored the IRQs to their original state.

Signed-off-by Dan Carpenter <[email protected]>
drivers/media/platform/soc_camera/mx3_camera.c +1 -1
index 1047e3e..4bae910 100644
@@ -334,7 +334,7 @@ static void mx3_videobuf_queue(struct vb2_buffer *vb)
 	if (!mx3_cam->active)
 		mx3_cam->active = buf;
 
-	spin_unlock_irq(&mx3_cam->lock);
+	spin_unlock_irqrestore(&mx3_cam->lock, flags);
Quoted 10 lines

drivers/media/platform/soc_camera/mx3_camera.c static void mx3_videobuf_queue(struct vb2_buffer *vb) · line 340

Quoted 2 lines
drivers/media/platform/soc_camera/mx3_camera.c +0 -0
 	cookie = txd->tx_submit(txd);
 	dev_dbg(icd->parent, "Submitted cookie %d DMA 0x%08x\n",

Please, wait with this. The above doesn't seem quite right to me. IIRC, the purpose of unlock_irq(), i.e. of the unconditionally enabling IRQs was to make sure ->tx_submit() is called with interrupts enabled. I'm currently on holiday with very scarce internet access. Either please double-check this yourself or I'll have another look at it when back home next week.

Quoted 3 lines

No problem. This is static checker stuff. The currect code is
definitely wrong because the return in the middle. When the function returns we don't know if IRQs are enabled or not.

Ok, I did suspect this, but just wanted to check, when I have a better access to sources. This function cannot be called with IRQs disabled. So, it is ok to replace ..._irqsave() / ..._irqrestore() calls in it with their plain ..._irq() counterparts. Could you cook up a patch, please?

Thanks
Guennadi

Quoted 5 lines
Quoted 3 lines

drivers/media/platform/soc_camera/mx3_camera.c static void mx3_videobuf_queue(struct vb2_buffer *vb) · line 344

Quoted 3 lines
drivers/media/platform/soc_camera/mx3_camera.c +0 -0
@@ -343,7 +343,7 @@ static void mx3_videobuf_queue(struct vb2_buffer *vb)
 	if (cookie >= 0)
 		return;

            ^^^^^^^^^^^
Quoted 1 line
Quoted 1 line
Quoted 9 lines
Quoted 2 lines

drivers/media/platform/soc_camera/mx3_camera.c static void mx3_videobuf_queue(struct vb2_buffer *vb) · line 346

Quoted 2 lines
drivers/media/platform/soc_camera/mx3_camera.c +1 -1
-	spin_lock_irq(&mx3_cam->lock);
+	spin_lock_irqsave(&mx3_cam->lock, flags);

I thought about using local_irq_restore(flags) but it seemed like an obvious oversight.

regards,
dan carpenter

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

author
#4 lore raw

PATCH v2 1/1 mx3-camera: locking cleanup in mx3_videobuf_queue()

Smatch complains about the locking here because we mix spin_lock_irq() with spin_lock_irqsave() in an unusual way. According to Smatch, it's not always clear if the IRQs are enabled or disabled when we return. It turns out this function is always called with IRQs enabled and we can just use spin_lock_irq().

It's called from __enqueue_in_driver().

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

---
v2: The first version changed everything to irq_save/restore() but that
wasn't right because we wanted IRQs enabled and not simply restored.

drivers/media/platform/soc_camera/mx3_camera.c +2 -3
index 1047e3e..18eab8e 100644
@@ -266,7 +266,6 @@ static void mx3_videobuf_queue(struct vb2_buffer *vb)
 	struct idmac_channel *ichan = mx3_cam->idmac_channel[0];
 	struct idmac_video_param *video = &ichan->params.video;
 	const struct soc_mbus_pixelfmt *host_fmt = icd->current_fmt->host_fmt;
-	unsigned long flags;
 	dma_cookie_t cookie;
 	size_t new_size;
 

@@ -328,7 +327,7 @@ static void mx3_videobuf_queue(struct vb2_buffer *vb)
 		memset(vb2_plane_vaddr(vb, 0), 0xaa, vb2_get_plane_payload(vb, 0));
 #endif
 
-	spin_lock_irqsave(&mx3_cam->lock, flags);
+	spin_lock_irq(&mx3_cam->lock);
 	list_add_tail(&buf->queue, &mx3_cam->capture);
 
 	if (!mx3_cam->active)

@@ -351,7 +350,7 @@ static void mx3_videobuf_queue(struct vb2_buffer *vb)
 	if (mx3_cam->active = buf)
 		mx3_cam->active = NULL;
 
-	spin_unlock_irqrestore(&mx3_cam->lock, flags);
+	spin_unlock_irq(&mx3_cam->lock);
 error:
 	vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
 }