| Message ID | 20180915054739.14117-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [media] dib7000p: Remove dead code | expand |
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 >
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
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
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
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 */
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
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;
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 >
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 >
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; }
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(-)