Re: [PATCH 1/2] [media] tw5864-core: remove double irq lock code

[Andrey Utkin <andrey_utkin@fastmail.com>]

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