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

[Andrey Utkin <andrey_utkin@fastmail.com>]

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",