| Message ID | 20130823094530.GN31293@elgon.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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> -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html