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

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

[Mauro Carvalho Chehab]

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

[PATCH 2/2] [media] tw5864: remove two unused vars

[Mauro Carvalho Chehab]

Remove those two vars that aren't used, as reported by smatch:

drivers/media/pci/tw5864/tw5864-video.c: In function 'tw5864_prepare_frame_headers':
drivers/media/pci/tw5864/tw5864-video.c:1219:16: warning: variable 'space_before_sl_hdr' set but not used [-Wunused-but-set-variable]
  unsigned long space_before_sl_hdr;
                ^~~~~~~~~~~~~~~~~~~
drivers/media/pci/tw5864/tw5864-video.c:1218:6: warning: variable 'sl_hdr' set but not used [-Wunused-but-set-variable]
  u8 *sl_hdr;
      ^~~~~~

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/pci/tw5864/tw5864-video.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/media/pci/tw5864/tw5864-video.c b/drivers/media/pci/tw5864/tw5864-video.c
index 3461ba9162e7..6c1685aeaea9 100644
--- a/drivers/media/pci/tw5864/tw5864-video.c
+++ b/drivers/media/pci/tw5864/tw5864-video.c
@@ -1215,8 +1215,6 @@ void tw5864_prepare_frame_headers(struct tw5864_input *input)
 	u8 *dst;
 	size_t dst_space;
 	unsigned long flags;
-	u8 *sl_hdr;
-	unsigned long space_before_sl_hdr;
 
 	if (!vb) {
 		spin_lock_irqsave(&input->slock, flags);
@@ -1253,8 +1251,6 @@ void tw5864_prepare_frame_headers(struct tw5864_input *input)
 					      input->width, input->height);
 
 	/* Put slice header */
-	sl_hdr = dst;
-	space_before_sl_hdr = dst_space;
 	tw5864_h264_put_slice_header(&dst, &dst_space, input->h264_idr_pic_id,
 				     input->frame_gop_seqno,
 				     &input->tail_nb_bits, &input->tail);
-- 
2.7.4

Re: [PATCH 2/2] [media] tw5864: remove two unused vars

[Andrey Utkin]

On Wed, Aug 24, 2016 at 01:30:40PM -0300, Mauro Carvalho Chehab wrote:
> Remove those two vars that aren't used, as reported by smatch:

Acked-by: Andrey Utkin <andrey.utkin@corp.bluecherry.net>

Sorry for missing this.
Thanks a lot.

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

[Andrey Utkin]

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

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

[Andrey Utkin]

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

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

[Andrey Utkin]

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).