| Message ID | 1472136366.1628907.705944009.76C1558A@webmail.messagingengine.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Mauro, Have you received my letter which is cited below? I also tried to get in touch with you on #linuxtv. On Thu, Aug 25, 2016 at 05:46:06PM +0300, Andrey Utkin wrote: > For some reason (maybe "unlisted recipients"?), my reply didn't get > through to maillists. Resending my reply. > > On Wed, Aug 24, 2016, at 19:30, Mauro Carvalho Chehab wrote: > > As warned by smatch: > > drivers/media/pci/tw5864/tw5864-core.c:160 tw5864_h264_isr() error: double lock 'irqsave:flags' > > drivers/media/pci/tw5864/tw5864-core.c:174 tw5864_h264_isr() error: double unlock 'irqsave:flags' > > > > Remove the IRQ duplicated lock. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > > --- > > drivers/media/pci/tw5864/tw5864-core.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/media/pci/tw5864/tw5864-core.c > > b/drivers/media/pci/tw5864/tw5864-core.c > > index 440cd7bb8d04..e3d884e963c0 100644 > > --- a/drivers/media/pci/tw5864/tw5864-core.c > > +++ b/drivers/media/pci/tw5864/tw5864-core.c > > @@ -157,12 +157,10 @@ static void tw5864_h264_isr(struct tw5864_dev *dev) > > > > cur_frame = next_frame; > > > > - spin_lock_irqsave(&input->slock, flags); > > input->frame_seqno++; > > input->frame_gop_seqno++; > > if (input->frame_gop_seqno >= input->gop) > > input->frame_gop_seqno = 0; > > - spin_unlock_irqrestore(&input->slock, flags); > > } else { > > dev_err(&dev->pci->dev, > > "Skipped frame on input %d because all buffers busy\n", > > > Thank you very much for catching this issue, but NACK on the patch. > > These two lock operations are on different spinlocks. One for device, > another > one for input (a subordinate entity of device). What is superfluous here > is > second _irqsave. Also "flags" variable reuse is wrong. So what would be > right, > in my opinion, is the following (going to submit this patch): > > diff --git a/drivers/media/pci/tw5864/tw5864-core.c > b/drivers/media/pci/tw5864/tw5864-core.c > index 440cd7b..1d43b96 100644 > --- a/drivers/media/pci/tw5864/tw5864-core.c > +++ b/drivers/media/pci/tw5864/tw5864-core.c > @@ -157,12 +157,12 @@ static void tw5864_h264_isr(struct tw5864_dev > *dev) > > cur_frame = next_frame; > > - spin_lock_irqsave(&input->slock, flags); > + spin_lock(&input->slock); > input->frame_seqno++; > input->frame_gop_seqno++; > if (input->frame_gop_seqno >= input->gop) > input->frame_gop_seqno = 0; > - spin_unlock_irqrestore(&input->slock, flags); > + spin_unlock(&input->slock); > } else { > dev_err(&dev->pci->dev, > "Skipped frame on input %d because all buffers > busy\n", > -- > 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 -- 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
Just received a notification from patchwork that my alternative patch on this was rejected as not applicable. Thanks to Hans Verkuil - he has applied a reversion of Mauro's wrong patch and also applied my patch in his repo, git://linuxtv.org/hverkuil/media_tree.git (branch for-v4.9c). As I see the wrong patch is in git://linuxtv.org/media_tree.git master branch, and is considered "merged", but I would love to see that wrong patch (and reversion of it) dropped before it gets into linux-next (I don't know whether media tree has a tradition to not reset branches). -- 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/pci/tw5864/tw5864-core.c b/drivers/media/pci/tw5864/tw5864-core.c index 440cd7b..1d43b96 100644 --- a/drivers/media/pci/tw5864/tw5864-core.c +++ b/drivers/media/pci/tw5864/tw5864-core.c @@ -157,12 +157,12 @@ static void tw5864_h264_isr(struct tw5864_dev *dev) cur_frame = next_frame; - spin_lock_irqsave(&input->slock, flags); + spin_lock(&input->slock); input->frame_seqno++; input->frame_gop_seqno++; if (input->frame_gop_seqno >= input->gop) input->frame_gop_seqno = 0; - spin_unlock_irqrestore(&input->slock, flags); + spin_unlock(&input->slock); } else { dev_err(&dev->pci->dev,