linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Nick Desaulniers <ndesaulniers@google.com>
To: Nathan Chancellor <natechancellor@gmail.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
	linux-media@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] [media] dib7000p: Remove dead code
Date: Mon, 17 Sep 2018 10:58:32 -0700	[thread overview]
Message-ID: <CAKwvOdmQ4pbbPuvYrVYB9myD8ap36h6nLjEdL-mSbYjM37UJ_g@mail.gmail.com> (raw)
In-Reply-To: <20180915054739.14117-1-natechancellor@gmail.com>

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

  reply	other threads:[~2018-09-17 17:58 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-15  5:47 Nathan Chancellor
2018-09-17 17:58 ` Nick Desaulniers [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKwvOdmQ4pbbPuvYrVYB9myD8ap36h6nLjEdL-mSbYjM37UJ_g@mail.gmail.com \
    --to=ndesaulniers@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=natechancellor@gmail.com \
    --subject='Re: [PATCH] [media] dib7000p: Remove dead code' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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