Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1322968 > unrolled thread

[PATCH] clk: vt8500: don't return possibly uninitialized data

Back to article view | Back to linux.kernel

Contents

  [PATCH] clk: vt8500: don't return possibly uninitialized data Arnd Bergmann <arnd@arndb.de> - 2016-02-01 11:30 +0100
    Re: [PATCH] clk: vt8500: don't return possibly uninitialized data Stephen Boyd <sboyd@codeaurora.org> - 2016-02-02 02:20 +0100
      Re: [PATCH] clk: vt8500: don't return possibly uninitialized data Arnd Bergmann <arnd@arndb.de> - 2016-02-02 14:00 +0100
        Re: [PATCH] clk: vt8500: don't return possibly uninitialized data Stephen Boyd <sboyd@codeaurora.org> - 2016-02-02 20:50 +0100
          Re: [PATCH] clk: vt8500: don't return possibly uninitialized data Arnd Bergmann <arnd@arndb.de> - 2016-02-02 21:10 +0100
            Re: [PATCH] clk: vt8500: don't return possibly uninitialized data Roman Volkov <v1ron@mail.ru> - 2016-02-03 09:40 +0100
              Re: [PATCH] clk: vt8500: don't return possibly uninitialized data Arnd Bergmann <arnd@arndb.de> - 2016-02-03 10:20 +0100

#1322968 — [PATCH] clk: vt8500: don't return possibly uninitialized data

The clk-vt8500.c driver would previously enter an endless loop
when invalid settings got requested, this was now fixed. However,
the driver will now return uninitialized data for a subset of those
cases instead, as the gcc correctly warns:

clk/clk-vt8500.c: In function 'wm8650_find_pll_bits':
clk/clk-vt8500.c:423:12: error: 'best_div2' may be used uninitialized in this function [-Werror=maybe-uninitialized]
  *divisor2 = best_div2;
            ^
clk/clk-vt8500.c:422:12: error: 'best_div1' may be used uninitialized in this function [-Werror=maybe-uninitialized]
  *divisor1 = best_div1;
            ^
clk/clk-vt8500.c:421:14: error: 'best_mul' may be used uninitialized in this function [-Werror=maybe-uninitialized]
  *multiplier = best_mul;

This reworks the error handling in the driver so we now return
-EINVAL from clk_round_rate() and clk_set_rate() when we get
impossible inputs.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 090341b0a95d ("clk: vt8500: fix sign of possible PLL values")

diff --git a/drivers/clk/clk-vt8500.c b/drivers/clk/clk-vt8500.c
index 98c4492d2c0d..b0f76a84f1e9 100644
--- a/drivers/clk/clk-vt8500.c
+++ b/drivers/clk/clk-vt8500.c
@@ -355,7 +355,7 @@ CLK_OF_DECLARE(vt8500_device, "via,vt8500-device-clock", vtwm_device_clk_init);
 #define WM8850_BITS_TO_VAL(m, d1, d2)					\
 		((((m / 2) - 1) << 16) | ((d1 - 1) << 8) | d2)
 
-static void vt8500_find_pll_bits(unsigned long rate, unsigned long parent_rate,
+static int vt8500_find_pll_bits(unsigned long rate, unsigned long parent_rate,
 				u32 *multiplier, u32 *prediv)
 {
 	unsigned long tclk;
@@ -365,7 +365,7 @@ static void vt8500_find_pll_bits(unsigned long rate, unsigned long parent_rate,
 		pr_err("%s: requested rate out of range\n", __func__);
 		*multiplier = 0;
 		*prediv = 1;
-		return;
+		return -EINVAL;
 	}
 	if (rate <= parent_rate * 31)
 		/* use the prediv to double the resolution */
@@ -379,9 +379,11 @@ static void vt8500_find_pll_bits(unsigned long rate, unsigned long parent_rate,
 	if (tclk != rate)
 		pr_warn("%s: requested rate %lu, found rate %lu\n", __func__,
 								rate, tclk);
+
+	return 0;
 }
 
-static void wm8650_find_pll_bits(unsigned long rate, unsigned long parent_rate,
+static int wm8650_find_pll_bits(unsigned long rate, unsigned long parent_rate,
 				u32 *multiplier, u32 *divisor1, u32 *divisor2)
 {
 	u32 mul, div1;
@@ -404,7 +406,7 @@ static void wm8650_find_pll_bits(unsigned long rate, unsigned long parent_rate,
 					*multiplier = mul;
 					*divisor1 = div1;
 					*divisor2 = div2;
-					return;
+					return 0;
 				}
 
 				if (rate_err < best_err) {
@@ -415,12 +417,19 @@ static void wm8650_find_pll_bits(unsigned long rate, unsigned long parent_rate,
 				}
 			}
 
+	if (best_err == (unsigned long)-1) {
+		pr_warn("%s: impossible rate %lu\n", __func__, rate);
+		return -EINVAL;
+	}
+
 	/* if we got here, it wasn't an exact match */
 	pr_warn("%s: requested rate %lu, found rate %lu\n", __func__, rate,
 							rate - best_err);
 	*multiplier = best_mul;
 	*divisor1 = best_div1;
 	*divisor2 = best_div2;
+
+	return 0;
 }
 
 static u32 wm8750_get_filter(u32 parent_rate, u32 divisor1)
@@ -450,7 +459,7 @@ static u32 wm8750_get_filter(u32 parent_rate, u32 divisor1)
 	return 0;
 }
 
-static void wm8750_find_pll_bits(unsigned long rate, unsigned long parent_rate,
+static int wm8750_find_pll_bits(unsigned long rate, unsigned long parent_rate,
 				u32 *filter, u32 *multiplier, u32 *divisor1, u32 *divisor2)
 {
 	u32 mul;
@@ -474,7 +483,7 @@ static void wm8750_find_pll_bits(unsigned long rate, unsigned long parent_rate,
 					*multiplier = mul;
 					*divisor1 = div1;
 					*divisor2 = div2;
-					return;
+					return 0;
 				}
 
 				if (rate_err < best_err) {
@@ -485,6 +494,11 @@ static void wm8750_find_pll_bits(unsigned long rate, unsigned long parent_rate,
 				}
 			}
 
+	if (best_err == (unsigned long)-1) {
+		pr_warn("%s: impossible rate %lu\n", __func__, rate);
+		return -EINVAL;
+	}
+
 	/* if we got here, it wasn't an exact match */
 	pr_warn("%s: requested rate %lu, found rate %lu\n", __func__, rate,
 							rate - best_err);
@@ -493,9 +507,11 @@ static void wm8750_find_pll_bits(unsigned long rate, unsigned long parent_rate,
 	*multiplier = best_mul;
 	*divisor1 = best_div1;
 	*divisor2 = best_div2;
+
+	return 0;
 }
 
-static void wm8850_find_pll_bits(unsigned long rate, unsigned long parent_rate,
+static int wm8850_find_pll_bits(unsigned long rate, unsigned long parent_rate,
 				u32 *multiplier, u32 *divisor1, u32 *divisor2)
 {
 	u32 mul;
@@ -519,7 +535,7 @@ static void wm8850_find_pll_bits(unsigned long rate, unsigned long parent_rate,
 					*multiplier = mul;
 					*divisor1 = div1;
 					*divisor2 = div2;
-					return;
+					return 0;
 				}
 
 				if (rate_err < best_err) {
@@ -530,6 +546,11 @@ static void wm8850_find_pll_bits(unsigned long rate, unsigned long parent_rate,
 				}
 			}
 
+	if (best_err == (unsigned long)-1) {
+		pr_warn("%s: impossible rate %lu\n", __func__, rate);
+		return -EINVAL;
+	}
+
 	/* if we got here, it wasn't an exact match */
 	pr_warn("%s: requested rate %lu, found rate %lu\n", __func__, rate,
 							rate - best_err);
@@ -537,6 +558,8 @@ static void wm8850_find_pll_bits(unsigned long rate, unsigned long parent_rate,
 	*multiplier = best_mul;
 	*divisor1 = best_div1;
 	*divisor2 = best_div2;
+
+	return 0;
 }
 
 static int vtwm_pll_set_rate(struct clk_hw *hw, unsigned long rate,
@@ -546,31 +569,39 @@ static int vtwm_pll_set_rate(struct clk_hw *hw, unsigned long rate,
 	u32 filter, mul, div1, div2;
 	u32 pll_val;
 	unsigned long flags = 0;
+	int ret;
 
 	/* sanity check */
 
 	switch (pll->type) {
 	case PLL_TYPE_VT8500:
-		vt8500_find_pll_bits(rate, parent_rate, &mul, &div1);
-		pll_val = VT8500_BITS_TO_VAL(mul, div1);
+		ret = vt8500_find_pll_bits(rate, parent_rate, &mul, &div1);
+		if (!ret)
+			pll_val = VT8500_BITS_TO_VAL(mul, div1);
 		break;
 	case PLL_TYPE_WM8650:
-		wm8650_find_pll_bits(rate, parent_rate, &mul, &div1, &div2);
-		pll_val = WM8650_BITS_TO_VAL(mul, div1, div2);
+		ret = wm8650_find_pll_bits(rate, parent_rate, &mul, &div1, &div2);
+		if (!ret)
+			pll_val = WM8650_BITS_TO_VAL(mul, div1, div2);
 		break;
 	case PLL_TYPE_WM8750:
-		wm8750_find_pll_bits(rate, parent_rate, &filter, &mul, &div1, &div2);
-		pll_val = WM8750_BITS_TO_VAL(filter, mul, div1, div2);
+		ret = wm8750_find_pll_bits(rate, parent_rate, &filter, &mul, &div1, &div2);
+		if (!ret)
+			pll_val = WM8750_BITS_TO_VAL(filter, mul, div1, div2);
 		break;
 	case PLL_TYPE_WM8850:
-		wm8850_find_pll_bits(rate, parent_rate, &mul, &div1, &div2);
-		pll_val = WM8850_BITS_TO_VAL(mul, div1, div2);
+		ret = wm8850_find_pll_bits(rate, parent_rate, &mul, &div1, &div2);
+		if (!ret)
+			pll_val = WM8850_BITS_TO_VAL(mul, div1, div2);
 		break;
 	default:
 		pr_err("%s: invalid pll type\n", __func__);
-		return 0;
+		ret = -EINVAL;
 	}
 
+	if (ret)
+		return ret;
+
 	spin_lock_irqsave(pll->lock, flags);
 
 	vt8500_pmc_wait_busy();
@@ -588,28 +619,36 @@ static long vtwm_pll_round_rate(struct clk_hw *hw, unsigned long rate,
 	struct clk_pll *pll = to_clk_pll(hw);
 	u32 filter, mul, div1, div2;
 	long round_rate;
+	int ret;
 
 	switch (pll->type) {
 	case PLL_TYPE_VT8500:
-		vt8500_find_pll_bits(rate, *prate, &mul, &div1);
-		round_rate = VT8500_BITS_TO_FREQ(*prate, mul, div1);
+		ret = vt8500_find_pll_bits(rate, *prate, &mul, &div1);
+		if (!ret)
+			round_rate = VT8500_BITS_TO_FREQ(*prate, mul, div1);
 		break;
 	case PLL_TYPE_WM8650:
-		wm8650_find_pll_bits(rate, *prate, &mul, &div1, &div2);
-		round_rate = WM8650_BITS_TO_FREQ(*prate, mul, div1, div2);
+		ret = wm8650_find_pll_bits(rate, *prate, &mul, &div1, &div2);
+		if (!ret)
+			round_rate = WM8650_BITS_TO_FREQ(*prate, mul, div1, div2);
 		break;
 	case PLL_TYPE_WM8750:
-		wm8750_find_pll_bits(rate, *prate, &filter, &mul, &div1, &div2);
-		round_rate = WM8750_BITS_TO_FREQ(*prate, mul, div1, div2);
+		ret = wm8750_find_pll_bits(rate, *prate, &filter, &mul, &div1, &div2);
+		if (!ret)
+			round_rate = WM8750_BITS_TO_FREQ(*prate, mul, div1, div2);
 		break;
 	case PLL_TYPE_WM8850:
-		wm8850_find_pll_bits(rate, *prate, &mul, &div1, &div2);
-		round_rate = WM8850_BITS_TO_FREQ(*prate, mul, div1, div2);
+		ret = wm8850_find_pll_bits(rate, *prate, &mul, &div1, &div2);
+		if (!ret)
+			round_rate = WM8850_BITS_TO_FREQ(*prate, mul, div1, div2);
 		break;
 	default:
-		round_rate = 0;
+		ret = -EINVAL;
 	}
 
+	if (ret)
+		return ret;
+
 	return round_rate;
 }
 

[toc] | [next] | [standalone]

#1323690

On 02/01, Arnd Bergmann wrote:
> The clk-vt8500.c driver would previously enter an endless loop
> when invalid settings got requested, this was now fixed. However,
> the driver will now return uninitialized data for a subset of those
> cases instead, as the gcc correctly warns:
> 
> clk/clk-vt8500.c: In function 'wm8650_find_pll_bits':
> clk/clk-vt8500.c:423:12: error: 'best_div2' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>   *divisor2 = best_div2;
>             ^
> clk/clk-vt8500.c:422:12: error: 'best_div1' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>   *divisor1 = best_div1;
>             ^
> clk/clk-vt8500.c:421:14: error: 'best_mul' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>   *multiplier = best_mul;
> 
> This reworks the error handling in the driver so we now return
> -EINVAL from clk_round_rate() and clk_set_rate() when we get
> impossible inputs.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 090341b0a95d ("clk: vt8500: fix sign of possible PLL values")

My compiler still gets warnings even after this patch is applied.

drivers/clk/clk-vt8500.c: In function ‘wm8750_find_pll_bits’:
drivers/clk/clk-vt8500.c:509:12: warning: ‘best_div2’ may be used uninitialized in this function [-Wmaybe-uninitialized]
drivers/clk/clk-vt8500.c:508:12: warning: ‘best_div1’ may be used uninitialized in this function [-Wmaybe-uninitialized]
drivers/clk/clk-vt8500.c:507:14: warning: ‘best_mul’ may be used uninitialized in this function [-Wmaybe-uninitialized]
drivers/clk/clk-vt8500.c: In function ‘wm8650_find_pll_bits’:
drivers/clk/clk-vt8500.c:430:12: warning: ‘best_div2’ may be used uninitialized in this function [-Wmaybe-uninitialized]
drivers/clk/clk-vt8500.c:429:12: warning: ‘best_div1’ may be used uninitialized in this function [-Wmaybe-uninitialized]
drivers/clk/clk-vt8500.c:428:14: warning: ‘best_mul’ may be used uninitialized in this function [-Wmaybe-uninitialized]
drivers/clk/clk-vt8500.c: In function ‘wm8850_find_pll_bits’:
drivers/clk/clk-vt8500.c:560:12: warning: ‘best_div2’ may be used uninitialized in this function [-Wmaybe-uninitialized]
drivers/clk/clk-vt8500.c:559:12: warning: ‘best_div1’ may be used uninitialized in this function [-Wmaybe-uninitialized]
drivers/clk/clk-vt8500.c:558:14: warning: ‘best_mul’ may be used uninitialized in this function [-Wmaybe-uninitialized]

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1324052

On Monday 01 February 2016 17:15:45 Stephen Boyd wrote:
> My compiler still gets warnings even after this patch is applied.
> 
> drivers/clk/clk-vt8500.c: In function ‘wm8750_find_pll_bits’:
> drivers/clk/clk-vt8500.c:509:12: warning: ‘best_div2’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> drivers/clk/clk-vt8500.c:508:12: warning: ‘best_div1’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> drivers/clk/clk-vt8500.c:507:14: warning: ‘best_mul’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> drivers/clk/clk-vt8500.c: In function ‘wm8650_find_pll_bits’:
> drivers/clk/clk-vt8500.c:430:12: warning: ‘best_div2’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> drivers/clk/clk-vt8500.c:429:12: warning: ‘best_div1’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> drivers/clk/clk-vt8500.c:428:14: warning: ‘best_mul’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> drivers/clk/clk-vt8500.c: In function ‘wm8850_find_pll_bits’:
> drivers/clk/clk-vt8500.c:560:12: warning: ‘best_div2’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> drivers/clk/clk-vt8500.c:559:12: warning: ‘best_div1’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> drivers/clk/clk-vt8500.c:558:14: warning: ‘best_mul’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> 

I see what you mean now. I checked different gcc versions, and with my patch I get
the warnings for 4.6 through 4.9, but not for 5.x.

In general, I tried to only address warnings I still see with newer gcc version,
as they are better about false positives. Do you think it's ok to take the
patch as is then? Otherwise we probably have to add fake initializations which
would shut up the warnings but not help with the code quality.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1324563

On 02/02, Arnd Bergmann wrote:
> On Monday 01 February 2016 17:15:45 Stephen Boyd wrote:
> 
> I see what you mean now. I checked different gcc versions, and with my patch I get
> the warnings for 4.6 through 4.9, but not for 5.x.
> 
> In general, I tried to only address warnings I still see with newer gcc version,
> as they are better about false positives. Do you think it's ok to take the
> patch as is then? Otherwise we probably have to add fake initializations which
> would shut up the warnings but not help with the code quality.
> 

Sure. I was hoping something could be done to restructure the
code to make it easier for the compiler to figure out the
variables would be initialized. But you're the one who's sending
the patch to silence them so if you're satisfied I'm not going to
spend too much time on this.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1324580

On Tuesday 02 February 2016 11:47:13 Stephen Boyd wrote:
> On 02/02, Arnd Bergmann wrote:
> > On Monday 01 February 2016 17:15:45 Stephen Boyd wrote:
> > 
> > I see what you mean now. I checked different gcc versions, and with my patch I get
> > the warnings for 4.6 through 4.9, but not for 5.x.
> > 
> > In general, I tried to only address warnings I still see with newer gcc version,
> > as they are better about false positives. Do you think it's ok to take the
> > patch as is then? Otherwise we probably have to add fake initializations which
> > would shut up the warnings but not help with the code quality.
> > 
> 
> Sure. I was hoping something could be done to restructure the
> code to make it easier for the compiler to figure out the
> variables would be initialized. But you're the one who's sending
> the patch to silence them so if you're satisfied I'm not going to
> spend too much time on this.
> 

Ok, thanks!

Note that this one patch was for a real bug involving undefined
behavior that is now fixed.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1325043

В Tue, 02 Feb 2016 21:00:29 +0100
Arnd Bergmann <arnd@arndb.de> пишет:

> On Tuesday 02 February 2016 11:47:13 Stephen Boyd wrote:
> > On 02/02, Arnd Bergmann wrote:  
> > > On Monday 01 February 2016 17:15:45 Stephen Boyd wrote:
> > > 
> > > I see what you mean now. I checked different gcc versions, and
> > > with my patch I get the warnings for 4.6 through 4.9, but not for
> > > 5.x.
> > > 
> > > In general, I tried to only address warnings I still see with
> > > newer gcc version, as they are better about false positives. Do
> > > you think it's ok to take the patch as is then? Otherwise we
> > > probably have to add fake initializations which would shut up the
> > > warnings but not help with the code quality. 
> > 
> > Sure. I was hoping something could be done to restructure the
> > code to make it easier for the compiler to figure out the
> > variables would be initialized. But you're the one who's sending
> > the patch to silence them so if you're satisfied I'm not going to
> > spend too much time on this.
> >   
> 
> Ok, thanks!
> 
> Note that this one patch was for a real bug involving undefined
> behavior that is now fixed.
> 
> 	Arnd

Hi Arnd,

Thanks for fixing this code! Did someone reproduce this bug, or this
is something theoretical, based on the code analysis? I just never
heard about the issue. I can look into the code on the weekends too, I
have WM8505\WM8650 machines to test.

Is it enough to run the regular kernel build for WM8650 to see the
warnings, or there are special options in the kernel to run the compiler
test?

Regards
Roman

[toc] | [prev] | [next] | [standalone]

#1325077

On Wednesday 03 February 2016 11:36:26 Roman Volkov wrote:
> 
> Hi Arnd,
> 
> Thanks for fixing this code! Did someone reproduce this bug, or this
> is something theoretical, based on the code analysis? I just never
> heard about the issue. I can look into the code on the weekends too, I
> have WM8505\WM8650 machines to test.

I only fixed it after analysing the gcc warnings I got after Andrzej Hajda's
patch, and he also did it to fix the initial problem he found using
coccinelle, so I don't think anyone has run into the problem on live
hardware.

As long as all drivers ask for clock rates that are valid, you won't
see either problem.
 
> Is it enough to run the regular kernel build for WM8650 to see the
> warnings, or there are special options in the kernel to run the compiler
> test?

The warning is hidden if you build with -Os (CONFIG_CC_OPTIMIZE_FOR_SIZE),
and it may not happen with all gcc versions. I was using gcc-5.2.

	Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web