linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] [media] dib7000p: Remove dead code
@ 2018-09-15  5:47 Nathan Chancellor
  2018-09-17 17:58 ` Nick Desaulniers
  0 siblings, 1 reply; 10+ messages in thread
From: Nathan Chancellor @ 2018-09-15  5:47 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: linux-media, linux-kernel, Nick Desaulniers, Nathan Chancellor

Clang warns that 'interleaving' is assigned to itself in this function.

drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
assigning value of variable of type 'int' to itself [-Wself-assign]
        interleaving = interleaving;
        ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
1 warning generated.

It's correct. Just removing the self-assignment would sufficiently hide
the warning but all of this code is dead because 'tmp' is zero due to
being multiplied by zero. This doesn't appear to be an issue with
dib8000, which this code was copied from in commit 041ad449683b
("[media] dib7000p: Add DVBv5 stats support").

Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 drivers/media/dvb-frontends/dib7000p.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
index 58387860b62d..25843658fc68 100644
--- a/drivers/media/dvb-frontends/dib7000p.c
+++ b/drivers/media/dvb-frontends/dib7000p.c
@@ -1800,9 +1800,8 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
 {
 	struct dtv_frontend_properties *c = &demod->dtv_property_cache;
 	u64 time_us, tmp64;
-	u32 tmp, denom;
-	int guard, rate_num, rate_denum = 1, bits_per_symbol;
-	int interleaving = 0, fft_div;
+	u32 denom;
+	int guard, rate_num, rate_denum = 1, bits_per_symbol, fft_div;
 
 	switch (c->guard_interval) {
 	case GUARD_INTERVAL_1_4:
@@ -1871,8 +1870,6 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
 		break;
 	}
 
-	interleaving = interleaving;
-
 	denom = bits_per_symbol * rate_num * fft_div * 384;
 
 	/* If calculus gets wrong, wait for 1s for the next stats */
@@ -1887,9 +1884,6 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
 	time_us += denom / 2;
 	do_div(time_us, denom);
 
-	tmp = 1008 * 96 * interleaving;
-	time_us += tmp + tmp / guard;
-
 	return time_us;
 }
 
-- 
2.18.0


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

* Re: [PATCH] [media] dib7000p: Remove dead code
  2018-09-15  5:47 [PATCH] [media] dib7000p: Remove dead code Nathan Chancellor
@ 2018-09-17 17:58 ` Nick Desaulniers
  2018-09-17 22:39   ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 10+ messages in thread
From: Nick Desaulniers @ 2018-09-17 17:58 UTC (permalink / raw)
  To: Nathan Chancellor; +Cc: Mauro Carvalho Chehab, linux-media, LKML

On Fri, Sep 14, 2018 at 10:47 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang warns that 'interleaving' is assigned to itself in this function.
>
> drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
> assigning value of variable of type 'int' to itself [-Wself-assign]
>         interleaving = interleaving;
>         ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
> 1 warning generated.
>
> It's correct. Just removing the self-assignment would sufficiently hide
> the warning but all of this code is dead because 'tmp' is zero due to
> being multiplied by zero. This doesn't appear to be an issue with
> dib8000, which this code was copied from in commit 041ad449683b
> ("[media] dib7000p: Add DVBv5 stats support").
>
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  drivers/media/dvb-frontends/dib7000p.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
> index 58387860b62d..25843658fc68 100644
> --- a/drivers/media/dvb-frontends/dib7000p.c
> +++ b/drivers/media/dvb-frontends/dib7000p.c
> @@ -1800,9 +1800,8 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)

Something looks wrong here (with this function).  The patch is no
functional change, since as you point out `interleaving` is
initialized to 0, then never updated before read, but I think there's
an underlying bug here that should be fixed differently.  Thanks for
the patch though, as it does raise the question.

dib7000p_get_time_us has this comment above it:

  1798 /* FIXME: may require changes - this one was borrowed from
dib8000 */

Wondering if it has the same bug, it seems it does not:
drivers/media/dvb-frontends/dib8000.c#dib8000_get_time_us():3986

dib8000_get_time_us() seems to loop over multiple layers, and then
assigns interleaving to the final layers interleaving (that looks like
loop invariant code to me).

Mauro, should dib7000p_get_time_us() use c->layer[???].interleaving?
I don't see a single reference to `layer` in
drivers/media/dvb-frontends/dib7000p.c.

>  {
>         struct dtv_frontend_properties *c = &demod->dtv_property_cache;
>         u64 time_us, tmp64;
> -       u32 tmp, denom;
> -       int guard, rate_num, rate_denum = 1, bits_per_symbol;
> -       int interleaving = 0, fft_div;
> +       u32 denom;
> +       int guard, rate_num, rate_denum = 1, bits_per_symbol, fft_div;
>
>         switch (c->guard_interval) {
>         case GUARD_INTERVAL_1_4:
> @@ -1871,8 +1870,6 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
>                 break;
>         }
>
> -       interleaving = interleaving;
> -
>         denom = bits_per_symbol * rate_num * fft_div * 384;
>
>         /* If calculus gets wrong, wait for 1s for the next stats */
> @@ -1887,9 +1884,6 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
>         time_us += denom / 2;
>         do_div(time_us, denom);
>
> -       tmp = 1008 * 96 * interleaving;
> -       time_us += tmp + tmp / guard;
> -
>         return time_us;
>  }
>
> --
> 2.18.0
>


-- 
Thanks,
~Nick Desaulniers

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

* Re: [PATCH] [media] dib7000p: Remove dead code
  2018-09-17 17:58 ` Nick Desaulniers
@ 2018-09-17 22:39   ` Mauro Carvalho Chehab
  2018-09-19 18:55     ` Nick Desaulniers
                       ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Mauro Carvalho Chehab @ 2018-09-17 22:39 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Nathan Chancellor, Mauro Carvalho Chehab, linux-media, LKML

Em Mon, 17 Sep 2018 10:58:32 -0700
Nick Desaulniers <ndesaulniers@google.com> escreveu:

> On Fri, Sep 14, 2018 at 10:47 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > Clang warns that 'interleaving' is assigned to itself in this function.
> >
> > drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
> > assigning value of variable of type 'int' to itself [-Wself-assign]
> >         interleaving = interleaving;
> >         ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
> > 1 warning generated.
> >
> > It's correct. Just removing the self-assignment would sufficiently hide
> > the warning but all of this code is dead because 'tmp' is zero due to
> > being multiplied by zero. This doesn't appear to be an issue with
> > dib8000, which this code was copied from in commit 041ad449683b
> > ("[media] dib7000p: Add DVBv5 stats support").
> >
> > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  drivers/media/dvb-frontends/dib7000p.c | 10 ++--------
> >  1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
> > index 58387860b62d..25843658fc68 100644
> > --- a/drivers/media/dvb-frontends/dib7000p.c
> > +++ b/drivers/media/dvb-frontends/dib7000p.c
> > @@ -1800,9 +1800,8 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)  
> 
> Something looks wrong here (with this function).  The patch is no
> functional change, since as you point out `interleaving` is
> initialized to 0, then never updated before read, but I think there's
> an underlying bug here that should be fixed differently.  Thanks for
> the patch though, as it does raise the question.
> 
> dib7000p_get_time_us has this comment above it:
> 
>   1798 /* FIXME: may require changes - this one was borrowed from
> dib8000 */

The goal of dib7000p_get_time_us() is to estimate how much time it
takes, with current tuning parameters, to have a certain number of
DVB-T packets. This is used for block error count. That's said,
on a quick look, it seems that the code is not right on many ways.

It should be aligned with the amount of data it is required for
dib7000 to update the block/bit error counters. There are two kinds
of implementation:

1) the frontend has an internal counter that it is shifted and made
   available to the driver after a certain amount of received data
   (usually in the order of 10^5 to 10^7 bits);

2) the frontend has an internal timer that shifts the data from its
   internal counter after a certain amount of time (usually at the
   seconds range).

Different vendors opt for either one of the strategy. Some updates
a counter with the amount of bits taken. Unfortunately, this is not
the case of those dib* frontends. So, the Kernel has to estimate
it, based on the tuning parameters.

From the code, it seems that, for block errors, it waits for 1,250,000
bits to arrive (e. g. about 766 packets), so, it uses type (1) strategy:

                /* Estimate the number of packets based on bitrate */
                if (!time_us)
                        time_us = dib7000p_get_time_us(demod);

                if (time_us) {
                        blocks = 1250000ULL * 1000000ULL;	// the multiply here is to convert to microsseconds...
                        do_div(blocks, time_us * 8 * 204);	// As here it divides by the time in microsseconds
                        c->block_count.stat[0].scale = FE_SCALE_COUNTER;
                        c->block_count.stat[0].uvalue += blocks;
                }

For BER, the logic assumes that the bit error count should be divided
by 10^-8:

                c->post_bit_count.stat[0].uvalue += 100000000;

and the counter is updated every second. So, it uses (2).

> 
> Wondering if it has the same bug, it seems it does not:
> drivers/media/dvb-frontends/dib8000.c#dib8000_get_time_us():3986
> 
> dib8000_get_time_us() seems to loop over multiple layers, and then
> assigns interleaving to the final layers interleaving (that looks like
> loop invariant code to me).
> 
> Mauro, should dib7000p_get_time_us() use c->layer[???].interleaving?

I don't think that time interleaving would affect the bit rate.
I suspect that the dead code on dib8000 is just a dead code.

> I don't see a single reference to `layer` in
> drivers/media/dvb-frontends/dib7000p.c.

Layers are specific for ISDB-T, but I think DVB-T (or at least DVB-T2)
may use time interleaving. 

Yet, as I said, the goal is to estimate the streaming bit rate. 

I don't remember anymore from where the dib8000 formula came.

My guts tell that time interleaving shouldn't do much changes (if any)
to the bit rate. I suspect that removing the dead code is likely
OK, but I'll try to see if I can find something related to where this
formula came.

> 
> >  {
> >         struct dtv_frontend_properties *c = &demod->dtv_property_cache;
> >         u64 time_us, tmp64;
> > -       u32 tmp, denom;
> > -       int guard, rate_num, rate_denum = 1, bits_per_symbol;
> > -       int interleaving = 0, fft_div;
> > +       u32 denom;
> > +       int guard, rate_num, rate_denum = 1, bits_per_symbol, fft_div;
> >
> >         switch (c->guard_interval) {
> >         case GUARD_INTERVAL_1_4:
> > @@ -1871,8 +1870,6 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
> >                 break;
> >         }
> >
> > -       interleaving = interleaving;
> > -
> >         denom = bits_per_symbol * rate_num * fft_div * 384;
> >
> >         /* If calculus gets wrong, wait for 1s for the next stats */
> > @@ -1887,9 +1884,6 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
> >         time_us += denom / 2;
> >         do_div(time_us, denom);
> >
> > -       tmp = 1008 * 96 * interleaving;
> > -       time_us += tmp + tmp / guard;
> > -
> >         return time_us;
> >  }
> >
> > --
> > 2.18.0
> >  
> 
> 



Thanks,
Mauro

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

* Re: [PATCH] [media] dib7000p: Remove dead code
  2018-09-17 22:39   ` Mauro Carvalho Chehab
@ 2018-09-19 18:55     ` Nick Desaulniers
  2018-10-25 18:09     ` Nathan Chancellor
  2018-12-04 10:26     ` Sean Young
  2 siblings, 0 replies; 10+ messages in thread
From: Nick Desaulniers @ 2018-09-19 18:55 UTC (permalink / raw)
  To: mchehab+samsung
  Cc: Nathan Chancellor, Mauro Carvalho Chehab, linux-media, LKML

On Mon, Sep 17, 2018 at 3:39 PM Mauro Carvalho Chehab
<mchehab+samsung@kernel.org> wrote:
>
> Em Mon, 17 Sep 2018 10:58:32 -0700
> Nick Desaulniers <ndesaulniers@google.com> escreveu:
>
> > On Fri, Sep 14, 2018 at 10:47 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > >
> > > Clang warns that 'interleaving' is assigned to itself in this function.
> > >
> > > drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
> > > assigning value of variable of type 'int' to itself [-Wself-assign]
> > >         interleaving = interleaving;
> > >         ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
> > > 1 warning generated.
> > >
> > > It's correct. Just removing the self-assignment would sufficiently hide
> > > the warning but all of this code is dead because 'tmp' is zero due to
> > > being multiplied by zero. This doesn't appear to be an issue with
> > > dib8000, which this code was copied from in commit 041ad449683b
> > > ("[media] dib7000p: Add DVBv5 stats support").
> > >
> > > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > ---
> > >  drivers/media/dvb-frontends/dib7000p.c | 10 ++--------
> > >  1 file changed, 2 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
> > > index 58387860b62d..25843658fc68 100644
> > > --- a/drivers/media/dvb-frontends/dib7000p.c
> > > +++ b/drivers/media/dvb-frontends/dib7000p.c
> > > @@ -1800,9 +1800,8 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
> >
> > Something looks wrong here (with this function).  The patch is no
> > functional change, since as you point out `interleaving` is
> > initialized to 0, then never updated before read, but I think there's
> > an underlying bug here that should be fixed differently.  Thanks for
> > the patch though, as it does raise the question.
> >
> > dib7000p_get_time_us has this comment above it:
> >
> >   1798 /* FIXME: may require changes - this one was borrowed from
> > dib8000 */
>
> The goal of dib7000p_get_time_us() is to estimate how much time it
> takes, with current tuning parameters, to have a certain number of
> DVB-T packets. This is used for block error count. That's said,
> on a quick look, it seems that the code is not right on many ways.
>
> It should be aligned with the amount of data it is required for
> dib7000 to update the block/bit error counters. There are two kinds
> of implementation:
>
> 1) the frontend has an internal counter that it is shifted and made
>    available to the driver after a certain amount of received data
>    (usually in the order of 10^5 to 10^7 bits);
>
> 2) the frontend has an internal timer that shifts the data from its
>    internal counter after a certain amount of time (usually at the
>    seconds range).
>
> Different vendors opt for either one of the strategy. Some updates
> a counter with the amount of bits taken. Unfortunately, this is not
> the case of those dib* frontends. So, the Kernel has to estimate
> it, based on the tuning parameters.
>
> From the code, it seems that, for block errors, it waits for 1,250,000
> bits to arrive (e. g. about 766 packets), so, it uses type (1) strategy:
>
>                 /* Estimate the number of packets based on bitrate */
>                 if (!time_us)
>                         time_us = dib7000p_get_time_us(demod);
>
>                 if (time_us) {
>                         blocks = 1250000ULL * 1000000ULL;       // the multiply here is to convert to microsseconds...
>                         do_div(blocks, time_us * 8 * 204);      // As here it divides by the time in microsseconds
>                         c->block_count.stat[0].scale = FE_SCALE_COUNTER;
>                         c->block_count.stat[0].uvalue += blocks;
>                 }
>
> For BER, the logic assumes that the bit error count should be divided
> by 10^-8:
>
>                 c->post_bit_count.stat[0].uvalue += 100000000;
>
> and the counter is updated every second. So, it uses (2).
>
> >
> > Wondering if it has the same bug, it seems it does not:
> > drivers/media/dvb-frontends/dib8000.c#dib8000_get_time_us():3986
> >
> > dib8000_get_time_us() seems to loop over multiple layers, and then
> > assigns interleaving to the final layers interleaving (that looks like
> > loop invariant code to me).
> >
> > Mauro, should dib7000p_get_time_us() use c->layer[???].interleaving?
>
> I don't think that time interleaving would affect the bit rate.
> I suspect that the dead code on dib8000 is just a dead code.
>
> > I don't see a single reference to `layer` in
> > drivers/media/dvb-frontends/dib7000p.c.
>
> Layers are specific for ISDB-T, but I think DVB-T (or at least DVB-T2)
> may use time interleaving.
>
> Yet, as I said, the goal is to estimate the streaming bit rate.
>
> I don't remember anymore from where the dib8000 formula came.
>
> My guts tell that time interleaving shouldn't do much changes (if any)
> to the bit rate. I suspect that removing the dead code is likely
> OK, but I'll try to see if I can find something related to where this
> formula came.

Thanks for the detailed feedback.  If you find no other references,
then I assume this version of the patch is good to go.

>
> >
> > >  {
> > >         struct dtv_frontend_properties *c = &demod->dtv_property_cache;
> > >         u64 time_us, tmp64;
> > > -       u32 tmp, denom;
> > > -       int guard, rate_num, rate_denum = 1, bits_per_symbol;
> > > -       int interleaving = 0, fft_div;
> > > +       u32 denom;
> > > +       int guard, rate_num, rate_denum = 1, bits_per_symbol, fft_div;
> > >
> > >         switch (c->guard_interval) {
> > >         case GUARD_INTERVAL_1_4:
> > > @@ -1871,8 +1870,6 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
> > >                 break;
> > >         }
> > >
> > > -       interleaving = interleaving;
> > > -
> > >         denom = bits_per_symbol * rate_num * fft_div * 384;
> > >
> > >         /* If calculus gets wrong, wait for 1s for the next stats */
> > > @@ -1887,9 +1884,6 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
> > >         time_us += denom / 2;
> > >         do_div(time_us, denom);
> > >
> > > -       tmp = 1008 * 96 * interleaving;
> > > -       time_us += tmp + tmp / guard;
> > > -
> > >         return time_us;
> > >  }
> > >
> > > --
> > > 2.18.0
> > >
> >
> >
>
>
>
> Thanks,
> Mauro



-- 
Thanks,
~Nick Desaulniers

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

* Re: [PATCH] [media] dib7000p: Remove dead code
  2018-09-17 22:39   ` Mauro Carvalho Chehab
  2018-09-19 18:55     ` Nick Desaulniers
@ 2018-10-25 18:09     ` Nathan Chancellor
  2018-12-04 10:26     ` Sean Young
  2 siblings, 0 replies; 10+ messages in thread
From: Nathan Chancellor @ 2018-10-25 18:09 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Nick Desaulniers, Mauro Carvalho Chehab, linux-media, LKML

On Mon, Sep 17, 2018 at 07:39:36PM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 17 Sep 2018 10:58:32 -0700
> Nick Desaulniers <ndesaulniers@google.com> escreveu:
> 
> > On Fri, Sep 14, 2018 at 10:47 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > >
> > > Clang warns that 'interleaving' is assigned to itself in this function.
> > >
> > > drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
> > > assigning value of variable of type 'int' to itself [-Wself-assign]
> > >         interleaving = interleaving;
> > >         ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
> > > 1 warning generated.
> > >
> > > It's correct. Just removing the self-assignment would sufficiently hide
> > > the warning but all of this code is dead because 'tmp' is zero due to
> > > being multiplied by zero. This doesn't appear to be an issue with
> > > dib8000, which this code was copied from in commit 041ad449683b
> > > ("[media] dib7000p: Add DVBv5 stats support").
> > >
> > > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > ---
> > >  drivers/media/dvb-frontends/dib7000p.c | 10 ++--------
> > >  1 file changed, 2 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
> > > index 58387860b62d..25843658fc68 100644
> > > --- a/drivers/media/dvb-frontends/dib7000p.c
> > > +++ b/drivers/media/dvb-frontends/dib7000p.c
> > > @@ -1800,9 +1800,8 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)  
> > 
> > Something looks wrong here (with this function).  The patch is no
> > functional change, since as you point out `interleaving` is
> > initialized to 0, then never updated before read, but I think there's
> > an underlying bug here that should be fixed differently.  Thanks for
> > the patch though, as it does raise the question.
> > 
> > dib7000p_get_time_us has this comment above it:
> > 
> >   1798 /* FIXME: may require changes - this one was borrowed from
> > dib8000 */
> 
> The goal of dib7000p_get_time_us() is to estimate how much time it
> takes, with current tuning parameters, to have a certain number of
> DVB-T packets. This is used for block error count. That's said,
> on a quick look, it seems that the code is not right on many ways.
> 
> It should be aligned with the amount of data it is required for
> dib7000 to update the block/bit error counters. There are two kinds
> of implementation:
> 
> 1) the frontend has an internal counter that it is shifted and made
>    available to the driver after a certain amount of received data
>    (usually in the order of 10^5 to 10^7 bits);
> 
> 2) the frontend has an internal timer that shifts the data from its
>    internal counter after a certain amount of time (usually at the
>    seconds range).
> 
> Different vendors opt for either one of the strategy. Some updates
> a counter with the amount of bits taken. Unfortunately, this is not
> the case of those dib* frontends. So, the Kernel has to estimate
> it, based on the tuning parameters.
> 
> From the code, it seems that, for block errors, it waits for 1,250,000
> bits to arrive (e. g. about 766 packets), so, it uses type (1) strategy:
> 
>                 /* Estimate the number of packets based on bitrate */
>                 if (!time_us)
>                         time_us = dib7000p_get_time_us(demod);
> 
>                 if (time_us) {
>                         blocks = 1250000ULL * 1000000ULL;	// the multiply here is to convert to microsseconds...
>                         do_div(blocks, time_us * 8 * 204);	// As here it divides by the time in microsseconds
>                         c->block_count.stat[0].scale = FE_SCALE_COUNTER;
>                         c->block_count.stat[0].uvalue += blocks;
>                 }
> 
> For BER, the logic assumes that the bit error count should be divided
> by 10^-8:
> 
>                 c->post_bit_count.stat[0].uvalue += 100000000;
> 
> and the counter is updated every second. So, it uses (2).
> 
> > 
> > Wondering if it has the same bug, it seems it does not:
> > drivers/media/dvb-frontends/dib8000.c#dib8000_get_time_us():3986
> > 
> > dib8000_get_time_us() seems to loop over multiple layers, and then
> > assigns interleaving to the final layers interleaving (that looks like
> > loop invariant code to me).
> > 
> > Mauro, should dib7000p_get_time_us() use c->layer[???].interleaving?
> 
> I don't think that time interleaving would affect the bit rate.
> I suspect that the dead code on dib8000 is just a dead code.
> 
> > I don't see a single reference to `layer` in
> > drivers/media/dvb-frontends/dib7000p.c.
> 
> Layers are specific for ISDB-T, but I think DVB-T (or at least DVB-T2)
> may use time interleaving. 
> 
> Yet, as I said, the goal is to estimate the streaming bit rate. 
> 
> I don't remember anymore from where the dib8000 formula came.
> 
> My guts tell that time interleaving shouldn't do much changes (if any)
> to the bit rate. I suspect that removing the dead code is likely
> OK, but I'll try to see if I can find something related to where this
> formula came.
> 

Hi Mauro,

Were you able to find anything related to that formula? If not, is this
patch acceptable?

Thanks!
Nathan

> > 
> > >  {
> > >         struct dtv_frontend_properties *c = &demod->dtv_property_cache;
> > >         u64 time_us, tmp64;
> > > -       u32 tmp, denom;
> > > -       int guard, rate_num, rate_denum = 1, bits_per_symbol;
> > > -       int interleaving = 0, fft_div;
> > > +       u32 denom;
> > > +       int guard, rate_num, rate_denum = 1, bits_per_symbol, fft_div;
> > >
> > >         switch (c->guard_interval) {
> > >         case GUARD_INTERVAL_1_4:
> > > @@ -1871,8 +1870,6 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
> > >                 break;
> > >         }
> > >
> > > -       interleaving = interleaving;
> > > -
> > >         denom = bits_per_symbol * rate_num * fft_div * 384;
> > >
> > >         /* If calculus gets wrong, wait for 1s for the next stats */
> > > @@ -1887,9 +1884,6 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
> > >         time_us += denom / 2;
> > >         do_div(time_us, denom);
> > >
> > > -       tmp = 1008 * 96 * interleaving;
> > > -       time_us += tmp + tmp / guard;
> > > -
> > >         return time_us;
> > >  }
> > >
> > > --
> > > 2.18.0
> > >  
> > 
> > 
> 
> 
> 
> Thanks,
> Mauro

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

* Re: [PATCH] [media] dib7000p: Remove dead code
  2018-09-17 22:39   ` Mauro Carvalho Chehab
  2018-09-19 18:55     ` Nick Desaulniers
  2018-10-25 18:09     ` Nathan Chancellor
@ 2018-12-04 10:26     ` Sean Young
  2018-12-04 11:57       ` Mauro Carvalho Chehab
  2 siblings, 1 reply; 10+ messages in thread
From: Sean Young @ 2018-12-04 10:26 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Nick Desaulniers, Nathan Chancellor, Mauro Carvalho Chehab,
	linux-media, LKML

On Mon, Sep 17, 2018 at 07:39:36PM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 17 Sep 2018 10:58:32 -0700
> Nick Desaulniers <ndesaulniers@google.com> escreveu:
> 
> > On Fri, Sep 14, 2018 at 10:47 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > >
> > > Clang warns that 'interleaving' is assigned to itself in this function.
> > >
> > > drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
> > > assigning value of variable of type 'int' to itself [-Wself-assign]
> > >         interleaving = interleaving;
> > >         ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
> > > 1 warning generated.
> > >
> > > It's correct. Just removing the self-assignment would sufficiently hide
> > > the warning but all of this code is dead because 'tmp' is zero due to
> > > being multiplied by zero. This doesn't appear to be an issue with
> > > dib8000, which this code was copied from in commit 041ad449683b
> > > ("[media] dib7000p: Add DVBv5 stats support").
> > >
> > > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > ---
> > >  drivers/media/dvb-frontends/dib7000p.c | 10 ++--------
> > >  1 file changed, 2 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
> > > index 58387860b62d..25843658fc68 100644
> > > --- a/drivers/media/dvb-frontends/dib7000p.c
> > > +++ b/drivers/media/dvb-frontends/dib7000p.c
> > > @@ -1800,9 +1800,8 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)  
> > 
> > Something looks wrong here (with this function).  The patch is no
> > functional change, since as you point out `interleaving` is
> > initialized to 0, then never updated before read, but I think there's
> > an underlying bug here that should be fixed differently.  Thanks for
> > the patch though, as it does raise the question.
> > 
> > dib7000p_get_time_us has this comment above it:
> > 
> >   1798 /* FIXME: may require changes - this one was borrowed from
> > dib8000 */
> 
> The goal of dib7000p_get_time_us() is to estimate how much time it
> takes, with current tuning parameters, to have a certain number of
> DVB-T packets. This is used for block error count. That's said,
> on a quick look, it seems that the code is not right on many ways.
> 
> It should be aligned with the amount of data it is required for
> dib7000 to update the block/bit error counters. There are two kinds
> of implementation:
> 
> 1) the frontend has an internal counter that it is shifted and made
>    available to the driver after a certain amount of received data
>    (usually in the order of 10^5 to 10^7 bits);
> 
> 2) the frontend has an internal timer that shifts the data from its
>    internal counter after a certain amount of time (usually at the
>    seconds range).
> 
> Different vendors opt for either one of the strategy. Some updates
> a counter with the amount of bits taken. Unfortunately, this is not
> the case of those dib* frontends. So, the Kernel has to estimate
> it, based on the tuning parameters.
> 
> From the code, it seems that, for block errors, it waits for 1,250,000
> bits to arrive (e. g. about 766 packets), so, it uses type (1) strategy:
> 
>                 /* Estimate the number of packets based on bitrate */
>                 if (!time_us)
>                         time_us = dib7000p_get_time_us(demod);
> 
>                 if (time_us) {
>                         blocks = 1250000ULL * 1000000ULL;	// the multiply here is to convert to microsseconds...
>                         do_div(blocks, time_us * 8 * 204);	// As here it divides by the time in microsseconds
>                         c->block_count.stat[0].scale = FE_SCALE_COUNTER;
>                         c->block_count.stat[0].uvalue += blocks;
>                 }
> 
> For BER, the logic assumes that the bit error count should be divided
> by 10^-8:
> 
>                 c->post_bit_count.stat[0].uvalue += 100000000;
> 
> and the counter is updated every second. So, it uses (2).
> 
> > 
> > Wondering if it has the same bug, it seems it does not:
> > drivers/media/dvb-frontends/dib8000.c#dib8000_get_time_us():3986
> > 
> > dib8000_get_time_us() seems to loop over multiple layers, and then
> > assigns interleaving to the final layers interleaving (that looks like
> > loop invariant code to me).
> > 
> > Mauro, should dib7000p_get_time_us() use c->layer[???].interleaving?
> 
> I don't think that time interleaving would affect the bit rate.
> I suspect that the dead code on dib8000 is just a dead code.
> 
> > I don't see a single reference to `layer` in
> > drivers/media/dvb-frontends/dib7000p.c.
> 
> Layers are specific for ISDB-T, but I think DVB-T (or at least DVB-T2)
> may use time interleaving. 
> 
> Yet, as I said, the goal is to estimate the streaming bit rate. 
> 
> I don't remember anymore from where the dib8000 formula came.
> 
> My guts tell that time interleaving shouldn't do much changes (if any)
> to the bit rate. I suspect that removing the dead code is likely
> OK, but I'll try to see if I can find something related to where this
> formula came.

So we have two issues. One is the clang issue and clearly the code needs
fixing up. The second issue is that we're not sure about the algorithm;
I've been reading up on mpeg-ts but I'm not anywhere near getting to an
answer on this.

How about we merge a patch which just fixes the clang issue and leave
the rest of the code as-is for now?

Thanks,

Sean

---
From c6e4c5f514c38511d2054c69f7b103e98c520af4 Mon Sep 17 00:00:00 2001
From: Sean Young <sean@mess.org>
Date: Tue, 4 Dec 2018 09:59:10 +0000
Subject: [PATCH v2] media: dib7000p: Remove dead code

Clang warns that 'interleaving' is assigned to itself in this function.

drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
assigning value of variable of type 'int' to itself [-Wself-assign]
        interleaving = interleaving;
        ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
1 warning generated.

Just remove the self-assign and leave existing code in place for now.

Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/dvb-frontends/dib7000p.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
index 58387860b62d..cd84320c61c9 100644
--- a/drivers/media/dvb-frontends/dib7000p.c
+++ b/drivers/media/dvb-frontends/dib7000p.c
@@ -1871,8 +1871,6 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
 		break;
 	}
 
-	interleaving = interleaving;
-
 	denom = bits_per_symbol * rate_num * fft_div * 384;
 
 	/* If calculus gets wrong, wait for 1s for the next stats */
-- 
2.19.2


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

* Re: [PATCH] [media] dib7000p: Remove dead code
  2018-12-04 10:26     ` Sean Young
@ 2018-12-04 11:57       ` Mauro Carvalho Chehab
  2018-12-04 13:39         ` Sean Young
  0 siblings, 1 reply; 10+ messages in thread
From: Mauro Carvalho Chehab @ 2018-12-04 11:57 UTC (permalink / raw)
  To: Sean Young
  Cc: Nick Desaulniers, Nathan Chancellor, Mauro Carvalho Chehab,
	linux-media, LKML

Em Tue, 4 Dec 2018 10:26:40 +0000
Sean Young <sean@mess.org> escreveu:

> On Mon, Sep 17, 2018 at 07:39:36PM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 17 Sep 2018 10:58:32 -0700
> > Nick Desaulniers <ndesaulniers@google.com> escreveu:
> >   
> > > On Fri, Sep 14, 2018 at 10:47 PM Nathan Chancellor
> > > <natechancellor@gmail.com> wrote:  
> > > >
> > > > Clang warns that 'interleaving' is assigned to itself in this function.
> > > >
> > > > drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
> > > > assigning value of variable of type 'int' to itself [-Wself-assign]
> > > >         interleaving = interleaving;
> > > >         ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
> > > > 1 warning generated.
> > > >
> > > > It's correct. Just removing the self-assignment would sufficiently hide
> > > > the warning but all of this code is dead because 'tmp' is zero due to
> > > > being multiplied by zero. This doesn't appear to be an issue with
> > > > dib8000, which this code was copied from in commit 041ad449683b
> > > > ("[media] dib7000p: Add DVBv5 stats support").
> > > >
> > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > > ---
> > > >  drivers/media/dvb-frontends/dib7000p.c | 10 ++--------
> > > >  1 file changed, 2 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
> > > > index 58387860b62d..25843658fc68 100644
> > > > --- a/drivers/media/dvb-frontends/dib7000p.c
> > > > +++ b/drivers/media/dvb-frontends/dib7000p.c
> > > > @@ -1800,9 +1800,8 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)    
> > > 
> > > Something looks wrong here (with this function).  The patch is no
> > > functional change, since as you point out `interleaving` is
> > > initialized to 0, then never updated before read, but I think there's
> > > an underlying bug here that should be fixed differently.  Thanks for
> > > the patch though, as it does raise the question.
> > > 
> > > dib7000p_get_time_us has this comment above it:
> > > 
> > >   1798 /* FIXME: may require changes - this one was borrowed from
> > > dib8000 */  
> > 
> > The goal of dib7000p_get_time_us() is to estimate how much time it
> > takes, with current tuning parameters, to have a certain number of
> > DVB-T packets. This is used for block error count. That's said,
> > on a quick look, it seems that the code is not right on many ways.
> > 
> > It should be aligned with the amount of data it is required for
> > dib7000 to update the block/bit error counters. There are two kinds
> > of implementation:
> > 
> > 1) the frontend has an internal counter that it is shifted and made
> >    available to the driver after a certain amount of received data
> >    (usually in the order of 10^5 to 10^7 bits);
> > 
> > 2) the frontend has an internal timer that shifts the data from its
> >    internal counter after a certain amount of time (usually at the
> >    seconds range).
> > 
> > Different vendors opt for either one of the strategy. Some updates
> > a counter with the amount of bits taken. Unfortunately, this is not
> > the case of those dib* frontends. So, the Kernel has to estimate
> > it, based on the tuning parameters.
> > 
> > From the code, it seems that, for block errors, it waits for 1,250,000
> > bits to arrive (e. g. about 766 packets), so, it uses type (1) strategy:
> > 
> >                 /* Estimate the number of packets based on bitrate */
> >                 if (!time_us)
> >                         time_us = dib7000p_get_time_us(demod);
> > 
> >                 if (time_us) {
> >                         blocks = 1250000ULL * 1000000ULL;	// the multiply here is to convert to microsseconds...
> >                         do_div(blocks, time_us * 8 * 204);	// As here it divides by the time in microsseconds
> >                         c->block_count.stat[0].scale = FE_SCALE_COUNTER;
> >                         c->block_count.stat[0].uvalue += blocks;
> >                 }
> > 
> > For BER, the logic assumes that the bit error count should be divided
> > by 10^-8:
> > 
> >                 c->post_bit_count.stat[0].uvalue += 100000000;
> > 
> > and the counter is updated every second. So, it uses (2).
> >   
> > > 
> > > Wondering if it has the same bug, it seems it does not:
> > > drivers/media/dvb-frontends/dib8000.c#dib8000_get_time_us():3986
> > > 
> > > dib8000_get_time_us() seems to loop over multiple layers, and then
> > > assigns interleaving to the final layers interleaving (that looks like
> > > loop invariant code to me).
> > > 
> > > Mauro, should dib7000p_get_time_us() use c->layer[???].interleaving?  
> > 
> > I don't think that time interleaving would affect the bit rate.
> > I suspect that the dead code on dib8000 is just a dead code.
> >   
> > > I don't see a single reference to `layer` in
> > > drivers/media/dvb-frontends/dib7000p.c.  
> > 
> > Layers are specific for ISDB-T, but I think DVB-T (or at least DVB-T2)
> > may use time interleaving. 
> > 
> > Yet, as I said, the goal is to estimate the streaming bit rate. 
> > 
> > I don't remember anymore from where the dib8000 formula came.
> > 
> > My guts tell that time interleaving shouldn't do much changes (if any)
> > to the bit rate. I suspect that removing the dead code is likely
> > OK, but I'll try to see if I can find something related to where this
> > formula came.  
> 
> So we have two issues. One is the clang issue and clearly the code needs
> fixing up. The second issue is that we're not sure about the algorithm;
> I've been reading up on mpeg-ts but I'm not anywhere near getting to an
> answer on this.
> 
> How about we merge a patch which just fixes the clang issue and leave
> the rest of the code as-is for now?

I'm ok with that, but it would be better to add a FIXME note somewhere.

> 
> Thanks,
> 
> Sean
> 
> ---
> From c6e4c5f514c38511d2054c69f7b103e98c520af4 Mon Sep 17 00:00:00 2001
> From: Sean Young <sean@mess.org>
> Date: Tue, 4 Dec 2018 09:59:10 +0000
> Subject: [PATCH v2] media: dib7000p: Remove dead code
> 
> Clang warns that 'interleaving' is assigned to itself in this function.
> 
> drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
> assigning value of variable of type 'int' to itself [-Wself-assign]
>         interleaving = interleaving;
>         ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
> 1 warning generated.
> 
> Just remove the self-assign and leave existing code in place for now.
> 
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/media/dvb-frontends/dib7000p.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
> index 58387860b62d..cd84320c61c9 100644
> --- a/drivers/media/dvb-frontends/dib7000p.c
> +++ b/drivers/media/dvb-frontends/dib7000p.c
> @@ -1871,8 +1871,6 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
>  		break;
>  	}
>  
> -	interleaving = interleaving;
> -
>  	denom = bits_per_symbol * rate_num * fft_div * 384;

something like:

	/* 
	 * FIXME: check if the math makes sense. If so, fill the
	 * interleaving var.
	 */
>  
>  	/* If calculus gets wrong, wait for 1s for the next stats */



Thanks,
Mauro

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

* Re: [PATCH] [media] dib7000p: Remove dead code
  2018-12-04 11:57       ` Mauro Carvalho Chehab
@ 2018-12-04 13:39         ` Sean Young
  2018-12-04 18:54           ` Nick Desaulniers
  2018-12-04 19:04           ` Nathan Chancellor
  0 siblings, 2 replies; 10+ messages in thread
From: Sean Young @ 2018-12-04 13:39 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Nick Desaulniers, Nathan Chancellor, Mauro Carvalho Chehab,
	linux-media, LKML

On Tue, Dec 04, 2018 at 09:57:14AM -0200, Mauro Carvalho Chehab wrote:
> Em Tue, 4 Dec 2018 10:26:40 +0000
> Sean Young <sean@mess.org> escreveu:
> 
> > On Mon, Sep 17, 2018 at 07:39:36PM -0300, Mauro Carvalho Chehab wrote:
> > > Em Mon, 17 Sep 2018 10:58:32 -0700
> > > Nick Desaulniers <ndesaulniers@google.com> escreveu:
> > >   
> > > > On Fri, Sep 14, 2018 at 10:47 PM Nathan Chancellor
> > > > <natechancellor@gmail.com> wrote:  
> > > > >
> > > > > Clang warns that 'interleaving' is assigned to itself in this function.
> > > > >
> > > > > drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
> > > > > assigning value of variable of type 'int' to itself [-Wself-assign]
> > > > >         interleaving = interleaving;
> > > > >         ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
> > > > > 1 warning generated.
> > > > >
> > > > > It's correct. Just removing the self-assignment would sufficiently hide
> > > > > the warning but all of this code is dead because 'tmp' is zero due to
> > > > > being multiplied by zero. This doesn't appear to be an issue with
> > > > > dib8000, which this code was copied from in commit 041ad449683b
> > > > > ("[media] dib7000p: Add DVBv5 stats support").
> > > > >
> > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > > > ---
> > > > >  drivers/media/dvb-frontends/dib7000p.c | 10 ++--------
> > > > >  1 file changed, 2 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
> > > > > index 58387860b62d..25843658fc68 100644
> > > > > --- a/drivers/media/dvb-frontends/dib7000p.c
> > > > > +++ b/drivers/media/dvb-frontends/dib7000p.c
> > > > > @@ -1800,9 +1800,8 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)    
> > > > 
> > > > Something looks wrong here (with this function).  The patch is no
> > > > functional change, since as you point out `interleaving` is
> > > > initialized to 0, then never updated before read, but I think there's
> > > > an underlying bug here that should be fixed differently.  Thanks for
> > > > the patch though, as it does raise the question.
> > > > 
> > > > dib7000p_get_time_us has this comment above it:
> > > > 
> > > >   1798 /* FIXME: may require changes - this one was borrowed from
> > > > dib8000 */  
> > > 
> > > The goal of dib7000p_get_time_us() is to estimate how much time it
> > > takes, with current tuning parameters, to have a certain number of
> > > DVB-T packets. This is used for block error count. That's said,
> > > on a quick look, it seems that the code is not right on many ways.
> > > 
> > > It should be aligned with the amount of data it is required for
> > > dib7000 to update the block/bit error counters. There are two kinds
> > > of implementation:
> > > 
> > > 1) the frontend has an internal counter that it is shifted and made
> > >    available to the driver after a certain amount of received data
> > >    (usually in the order of 10^5 to 10^7 bits);
> > > 
> > > 2) the frontend has an internal timer that shifts the data from its
> > >    internal counter after a certain amount of time (usually at the
> > >    seconds range).
> > > 
> > > Different vendors opt for either one of the strategy. Some updates
> > > a counter with the amount of bits taken. Unfortunately, this is not
> > > the case of those dib* frontends. So, the Kernel has to estimate
> > > it, based on the tuning parameters.
> > > 
> > > From the code, it seems that, for block errors, it waits for 1,250,000
> > > bits to arrive (e. g. about 766 packets), so, it uses type (1) strategy:
> > > 
> > >                 /* Estimate the number of packets based on bitrate */
> > >                 if (!time_us)
> > >                         time_us = dib7000p_get_time_us(demod);
> > > 
> > >                 if (time_us) {
> > >                         blocks = 1250000ULL * 1000000ULL;	// the multiply here is to convert to microsseconds...
> > >                         do_div(blocks, time_us * 8 * 204);	// As here it divides by the time in microsseconds
> > >                         c->block_count.stat[0].scale = FE_SCALE_COUNTER;
> > >                         c->block_count.stat[0].uvalue += blocks;
> > >                 }
> > > 
> > > For BER, the logic assumes that the bit error count should be divided
> > > by 10^-8:
> > > 
> > >                 c->post_bit_count.stat[0].uvalue += 100000000;
> > > 
> > > and the counter is updated every second. So, it uses (2).
> > >   
> > > > 
> > > > Wondering if it has the same bug, it seems it does not:
> > > > drivers/media/dvb-frontends/dib8000.c#dib8000_get_time_us():3986
> > > > 
> > > > dib8000_get_time_us() seems to loop over multiple layers, and then
> > > > assigns interleaving to the final layers interleaving (that looks like
> > > > loop invariant code to me).
> > > > 
> > > > Mauro, should dib7000p_get_time_us() use c->layer[???].interleaving?  
> > > 
> > > I don't think that time interleaving would affect the bit rate.
> > > I suspect that the dead code on dib8000 is just a dead code.
> > >   
> > > > I don't see a single reference to `layer` in
> > > > drivers/media/dvb-frontends/dib7000p.c.  
> > > 
> > > Layers are specific for ISDB-T, but I think DVB-T (or at least DVB-T2)
> > > may use time interleaving. 
> > > 
> > > Yet, as I said, the goal is to estimate the streaming bit rate. 
> > > 
> > > I don't remember anymore from where the dib8000 formula came.
> > > 
> > > My guts tell that time interleaving shouldn't do much changes (if any)
> > > to the bit rate. I suspect that removing the dead code is likely
> > > OK, but I'll try to see if I can find something related to where this
> > > formula came.  
> > 
> > So we have two issues. One is the clang issue and clearly the code needs
> > fixing up. The second issue is that we're not sure about the algorithm;
> > I've been reading up on mpeg-ts but I'm not anywhere near getting to an
> > answer on this.
> > 
> > How about we merge a patch which just fixes the clang issue and leave
> > the rest of the code as-is for now?
> 
> I'm ok with that, but it would be better to add a FIXME note somewhere.
> 
> > 
> > Thanks,
> > 
> > Sean
> > 
> > ---
> > From c6e4c5f514c38511d2054c69f7b103e98c520af4 Mon Sep 17 00:00:00 2001
> > From: Sean Young <sean@mess.org>
> > Date: Tue, 4 Dec 2018 09:59:10 +0000
> > Subject: [PATCH v2] media: dib7000p: Remove dead code
> > 
> > Clang warns that 'interleaving' is assigned to itself in this function.
> > 
> > drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
> > assigning value of variable of type 'int' to itself [-Wself-assign]
> >         interleaving = interleaving;
> >         ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
> > 1 warning generated.
> > 
> > Just remove the self-assign and leave existing code in place for now.
> > 
> > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > Signed-off-by: Sean Young <sean@mess.org>
> > ---
> >  drivers/media/dvb-frontends/dib7000p.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
> > index 58387860b62d..cd84320c61c9 100644
> > --- a/drivers/media/dvb-frontends/dib7000p.c
> > +++ b/drivers/media/dvb-frontends/dib7000p.c
> > @@ -1871,8 +1871,6 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
> >  		break;
> >  	}
> >  
> > -	interleaving = interleaving;
> > -
> >  	denom = bits_per_symbol * rate_num * fft_div * 384;
> 
> something like:
> 
> 	/* 
> 	 * FIXME: check if the math makes sense. If so, fill the
> 	 * interleaving var.
> 	 */
> >  
> >  	/* If calculus gets wrong, wait for 1s for the next stats */
> 

Good point. 

Sean

From a31c18315830da40561db6443d3b90b8584d5232 Mon Sep 17 00:00:00 2001
From: Sean Young <sean@mess.org>
Date: Tue, 4 Dec 2018 09:59:10 +0000
Subject: [PATCH v3] media: dib7000p: Remove dead code

Clang warns that 'interleaving' is assigned to itself in this function.

drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
assigning value of variable of type 'int' to itself [-Wself-assign]
        interleaving = interleaving;
        ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
1 warning generated.

Just remove the self-assign and leave existing code in place for now.

Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/dvb-frontends/dib7000p.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
index 58387860b62d..2818e8def1b3 100644
--- a/drivers/media/dvb-frontends/dib7000p.c
+++ b/drivers/media/dvb-frontends/dib7000p.c
@@ -1871,10 +1871,13 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
 		break;
 	}
 
-	interleaving = interleaving;
-
 	denom = bits_per_symbol * rate_num * fft_div * 384;
 
+	/*
+	 * FIXME: check if the math makes sense. If so, fill the
+	 * interleaving var.
+	 */
+
 	/* If calculus gets wrong, wait for 1s for the next stats */
 	if (!denom)
 		return 0;
-- 
2.19.2


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

* Re: [PATCH] [media] dib7000p: Remove dead code
  2018-12-04 13:39         ` Sean Young
@ 2018-12-04 18:54           ` Nick Desaulniers
  2018-12-04 19:04           ` Nathan Chancellor
  1 sibling, 0 replies; 10+ messages in thread
From: Nick Desaulniers @ 2018-12-04 18:54 UTC (permalink / raw)
  To: sean
  Cc: mchehab+samsung, Nathan Chancellor, Mauro Carvalho Chehab,
	linux-media, LKML

On Tue, Dec 4, 2018 at 5:39 AM Sean Young <sean@mess.org> wrote:
>
> On Tue, Dec 04, 2018 at 09:57:14AM -0200, Mauro Carvalho Chehab wrote:
> > Em Tue, 4 Dec 2018 10:26:40 +0000
> > Sean Young <sean@mess.org> escreveu:
> >
> > > On Mon, Sep 17, 2018 at 07:39:36PM -0300, Mauro Carvalho Chehab wrote:
> > > > Em Mon, 17 Sep 2018 10:58:32 -0700
> > > > Nick Desaulniers <ndesaulniers@google.com> escreveu:
> > > >
> > > > > On Fri, Sep 14, 2018 at 10:47 PM Nathan Chancellor
> > > > > <natechancellor@gmail.com> wrote:
> > > > > >
> > > > > > Clang warns that 'interleaving' is assigned to itself in this function.
> > > > > >
> > > > > > drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
> > > > > > assigning value of variable of type 'int' to itself [-Wself-assign]
> > > > > >         interleaving = interleaving;
> > > > > >         ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
> > > > > > 1 warning generated.
> > > > > >
> > > > > > It's correct. Just removing the self-assignment would sufficiently hide
> > > > > > the warning but all of this code is dead because 'tmp' is zero due to
> > > > > > being multiplied by zero. This doesn't appear to be an issue with
> > > > > > dib8000, which this code was copied from in commit 041ad449683b
> > > > > > ("[media] dib7000p: Add DVBv5 stats support").
> > > > > >
> > > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > > > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > > > > ---
> > > > > >  drivers/media/dvb-frontends/dib7000p.c | 10 ++--------
> > > > > >  1 file changed, 2 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
> > > > > > index 58387860b62d..25843658fc68 100644
> > > > > > --- a/drivers/media/dvb-frontends/dib7000p.c
> > > > > > +++ b/drivers/media/dvb-frontends/dib7000p.c
> > > > > > @@ -1800,9 +1800,8 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
> > > > >
> > > > > Something looks wrong here (with this function).  The patch is no
> > > > > functional change, since as you point out `interleaving` is
> > > > > initialized to 0, then never updated before read, but I think there's
> > > > > an underlying bug here that should be fixed differently.  Thanks for
> > > > > the patch though, as it does raise the question.
> > > > >
> > > > > dib7000p_get_time_us has this comment above it:
> > > > >
> > > > >   1798 /* FIXME: may require changes - this one was borrowed from
> > > > > dib8000 */
> > > >
> > > > The goal of dib7000p_get_time_us() is to estimate how much time it
> > > > takes, with current tuning parameters, to have a certain number of
> > > > DVB-T packets. This is used for block error count. That's said,
> > > > on a quick look, it seems that the code is not right on many ways.
> > > >
> > > > It should be aligned with the amount of data it is required for
> > > > dib7000 to update the block/bit error counters. There are two kinds
> > > > of implementation:
> > > >
> > > > 1) the frontend has an internal counter that it is shifted and made
> > > >    available to the driver after a certain amount of received data
> > > >    (usually in the order of 10^5 to 10^7 bits);
> > > >
> > > > 2) the frontend has an internal timer that shifts the data from its
> > > >    internal counter after a certain amount of time (usually at the
> > > >    seconds range).
> > > >
> > > > Different vendors opt for either one of the strategy. Some updates
> > > > a counter with the amount of bits taken. Unfortunately, this is not
> > > > the case of those dib* frontends. So, the Kernel has to estimate
> > > > it, based on the tuning parameters.
> > > >
> > > > From the code, it seems that, for block errors, it waits for 1,250,000
> > > > bits to arrive (e. g. about 766 packets), so, it uses type (1) strategy:
> > > >
> > > >                 /* Estimate the number of packets based on bitrate */
> > > >                 if (!time_us)
> > > >                         time_us = dib7000p_get_time_us(demod);
> > > >
> > > >                 if (time_us) {
> > > >                         blocks = 1250000ULL * 1000000ULL; // the multiply here is to convert to microsseconds...
> > > >                         do_div(blocks, time_us * 8 * 204);        // As here it divides by the time in microsseconds
> > > >                         c->block_count.stat[0].scale = FE_SCALE_COUNTER;
> > > >                         c->block_count.stat[0].uvalue += blocks;
> > > >                 }
> > > >
> > > > For BER, the logic assumes that the bit error count should be divided
> > > > by 10^-8:
> > > >
> > > >                 c->post_bit_count.stat[0].uvalue += 100000000;
> > > >
> > > > and the counter is updated every second. So, it uses (2).
> > > >
> > > > >
> > > > > Wondering if it has the same bug, it seems it does not:
> > > > > drivers/media/dvb-frontends/dib8000.c#dib8000_get_time_us():3986
> > > > >
> > > > > dib8000_get_time_us() seems to loop over multiple layers, and then
> > > > > assigns interleaving to the final layers interleaving (that looks like
> > > > > loop invariant code to me).
> > > > >
> > > > > Mauro, should dib7000p_get_time_us() use c->layer[???].interleaving?
> > > >
> > > > I don't think that time interleaving would affect the bit rate.
> > > > I suspect that the dead code on dib8000 is just a dead code.
> > > >
> > > > > I don't see a single reference to `layer` in
> > > > > drivers/media/dvb-frontends/dib7000p.c.
> > > >
> > > > Layers are specific for ISDB-T, but I think DVB-T (or at least DVB-T2)
> > > > may use time interleaving.
> > > >
> > > > Yet, as I said, the goal is to estimate the streaming bit rate.
> > > >
> > > > I don't remember anymore from where the dib8000 formula came.
> > > >
> > > > My guts tell that time interleaving shouldn't do much changes (if any)
> > > > to the bit rate. I suspect that removing the dead code is likely
> > > > OK, but I'll try to see if I can find something related to where this
> > > > formula came.
> > >
> > > So we have two issues. One is the clang issue and clearly the code needs
> > > fixing up. The second issue is that we're not sure about the algorithm;
> > > I've been reading up on mpeg-ts but I'm not anywhere near getting to an
> > > answer on this.
> > >
> > > How about we merge a patch which just fixes the clang issue and leave
> > > the rest of the code as-is for now?
> >
> > I'm ok with that, but it would be better to add a FIXME note somewhere.
> >
> > >
> > > Thanks,
> > >
> > > Sean
> > >
> > > ---
> > > From c6e4c5f514c38511d2054c69f7b103e98c520af4 Mon Sep 17 00:00:00 2001
> > > From: Sean Young <sean@mess.org>
> > > Date: Tue, 4 Dec 2018 09:59:10 +0000
> > > Subject: [PATCH v2] media: dib7000p: Remove dead code
> > >
> > > Clang warns that 'interleaving' is assigned to itself in this function.
> > >
> > > drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
> > > assigning value of variable of type 'int' to itself [-Wself-assign]
> > >         interleaving = interleaving;
> > >         ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
> > > 1 warning generated.
> > >
> > > Just remove the self-assign and leave existing code in place for now.
> > >
> > > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > > Signed-off-by: Sean Young <sean@mess.org>

Thanks for taking the time to revisit the warning and clean it up.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

Might be nice to give Nathan some credit when applied:
Suggested-by: Nathan Chancellor <natechancellor@gmail.com>

> > > ---
> > >  drivers/media/dvb-frontends/dib7000p.c | 2 --
> > >  1 file changed, 2 deletions(-)
> > >
> > > diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
> > > index 58387860b62d..cd84320c61c9 100644
> > > --- a/drivers/media/dvb-frontends/dib7000p.c
> > > +++ b/drivers/media/dvb-frontends/dib7000p.c
> > > @@ -1871,8 +1871,6 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
> > >             break;
> > >     }
> > >
> > > -   interleaving = interleaving;
> > > -
> > >     denom = bits_per_symbol * rate_num * fft_div * 384;
> >
> > something like:
> >
> >       /*
> >        * FIXME: check if the math makes sense. If so, fill the
> >        * interleaving var.
> >        */
> > >
> > >     /* If calculus gets wrong, wait for 1s for the next stats */
> >
>
> Good point.
>
> Sean
>
> From a31c18315830da40561db6443d3b90b8584d5232 Mon Sep 17 00:00:00 2001
> From: Sean Young <sean@mess.org>
> Date: Tue, 4 Dec 2018 09:59:10 +0000
> Subject: [PATCH v3] media: dib7000p: Remove dead code
>
> Clang warns that 'interleaving' is assigned to itself in this function.
>
> drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
> assigning value of variable of type 'int' to itself [-Wself-assign]
>         interleaving = interleaving;
>         ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
> 1 warning generated.
>
> Just remove the self-assign and leave existing code in place for now.
>
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/media/dvb-frontends/dib7000p.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
> index 58387860b62d..2818e8def1b3 100644
> --- a/drivers/media/dvb-frontends/dib7000p.c
> +++ b/drivers/media/dvb-frontends/dib7000p.c
> @@ -1871,10 +1871,13 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
>                 break;
>         }
>
> -       interleaving = interleaving;
> -
>         denom = bits_per_symbol * rate_num * fft_div * 384;
>
> +       /*
> +        * FIXME: check if the math makes sense. If so, fill the
> +        * interleaving var.
> +        */
> +
>         /* If calculus gets wrong, wait for 1s for the next stats */
>         if (!denom)
>                 return 0;
> --
> 2.19.2
>


-- 
Thanks,
~Nick Desaulniers

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

* Re: [PATCH] [media] dib7000p: Remove dead code
  2018-12-04 13:39         ` Sean Young
  2018-12-04 18:54           ` Nick Desaulniers
@ 2018-12-04 19:04           ` Nathan Chancellor
  1 sibling, 0 replies; 10+ messages in thread
From: Nathan Chancellor @ 2018-12-04 19:04 UTC (permalink / raw)
  To: Sean Young
  Cc: Mauro Carvalho Chehab, Nick Desaulniers, Mauro Carvalho Chehab,
	linux-media, LKML

On Tue, Dec 04, 2018 at 01:39:22PM +0000, Sean Young wrote:
> On Tue, Dec 04, 2018 at 09:57:14AM -0200, Mauro Carvalho Chehab wrote:
> > Em Tue, 4 Dec 2018 10:26:40 +0000
> > Sean Young <sean@mess.org> escreveu:
> > 
> > > On Mon, Sep 17, 2018 at 07:39:36PM -0300, Mauro Carvalho Chehab wrote:
> > > > Em Mon, 17 Sep 2018 10:58:32 -0700
> > > > Nick Desaulniers <ndesaulniers@google.com> escreveu:
> > > >   
> > > > > On Fri, Sep 14, 2018 at 10:47 PM Nathan Chancellor
> > > > > <natechancellor@gmail.com> wrote:  
> > > > > >
> > > > > > Clang warns that 'interleaving' is assigned to itself in this function.
> > > > > >
> > > > > > drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
> > > > > > assigning value of variable of type 'int' to itself [-Wself-assign]
> > > > > >         interleaving = interleaving;
> > > > > >         ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
> > > > > > 1 warning generated.
> > > > > >
> > > > > > It's correct. Just removing the self-assignment would sufficiently hide
> > > > > > the warning but all of this code is dead because 'tmp' is zero due to
> > > > > > being multiplied by zero. This doesn't appear to be an issue with
> > > > > > dib8000, which this code was copied from in commit 041ad449683b
> > > > > > ("[media] dib7000p: Add DVBv5 stats support").
> > > > > >
> > > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > > > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > > > > ---
> > > > > >  drivers/media/dvb-frontends/dib7000p.c | 10 ++--------
> > > > > >  1 file changed, 2 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
> > > > > > index 58387860b62d..25843658fc68 100644
> > > > > > --- a/drivers/media/dvb-frontends/dib7000p.c
> > > > > > +++ b/drivers/media/dvb-frontends/dib7000p.c
> > > > > > @@ -1800,9 +1800,8 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)    
> > > > > 
> > > > > Something looks wrong here (with this function).  The patch is no
> > > > > functional change, since as you point out `interleaving` is
> > > > > initialized to 0, then never updated before read, but I think there's
> > > > > an underlying bug here that should be fixed differently.  Thanks for
> > > > > the patch though, as it does raise the question.
> > > > > 
> > > > > dib7000p_get_time_us has this comment above it:
> > > > > 
> > > > >   1798 /* FIXME: may require changes - this one was borrowed from
> > > > > dib8000 */  
> > > > 
> > > > The goal of dib7000p_get_time_us() is to estimate how much time it
> > > > takes, with current tuning parameters, to have a certain number of
> > > > DVB-T packets. This is used for block error count. That's said,
> > > > on a quick look, it seems that the code is not right on many ways.
> > > > 
> > > > It should be aligned with the amount of data it is required for
> > > > dib7000 to update the block/bit error counters. There are two kinds
> > > > of implementation:
> > > > 
> > > > 1) the frontend has an internal counter that it is shifted and made
> > > >    available to the driver after a certain amount of received data
> > > >    (usually in the order of 10^5 to 10^7 bits);
> > > > 
> > > > 2) the frontend has an internal timer that shifts the data from its
> > > >    internal counter after a certain amount of time (usually at the
> > > >    seconds range).
> > > > 
> > > > Different vendors opt for either one of the strategy. Some updates
> > > > a counter with the amount of bits taken. Unfortunately, this is not
> > > > the case of those dib* frontends. So, the Kernel has to estimate
> > > > it, based on the tuning parameters.
> > > > 
> > > > From the code, it seems that, for block errors, it waits for 1,250,000
> > > > bits to arrive (e. g. about 766 packets), so, it uses type (1) strategy:
> > > > 
> > > >                 /* Estimate the number of packets based on bitrate */
> > > >                 if (!time_us)
> > > >                         time_us = dib7000p_get_time_us(demod);
> > > > 
> > > >                 if (time_us) {
> > > >                         blocks = 1250000ULL * 1000000ULL;	// the multiply here is to convert to microsseconds...
> > > >                         do_div(blocks, time_us * 8 * 204);	// As here it divides by the time in microsseconds
> > > >                         c->block_count.stat[0].scale = FE_SCALE_COUNTER;
> > > >                         c->block_count.stat[0].uvalue += blocks;
> > > >                 }
> > > > 
> > > > For BER, the logic assumes that the bit error count should be divided
> > > > by 10^-8:
> > > > 
> > > >                 c->post_bit_count.stat[0].uvalue += 100000000;
> > > > 
> > > > and the counter is updated every second. So, it uses (2).
> > > >   
> > > > > 
> > > > > Wondering if it has the same bug, it seems it does not:
> > > > > drivers/media/dvb-frontends/dib8000.c#dib8000_get_time_us():3986
> > > > > 
> > > > > dib8000_get_time_us() seems to loop over multiple layers, and then
> > > > > assigns interleaving to the final layers interleaving (that looks like
> > > > > loop invariant code to me).
> > > > > 
> > > > > Mauro, should dib7000p_get_time_us() use c->layer[???].interleaving?  
> > > > 
> > > > I don't think that time interleaving would affect the bit rate.
> > > > I suspect that the dead code on dib8000 is just a dead code.
> > > >   
> > > > > I don't see a single reference to `layer` in
> > > > > drivers/media/dvb-frontends/dib7000p.c.  
> > > > 
> > > > Layers are specific for ISDB-T, but I think DVB-T (or at least DVB-T2)
> > > > may use time interleaving. 
> > > > 
> > > > Yet, as I said, the goal is to estimate the streaming bit rate. 
> > > > 
> > > > I don't remember anymore from where the dib8000 formula came.
> > > > 
> > > > My guts tell that time interleaving shouldn't do much changes (if any)
> > > > to the bit rate. I suspect that removing the dead code is likely
> > > > OK, but I'll try to see if I can find something related to where this
> > > > formula came.  
> > > 
> > > So we have two issues. One is the clang issue and clearly the code needs
> > > fixing up. The second issue is that we're not sure about the algorithm;
> > > I've been reading up on mpeg-ts but I'm not anywhere near getting to an
> > > answer on this.
> > > 
> > > How about we merge a patch which just fixes the clang issue and leave
> > > the rest of the code as-is for now?
> > 
> > I'm ok with that, but it would be better to add a FIXME note somewhere.
> > 
> > > 
> > > Thanks,
> > > 
> > > Sean
> > > 
> > > ---
> > > From c6e4c5f514c38511d2054c69f7b103e98c520af4 Mon Sep 17 00:00:00 2001
> > > From: Sean Young <sean@mess.org>
> > > Date: Tue, 4 Dec 2018 09:59:10 +0000
> > > Subject: [PATCH v2] media: dib7000p: Remove dead code
> > > 
> > > Clang warns that 'interleaving' is assigned to itself in this function.
> > > 
> > > drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
> > > assigning value of variable of type 'int' to itself [-Wself-assign]
> > >         interleaving = interleaving;
> > >         ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
> > > 1 warning generated.
> > > 
> > > Just remove the self-assign and leave existing code in place for now.
> > > 
> > > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > > Signed-off-by: Sean Young <sean@mess.org>
> > > ---
> > >  drivers/media/dvb-frontends/dib7000p.c | 2 --
> > >  1 file changed, 2 deletions(-)
> > > 
> > > diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
> > > index 58387860b62d..cd84320c61c9 100644
> > > --- a/drivers/media/dvb-frontends/dib7000p.c
> > > +++ b/drivers/media/dvb-frontends/dib7000p.c
> > > @@ -1871,8 +1871,6 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
> > >  		break;
> > >  	}
> > >  
> > > -	interleaving = interleaving;
> > > -
> > >  	denom = bits_per_symbol * rate_num * fft_div * 384;
> > 
> > something like:
> > 
> > 	/* 
> > 	 * FIXME: check if the math makes sense. If so, fill the
> > 	 * interleaving var.
> > 	 */
> > >  
> > >  	/* If calculus gets wrong, wait for 1s for the next stats */
> > 
> 
> Good point. 
> 
> Sean
> 
> From a31c18315830da40561db6443d3b90b8584d5232 Mon Sep 17 00:00:00 2001
> From: Sean Young <sean@mess.org>
> Date: Tue, 4 Dec 2018 09:59:10 +0000
> Subject: [PATCH v3] media: dib7000p: Remove dead code
> 
> Clang warns that 'interleaving' is assigned to itself in this function.
> 
> drivers/media/dvb-frontends/dib7000p.c:1874:15: warning: explicitly
> assigning value of variable of type 'int' to itself [-Wself-assign]
>         interleaving = interleaving;
>         ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
> 1 warning generated.
> 
> Just remove the self-assign and leave existing code in place for now.
> 
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Sean Young <sean@mess.org>

Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

> ---
>  drivers/media/dvb-frontends/dib7000p.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/dvb-frontends/dib7000p.c b/drivers/media/dvb-frontends/dib7000p.c
> index 58387860b62d..2818e8def1b3 100644
> --- a/drivers/media/dvb-frontends/dib7000p.c
> +++ b/drivers/media/dvb-frontends/dib7000p.c
> @@ -1871,10 +1871,13 @@ static u32 dib7000p_get_time_us(struct dvb_frontend *demod)
>  		break;
>  	}
>  
> -	interleaving = interleaving;
> -
>  	denom = bits_per_symbol * rate_num * fft_div * 384;
>  
> +	/*
> +	 * FIXME: check if the math makes sense. If so, fill the
> +	 * interleaving var.
> +	 */
> +
>  	/* If calculus gets wrong, wait for 1s for the next stats */
>  	if (!denom)
>  		return 0;
> -- 
> 2.19.2
> 

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

end of thread, other threads:[~2018-12-04 19:04 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-15  5:47 [PATCH] [media] dib7000p: Remove dead code Nathan Chancellor
2018-09-17 17:58 ` Nick Desaulniers
2018-09-17 22:39   ` Mauro Carvalho Chehab
2018-09-19 18:55     ` Nick Desaulniers
2018-10-25 18:09     ` Nathan Chancellor
2018-12-04 10:26     ` Sean Young
2018-12-04 11:57       ` Mauro Carvalho Chehab
2018-12-04 13:39         ` Sean Young
2018-12-04 18:54           ` Nick Desaulniers
2018-12-04 19:04           ` Nathan Chancellor

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).