Re: [PATCH] [media] dib7000p: Remove dead code
On Tue, Dec 04, 2018 at 09:57:14AM -0200, Mauro Carvalho Chehab wrote: > Em Tue, 4 Dec 2018 10:26:40 + > Sean Young 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 escreveu: > > > > > > > On Fri, Sep 14, 2018 at 10:47 PM Nathan Chancellor > > > > 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 > > > > > Signed-off-by: Nathan Chancellor > > > > > --- > > > > > 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 = 125ULL * 100ULL; // 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 += 1; > > > > > > 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
Re: [PATCH] [media] dib7000p: Remove dead code
On Tue, Dec 04, 2018 at 09:57:14AM -0200, Mauro Carvalho Chehab wrote: > Em Tue, 4 Dec 2018 10:26:40 + > Sean Young 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 escreveu: > > > > > > > On Fri, Sep 14, 2018 at 10:47 PM Nathan Chancellor > > > > 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 > > > > > Signed-off-by: Nathan Chancellor > > > > > --- > > > > > 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 = 125ULL * 100ULL; // 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 += 1; > > > > > > 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