Re: [scan-admin@coverity.com: New Defects reported by Coverity Scan for Das U-Boot]

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 11419 bytes --]

On Mon, Jul 26, 2021 at 11:26:39PM -0400, Sean Anderson wrote:
> On 7/26/21 10:52 PM, Tom Rini wrote:
> > ----- Forwarded message from scan-admin@coverity.com -----
> > 
> > Date: Tue, 27 Jul 2021 01:10:27 +0000 (UTC)
> > From: scan-admin@coverity.com
> > To: tom.rini@gmail.com
> > Subject: New Defects reported by Coverity Scan for Das U-Boot
> > 
> > Hi,
> > 
> > Please find the latest report on new defect(s) introduced to Das U-Boot found with Coverity Scan.
> > 
> > 6 new defect(s) introduced to Das U-Boot found with Coverity Scan.
> > 9 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.
> > 
> > New defect(s) Reported-by: Coverity Scan
> > Showing 6 of 6 defect(s)
> > 
> > 
> > ** CID 332931:  Control flow issues  (NO_EFFECT)
> > /drivers/clk/clk_kendryte.c: 852 in k210_pll_set_rate()
> > 
> > 
> > ________________________________________________________________________________________________________
> > *** CID 332931:  Control flow issues  (NO_EFFECT)
> > /drivers/clk/clk_kendryte.c: 852 in k210_pll_set_rate()
> > 846     	int err;
> > 847     	const struct k210_pll_params *pll = &k210_plls[id];
> > 848     	struct k210_pll_config config = {};
> > 849     	u32 reg;
> > 850     	ulong calc_rate;
> > 851
> > > > >      CID 332931:  Control flow issues  (NO_EFFECT)
> > > > >      This less-than-zero comparison of an unsigned value is never true. "rate_in < 0UL".
> > 852     	if (rate_in < 0)
> > 853     		return rate_in;
> > 854
> > 855     	err = k210_pll_calc_config(rate, rate_in, &config);
> > 856     	if (err)
> > 857     		return err;
> > 
> 
> > ** CID 332929:  Integer handling issues  (NO_EFFECT)
> > /drivers/clk/clk_kendryte.c: 898 in k210_pll_get_rate()
> > 
> > 
> > ________________________________________________________________________________________________________
> > *** CID 332929:  Integer handling issues  (NO_EFFECT)
> > /drivers/clk/clk_kendryte.c: 898 in k210_pll_get_rate()
> > 892     static ulong k210_pll_get_rate(struct k210_clk_priv *priv, int id,
> > 893     			       ulong rate_in)
> > 894     {
> > 895     	u64 r, f, od;
> > 896     	u32 reg = readl(priv->base + k210_plls[id].off);
> > 897
> > > > >      CID 332929:  Integer handling issues  (NO_EFFECT)
> > > > >      This less-than-zero comparison of an unsigned value is never true. "rate_in < 0UL".
> > 898     	if (rate_in < 0 || (reg & K210_PLL_BYPASS))
> > 899     		return rate_in;
> > 900
> > 901     	if (!(reg & K210_PLL_PWRD))
> > 902     		return 0;
> > 903
> > 
> 
> 
> Will send a patch for these.
> 
> > ** CID 332927:    (DIVIDE_BY_ZERO)
> > /drivers/clk/clk_kendryte.c: 784 in k210_pll_calc_config()
> > /drivers/clk/clk_kendryte.c: 784 in k210_pll_calc_config()
> > /drivers/clk/clk_kendryte.c: 784 in k210_pll_calc_config()
> > /drivers/clk/clk_kendryte.c: 784 in k210_pll_calc_config()
> > /drivers/clk/clk_kendryte.c: 784 in k210_pll_calc_config()
> > /drivers/clk/clk_kendryte.c: 784 in k210_pll_calc_config()
> > 
> > 
> > ________________________________________________________________________________________________________
> > *** CID 332927:    (DIVIDE_BY_ZERO)
> > /drivers/clk/clk_kendryte.c: 784 in k210_pll_calc_config()
> > 778     			} else {
> > 779     				/*
> > 780     				 * There is no way to only divide once; we need
> > 781     				 * to examine the frequency with and without the
> > 782     				 * effect of od.
> > 783     				 */
> > > > >      CID 332927:    (DIVIDE_BY_ZERO)
> > > > >      In function call "__div64_32", division by expression "__base" which may be zero has undefined behavior.
> > 784     				u64 vco = DIV_ROUND_CLOSEST_ULL(rate_in * f, r);
> > 785
> > 786     				if (vco > 1750000000 || vco < 340000000)
> > 787     					out_of_spec = true;
> > 788     			}
> > 789
> > /drivers/clk/clk_kendryte.c: 784 in k210_pll_calc_config()
> > 778     			} else {
> > 779     				/*
> > 780     				 * There is no way to only divide once; we need
> > 781     				 * to examine the frequency with and without the
> > 782     				 * effect of od.
> > 783     				 */
> > > > >      CID 332927:    (DIVIDE_BY_ZERO)
> > > > >      In expression "(u32)_tmp % __base", modulo by expression "__base" which may be zero has undefined behavior.
> > 784     				u64 vco = DIV_ROUND_CLOSEST_ULL(rate_in * f, r);
> > 785
> > 786     				if (vco > 1750000000 || vco < 340000000)
> > 787     					out_of_spec = true;
> > 788     			}
> > 789
> > /drivers/clk/clk_kendryte.c: 784 in k210_pll_calc_config()
> > 778     			} else {
> > 779     				/*
> > 780     				 * There is no way to only divide once; we need
> > 781     				 * to examine the frequency with and without the
> > 782     				 * effect of od.
> > 783     				 */
> > > > >      CID 332927:    (DIVIDE_BY_ZERO)
> > > > >      In function call "__div64_32", division by expression "__base" which may be zero has undefined behavior.
> > 784     				u64 vco = DIV_ROUND_CLOSEST_ULL(rate_in * f, r);
> > 785
> > 786     				if (vco > 1750000000 || vco < 340000000)
> > 787     					out_of_spec = true;
> > 788     			}
> > 789
> > /drivers/clk/clk_kendryte.c: 784 in k210_pll_calc_config()
> > 778     			} else {
> > 779     				/*
> > 780     				 * There is no way to only divide once; we need
> > 781     				 * to examine the frequency with and without the
> > 782     				 * effect of od.
> > 783     				 */
> > > > >      CID 332927:    (DIVIDE_BY_ZERO)
> > > > >      In function call "__div64_32", division by expression "__base" which may be zero has undefined behavior.
> > 784     				u64 vco = DIV_ROUND_CLOSEST_ULL(rate_in * f, r);
> > 785
> > 786     				if (vco > 1750000000 || vco < 340000000)
> > 787     					out_of_spec = true;
> > 788     			}
> > 789
> > /drivers/clk/clk_kendryte.c: 784 in k210_pll_calc_config()
> > 778     			} else {
> > 779     				/*
> > 780     				 * There is no way to only divide once; we need
> > 781     				 * to examine the frequency with and without the
> > 782     				 * effect of od.
> > 783     				 */
> > > > >      CID 332927:    (DIVIDE_BY_ZERO)
> > > > >      In function call "__div64_32", division by expression "__base" which may be zero has undefined behavior.
> > 784     				u64 vco = DIV_ROUND_CLOSEST_ULL(rate_in * f, r);
> > 785
> > 786     				if (vco > 1750000000 || vco < 340000000)
> > 787     					out_of_spec = true;
> > 788     			}
> > 789
> > /drivers/clk/clk_kendryte.c: 784 in k210_pll_calc_config()
> > 778     			} else {
> > 779     				/*
> > 780     				 * There is no way to only divide once; we need
> > 781     				 * to examine the frequency with and without the
> > 782     				 * effect of od.
> > 783     				 */
> > > > >      CID 332927:    (DIVIDE_BY_ZERO)
> > > > >      In function call "__div64_32", division by expression "__base" which may be zero has undefined behavior.
> > 784     				u64 vco = DIV_ROUND_CLOSEST_ULL(rate_in * f, r);
> > 785
> > 786     				if (vco > 1750000000 || vco < 340000000)
> > 787     					out_of_spec = true;
> > 788     			}
> > 789
> > /drivers/clk/clk_kendryte.c: 784 in k210_pll_calc_config()
> > 778     			} else {
> > 779     				/*
> > 780     				 * There is no way to only divide once; we need
> > 781     				 * to examine the frequency with and without the
> > 782     				 * effect of od.
> > 783     				 */
> > > > >      CID 332927:    (DIVIDE_BY_ZERO)
> > > > >      In expression "(u32)_tmp % __base", modulo by expression "__base" which may be zero has undefined behavior.
> > 784     				u64 vco = DIV_ROUND_CLOSEST_ULL(rate_in * f, r);
> > 785
> > 786     				if (vco > 1750000000 || vco < 340000000)
> > 787     					out_of_spec = true;
> > 788     			}
> > 789
> > /drivers/clk/clk_kendryte.c: 784 in k210_pll_calc_config()
> > 778     			} else {
> > 779     				/*
> > 780     				 * There is no way to only divide once; we need
> > 781     				 * to examine the frequency with and without the
> > 782     				 * effect of od.
> > 783     				 */
> > > > >      CID 332927:    (DIVIDE_BY_ZERO)
> > > > >      In expression "(u32)_tmp % __base", modulo by expression "__base" which may be zero has undefined behavior.
> > 784     				u64 vco = DIV_ROUND_CLOSEST_ULL(rate_in * f, r);
> > 785
> > 786     				if (vco > 1750000000 || vco < 340000000)
> > 787     					out_of_spec = true;
> > 788     			}
> > 789
> > /drivers/clk/clk_kendryte.c: 784 in k210_pll_calc_config()
> > 778     			} else {
> > 779     				/*
> > 780     				 * There is no way to only divide once; we need
> > 781     				 * to examine the frequency with and without the
> > 782     				 * effect of od.
> > 783     				 */
> > > > >      CID 332927:    (DIVIDE_BY_ZERO)
> > > > >      In expression "(u32)_tmp % __base", modulo by expression "__base" which may be zero has undefined behavior.
> > 784     				u64 vco = DIV_ROUND_CLOSEST_ULL(rate_in * f, r);
> > 785
> > 786     				if (vco > 1750000000 || vco < 340000000)
> > 787     					out_of_spec = true;
> > 788     			}
> > 789
> > /drivers/clk/clk_kendryte.c: 784 in k210_pll_calc_config()
> > 778     			} else {
> > 779     				/*
> > 780     				 * There is no way to only divide once; we need
> > 781     				 * to examine the frequency with and without the
> > 782     				 * effect of od.
> > 783     				 */
> > > > >      CID 332927:    (DIVIDE_BY_ZERO)
> > > > >      In expression "(u32)_tmp % __base", modulo by expression "__base" which may be zero has undefined behavior.
> > 784     				u64 vco = DIV_ROUND_CLOSEST_ULL(rate_in * f, r);
> > 785
> > 786     				if (vco > 1750000000 || vco < 340000000)
> > 787     					out_of_spec = true;
> > 788     			}
> > 789
> > /drivers/clk/clk_kendryte.c: 784 in k210_pll_calc_config()
> > 778     			} else {
> > 779     				/*
> > 780     				 * There is no way to only divide once; we need
> > 781     				 * to examine the frequency with and without the
> > 782     				 * effect of od.
> > 783     				 */
> > > > >      CID 332927:    (DIVIDE_BY_ZERO)
> > > > >      In expression "(u32)_tmp % __base", modulo by expression "__base" which may be zero has undefined behavior.
> > 784     				u64 vco = DIV_ROUND_CLOSEST_ULL(rate_in * f, r);
> > 785
> > 786     				if (vco > 1750000000 || vco < 340000000)
> > 787     					out_of_spec = true;
> > 788     			}
> > 789
> 
> These are completely safe, but it is relatively non-obvious why. The
> only way that r can be 0 is on the very first iteration. When rate >
> rate_in, r gets assigned (to a non-zero number) immediately. For the
> converse, we only assign to r and od when r * od < goal. goal is
> calculated by multiplying f (which is always at least 1) with inv_ratio,
> shifted right by 32 bits. In the worst-case (the first iteration), this
> is just inv_ratio >> 32. But inv_ratio is rate_in << 32 / rate, and
> above we assumed that rate <= rate_in. So inv_ratio is always at least 1
> << 32, and we never divide by 0 :)
> 
> In the course of investigating the above, I added some additional test
> cases and discovered that we don't always get the best factors in some
> cases. I will also send a patch for this.

Thanks for looking so quickly!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]