* [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.