All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] [media] tw5864-core: remove double irq lock code
@ 2016-08-24 16:30 Mauro Carvalho Chehab
  2016-08-24 16:30 ` [PATCH 2/2] [media] tw5864: remove two unused vars Mauro Carvalho Chehab
  2016-08-25 14:46 ` [PATCH 1/2] [media] tw5864-core: remove double irq lock code Andrey Utkin
  0 siblings, 2 replies; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2016-08-24 16:30 UTC (permalink / raw)
  Cc: Mauro Carvalho Chehab, Linux Media Mailing List,
	Mauro Carvalho Chehab, Bluecherry Maintainers, Andrey Utkin,
	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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] [media] tw5864: remove two unused vars
  2016-08-24 16:30 [PATCH 1/2] [media] tw5864-core: remove double irq lock code Mauro Carvalho Chehab
@ 2016-08-24 16:30 ` Mauro Carvalho Chehab
  2016-08-24 23:07   ` Andrey Utkin
  2016-08-25 14:46 ` [PATCH 1/2] [media] tw5864-core: remove double irq lock code Andrey Utkin
  1 sibling, 1 reply; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2016-08-24 16:30 UTC (permalink / raw)
  Cc: Mauro Carvalho Chehab, Linux Media Mailing List,
	Mauro Carvalho Chehab, Bluecherry Maintainers, Andrey Utkin,
	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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] [media] tw5864: remove two unused vars
  2016-08-24 16:30 ` [PATCH 2/2] [media] tw5864: remove two unused vars Mauro Carvalho Chehab
@ 2016-08-24 23:07   ` Andrey Utkin
  0 siblings, 0 replies; 6+ messages in thread
From: Andrey Utkin @ 2016-08-24 23:07 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Linux Media Mailing List, Mauro Carvalho Chehab,
	Bluecherry Maintainers, Mauro Carvalho Chehab

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] [media] tw5864-core: remove double irq lock code
  2016-08-24 16:30 [PATCH 1/2] [media] tw5864-core: remove double irq lock code Mauro Carvalho Chehab
  2016-08-24 16:30 ` [PATCH 2/2] [media] tw5864: remove two unused vars Mauro Carvalho Chehab
@ 2016-08-25 14:46 ` Andrey Utkin
  2016-08-30 22:19   ` Andrey Utkin
  2016-09-05 13:58   ` Andrey Utkin
  1 sibling, 2 replies; 6+ messages in thread
From: Andrey Utkin @ 2016-08-25 14:46 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Linux Media Mailing List, Mauro Carvalho Chehab,
	Bluecherry Maintainers, Andrey Utkin, Mauro Carvalho Chehab

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] [media] tw5864-core: remove double irq lock code
  2016-08-25 14:46 ` [PATCH 1/2] [media] tw5864-core: remove double irq lock code Andrey Utkin
@ 2016-08-30 22:19   ` Andrey Utkin
  2016-09-05 13:58   ` Andrey Utkin
  1 sibling, 0 replies; 6+ messages in thread
From: Andrey Utkin @ 2016-08-30 22:19 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Linux Media Mailing List, Mauro Carvalho Chehab,
	Bluecherry Maintainers, Andrey Utkin, Mauro Carvalho Chehab

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] [media] tw5864-core: remove double irq lock code
  2016-08-25 14:46 ` [PATCH 1/2] [media] tw5864-core: remove double irq lock code Andrey Utkin
  2016-08-30 22:19   ` Andrey Utkin
@ 2016-09-05 13:58   ` Andrey Utkin
  1 sibling, 0 replies; 6+ messages in thread
From: Andrey Utkin @ 2016-09-05 13:58 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, Hans Verkuil
  Cc: Linux Media Mailing List, Mauro Carvalho Chehab,
	Bluecherry Maintainers, Andrey Utkin, Mauro Carvalho Chehab

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-09-05 13:58 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-24 16:30 [PATCH 1/2] [media] tw5864-core: remove double irq lock code Mauro Carvalho Chehab
2016-08-24 16:30 ` [PATCH 2/2] [media] tw5864: remove two unused vars Mauro Carvalho Chehab
2016-08-24 23:07   ` Andrey Utkin
2016-08-25 14:46 ` [PATCH 1/2] [media] tw5864-core: remove double irq lock code Andrey Utkin
2016-08-30 22:19   ` Andrey Utkin
2016-09-05 13:58   ` Andrey Utkin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.