| Message ID | 20170906135628.3513172-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 6, 2017 at 3:56 PM, Arnd Bergmann <arnd@arndb.de> wrote: > gcc thinks that interpreting a multiplication result as a bool > is confusing: > > drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c: In function 'read_pll': > drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c:133:8: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context] > > In this instance, I think using multiplication is more intuitive > than '&&', so I'm adding a comparison to zero instead to shut up > the warning. To further improve readability, I also make the > error case indented and leave the normal case as the final 'return' > statement. > I think to make perfectly clear why this check is done, we simply should precompute the denominator and do something like this: int denominator = M * P if (denominator == 0) return 0; return sclk * N / denominator; but with a better name for "denominator". I even imagine, that there are some macros in the kernel for dealing with something like this already, so that we could do instead: return AWESOME_DIV_MACRO(sclk * N, M * P) > Fixes: 7632b30e4b8b ("drm/nouveau/clk: namespace + nvidia gpu names (no binary change)") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > Originally submitted on July 14, but no reply. This is the same > patch again. The warning is currently disabled in mainline, but > I think we can turn it back on in the future, and this change here > seems harmless. > --- > drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c > index 96e0941c8edd..04b4f4ccf186 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c > @@ -130,10 +130,10 @@ read_pll(struct gt215_clk *clk, int idx, u32 pll) > sclk = read_clk(clk, 0x10 + idx, false); > } > > - if (M * P) > - return sclk * N / (M * P); > + if (M * P == 0) > + return 0; > > - return 0; > + return sclk * N / (M * P); > } > > static int > -- > 2.9.0 > > _______________________________________________ > Nouveau mailing list > Nouveau@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau
On Wed, Sep 6, 2017 at 4:20 PM, Karol Herbst <karolherbst@gmail.com> wrote: >> In this instance, I think using multiplication is more intuitive >> than '&&', so I'm adding a comparison to zero instead to shut up >> the warning. To further improve readability, I also make the >> error case indented and leave the normal case as the final 'return' >> statement. >> > > I think to make perfectly clear why this check is done, we simply > should precompute the denominator and do something like this: > > int denominator = M * P > if (denominator == 0) > return 0; > return sclk * N / denominator; > > but with a better name for "denominator". I don't know what M and P actually are in this function, so I couldn't come up with a much better name either, how about simply 'divisor'? > I even imagine, that there > are some macros in the kernel for dealing with something like this > already, so that we could do instead: > > return AWESOME_DIV_MACRO(sclk * N, M * P) I don't think that exists, the behavior for divide-by-zero really depends a lot on the context, and returning zero is probably often not a good solution. Arnd
On Wed, Sep 6, 2017 at 10:11 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wed, Sep 6, 2017 at 4:20 PM, Karol Herbst <karolherbst@gmail.com> wrote: >>> In this instance, I think using multiplication is more intuitive >>> than '&&', so I'm adding a comparison to zero instead to shut up >>> the warning. To further improve readability, I also make the >>> error case indented and leave the normal case as the final 'return' >>> statement. >>> >> >> I think to make perfectly clear why this check is done, we simply >> should precompute the denominator and do something like this: >> >> int denominator = M * P >> if (denominator == 0) >> return 0; >> return sclk * N / denominator; >> >> but with a better name for "denominator". > > I don't know what M and P actually are in this function, so I couldn't > come up with a much better name either, how about simply 'divisor'? > what about "MP"? M and P are simply dividers for the PLL configuration and there are two of them. I think "P" is the post divider. >> I even imagine, that there >> are some macros in the kernel for dealing with something like this >> already, so that we could do instead: >> >> return AWESOME_DIV_MACRO(sclk * N, M * P) > > I don't think that exists, the behavior for divide-by-zero really > depends a lot on the context, and returning zero is probably > often not a good solution. > > Arnd
On Wed, Sep 6, 2017 at 10:15 PM, Karol Herbst <karolherbst@gmail.com> wrote: > On Wed, Sep 6, 2017 at 10:11 PM, Arnd Bergmann <arnd@arndb.de> wrote: >>> but with a better name for "denominator". >> >> I don't know what M and P actually are in this function, so I couldn't >> come up with a much better name either, how about simply 'divisor'? >> > > what about "MP"? M and P are simply dividers for the PLL configuration > and there are two of them. I think "P" is the post divider. > Fine with me. That was actually what I initially did after your reply and then changed it to divisor. I've changed it back now and sent the new version. Arnd
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c index 96e0941c8edd..04b4f4ccf186 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c @@ -130,10 +130,10 @@ read_pll(struct gt215_clk *clk, int idx, u32 pll) sclk = read_clk(clk, 0x10 + idx, false); } - if (M * P) - return sclk * N / (M * P); + if (M * P == 0) + return 0; - return 0; + return sclk * N / (M * P); } static int
gcc thinks that interpreting a multiplication result as a bool is confusing: drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c: In function 'read_pll': drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c:133:8: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context] In this instance, I think using multiplication is more intuitive than '&&', so I'm adding a comparison to zero instead to shut up the warning. To further improve readability, I also make the error case indented and leave the normal case as the final 'return' statement. Fixes: 7632b30e4b8b ("drm/nouveau/clk: namespace + nvidia gpu names (no binary change)") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- Originally submitted on July 14, but no reply. This is the same patch again. The warning is currently disabled in mainline, but I think we can turn it back on in the future, and this change here seems harmless. --- drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)