| Message ID | 20230824104812.147775-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | Fix Versa3 clock mapping | expand |
Quoting Biju Das (2023-08-24 03:48:10) > Fix the below cocci warnings by replacing do_div()->div64_ul() and > bound the result with a max value of U16_MAX. > > cocci warnings: > drivers/clk/clk-versaclock3.c:404:2-8: WARNING: do_div() does a > 64-by-32 division, please consider using div64_ul instead. > > Reported-by: Julia Lawall <julia.lawall@inria.fr> > Closes: https://lore.kernel.org/r/202307270841.yr5HxYIl-lkp@intel.com/ > Fixes: 6e9aff555db7 ("clk: Add support for versa3 clock driver") > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- Applied to clk-fixes
On Thu, Aug 24, 2023 at 11:48:10AM +0100, Biju Das wrote: > Fix the below cocci warnings by replacing do_div()->div64_ul() and > bound the result with a max value of U16_MAX. > > cocci warnings: > drivers/clk/clk-versaclock3.c:404:2-8: WARNING: do_div() does a > 64-by-32 division, please consider using div64_ul instead. It's nice, but there is a room for a couple of improvements. See below. ... > /* Determine best fractional part, which is 16 bit wide */ > div_frc = rate % *parent_rate; > div_frc *= BIT(16) - 1; > - do_div(div_frc, *parent_rate); > > - vc3->div_frc = (u32)div_frc; > + vc3->div_frc = min_t(u64, div64_ul(div_frc, *parent_rate), U16_MAX); First of all, as Linus Torvalds pointed out [1] min_t() is often used as a shortcut for clamp(). Second one, the BIT(16) - 1 is specifically used as the value related to the bits in the hardware and u16 is a software type that coincidentially has the same maximum as the above mentioned bitfield. That said, here this should be clamped to the [0 .. BIT(16) - 1] range. Since the patch is applied perhaps you can cook a followup. To everyone the message is simple: try to not use typed version of min() and clamp() at all. > rate = (*parent_rate * > - (vc3->div_int * VC3_2_POW_16 + div_frc) / VC3_2_POW_16); > + (vc3->div_int * VC3_2_POW_16 + vc3->div_frc) / VC3_2_POW_16); [1]: https://lore.kernel.org/lkml/CAHk-=whwEAc22wm8h9FESPB5X+P4bLDgv0erBQMa1buTNQW7tA@mail.gmail.com/
Hi Andy Shevchenko, Thanks for the feedback. > Subject: Re: [PATCH v5 2/4] clk: vc3: Fix 64 by 64 division > > On Thu, Aug 24, 2023 at 11:48:10AM +0100, Biju Das wrote: > > Fix the below cocci warnings by replacing do_div()->div64_ul() and > > bound the result with a max value of U16_MAX. > > > > cocci warnings: > > drivers/clk/clk-versaclock3.c:404:2-8: WARNING: do_div() does a > > 64-by-32 division, please consider using div64_ul instead. > > It's nice, but there is a room for a couple of improvements. See below. Ok. > > ... > > > /* Determine best fractional part, which is 16 bit wide */ > > div_frc = rate % *parent_rate; > > div_frc *= BIT(16) - 1; > > - do_div(div_frc, *parent_rate); > > > > - vc3->div_frc = (u32)div_frc; > > + vc3->div_frc = min_t(u64, div64_ul(div_frc, *parent_rate), > > +U16_MAX); Ok, Will send follow up patch using clamp(). vc3->div_frc = clamp(div64_ul(div_frc, *parent_rate), 0, BIT(16) - 1); Cheers, Biju > > First of all, as Linus Torvalds pointed out [1] min_t() is often used as a > shortcut for clamp(). Second one, the BIT(16) - 1 is specifically used as > the value related to the bits in the hardware and u16 is a software type > that coincidentially has the same maximum as the above mentioned bitfield. > > That said, here this should be clamped to the [0 .. BIT(16) - 1] range. > > Since the patch is applied perhaps you can cook a followup. > > To everyone the message is simple: try to not use typed version of min() > and clamp() at all. > > > rate = (*parent_rate * > > - (vc3->div_int * VC3_2_POW_16 + div_frc) / VC3_2_POW_16); > > + (vc3->div_int * VC3_2_POW_16 + vc3->div_frc) / > VC3_2_POW_16); >
diff --git a/drivers/clk/clk-versaclock3.c b/drivers/clk/clk-versaclock3.c index 7ab2447bd203..b1a94db1f3c9 100644 --- a/drivers/clk/clk-versaclock3.c +++ b/drivers/clk/clk-versaclock3.c @@ -401,11 +401,10 @@ static long vc3_pll_round_rate(struct clk_hw *hw, unsigned long rate, /* Determine best fractional part, which is 16 bit wide */ div_frc = rate % *parent_rate; div_frc *= BIT(16) - 1; - do_div(div_frc, *parent_rate); - vc3->div_frc = (u32)div_frc; + vc3->div_frc = min_t(u64, div64_ul(div_frc, *parent_rate), U16_MAX); rate = (*parent_rate * - (vc3->div_int * VC3_2_POW_16 + div_frc) / VC3_2_POW_16); + (vc3->div_int * VC3_2_POW_16 + vc3->div_frc) / VC3_2_POW_16); } else { rate = *parent_rate * vc3->div_int; }
Fix the below cocci warnings by replacing do_div()->div64_ul() and bound the result with a max value of U16_MAX. cocci warnings: drivers/clk/clk-versaclock3.c:404:2-8: WARNING: do_div() does a 64-by-32 division, please consider using div64_ul instead. Reported-by: Julia Lawall <julia.lawall@inria.fr> Closes: https://lore.kernel.org/r/202307270841.yr5HxYIl-lkp@intel.com/ Fixes: 6e9aff555db7 ("clk: Add support for versa3 clock driver") Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v4->v5: * No change. v3->v4: * Used clamped value for rate calculation in vc3_pll_round_rate(). v2->v3: * Added to this patch series. v1->v2: * Added fixes tag. --- drivers/clk/clk-versaclock3.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)