[patch] [media] mx3-camera: locking typo in mx3_videobuf_queue()

[patch] [media] mx3-camera: locking typo in mx3_videobuf_queue()

[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 <dan.carpenter@oracle.com>

diff --git a/drivers/media/platform/soc_camera/mx3_camera.c b/drivers/media/platform/soc_camera/mx3_camera.c
index 1047e3e..4bae910 100644
--- a/drivers/media/platform/soc_camera/mx3_camera.c
+++ b/drivers/media/platform/soc_camera/mx3_camera.c
@@ -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);

Re: [patch] [media] mx3-camera: locking typo in mx3_videobuf_queue()

[Guennadi Liakhovetski]

Hi Dan,

On Fri, 23 Aug 2013, Dan Carpenter wrote:

> There is a return in the middle where we haven't restored the IRQs to
> their original state.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/media/platform/soc_camera/mx3_camera.c b/drivers/media/platform/soc_camera/mx3_camera.c
> index 1047e3e..4bae910 100644
> --- a/drivers/media/platform/soc_camera/mx3_camera.c
> +++ b/drivers/media/platform/soc_camera/mx3_camera.c
> @@ -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

> @@ -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/

Re: [patch] [media] mx3-camera: locking typo in mx3_videobuf_queue()

[Dan Carpenter]

On Fri, Aug 23, 2013 at 11:16:51PM +0200, Guennadi Liakhovetski wrote:
> Hi Dan,
> 
> On Fri, 23 Aug 2013, Dan Carpenter wrote:
> 
> > There is a return in the middle where we haven't restored the IRQs to
> > their original state.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/drivers/media/platform/soc_camera/mx3_camera.c b/drivers/media/platform/soc_camera/mx3_camera.c
> > index 1047e3e..4bae910 100644
> > --- a/drivers/media/platform/soc_camera/mx3_camera.c
> > +++ b/drivers/media/platform/soc_camera/mx3_camera.c
> > @@ -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.
> 

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.

> > @@ -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);

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

regards,
dan carpenter

Re: [patch] [media] mx3-camera: locking typo in mx3_videobuf_queue()

[Guennadi Liakhovetski]

On Sat, 24 Aug 2013, Dan Carpenter wrote:

> On Fri, Aug 23, 2013 at 11:16:51PM +0200, Guennadi Liakhovetski wrote:
> > Hi Dan,
> > 
> > On Fri, 23 Aug 2013, Dan Carpenter wrote:
> > 
> > > There is a return in the middle where we haven't restored the IRQs to
> > > their original state.
> > > 
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > 
> > > diff --git a/drivers/media/platform/soc_camera/mx3_camera.c b/drivers/media/platform/soc_camera/mx3_camera.c
> > > index 1047e3e..4bae910 100644
> > > --- a/drivers/media/platform/soc_camera/mx3_camera.c
> > > +++ b/drivers/media/platform/soc_camera/mx3_camera.c
> > > @@ -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.
> > 
> 
> 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

> > > @@ -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);
> 
> 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/

[patch v2] [media] mx3-camera: locking cleanup in mx3_videobuf_queue()

[Dan Carpenter]

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 <dan.carpenter@oracle.com>
---
v2: The first version changed everything to irq_save/restore() but that
    wasn't right because we wanted IRQs enabled and not simply restored.

diff --git a/drivers/media/platform/soc_camera/mx3_camera.c b/drivers/media/platform/soc_camera/mx3_camera.c
index 1047e3e..18eab8e 100644
--- a/drivers/media/platform/soc_camera/mx3_camera.c
+++ b/drivers/media/platform/soc_camera/mx3_camera.c
@@ -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);
 }