| Message ID | 20191004044649.2405-1-yzhai003@ucr.edu |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: stm32: Fix the usage of uninitialized variable in stm32_pwm_config() | expand |
Hello, On Thu, Oct 03, 2019 at 09:46:49PM -0700, Yizhuo wrote: > Inside function stm32_pwm_config(), variable "psc" and " arr" > could be uninitialized if regmap_read() returns -EINVALs. > However, they are used later in the if statement to decide > the return value which is potentially unsafe. > > The same case happens in function stm32_pwm_detect_channels() > with variable "ccer", but we cannot just return -EINVAL because > the error code is not acceptable by the caller. Aslo, the variable s/Aslo/Also/ > "ccer" in functionstm32_pwm_detect_complementary() could also be s/functionstm32_pwm_detect_/function stm32_pwm_detect_/ > uninitialized, since stm32_pwm_detect_complementary() returns void, > the patch is not easy. active_channels() is also affected. Also there are calls to regmap_update_bits which should have their return values checked. While a patch to fix these all is not trivial it is certainly possible and I would prefer to fix the problem completely. Best regards Uwe
On 10/4/19 8:23 AM, Uwe Kleine-König wrote: > Hello, > > On Thu, Oct 03, 2019 at 09:46:49PM -0700, Yizhuo wrote: >> Inside function stm32_pwm_config(), variable "psc" and " arr" >> could be uninitialized if regmap_read() returns -EINVALs. >> However, they are used later in the if statement to decide >> the return value which is potentially unsafe. Hi Yizhuo, like for the your patch in IIO trigger regmap_read could only failed if the hardware block is no more clocked and in this case we won't return of regmap_read. Testing regmap_read() return value just add code but doesn't provide a valid information. If you really want to log all the possible errors cases please do it in regmap code itself and not in *all* the drivers. Thanks, Benjamin >> >> The same case happens in function stm32_pwm_detect_channels() >> with variable "ccer", but we cannot just return -EINVAL because >> the error code is not acceptable by the caller. Aslo, the variable > s/Aslo/Also/ > >> "ccer" in functionstm32_pwm_detect_complementary() could also be > s/functionstm32_pwm_detect_/function stm32_pwm_detect_/ > >> uninitialized, since stm32_pwm_detect_complementary() returns void, >> the patch is not easy. > active_channels() is also affected. Also there are calls to > regmap_update_bits which should have their return values checked. > > While a patch to fix these all is not trivial it is certainly possible > and I would prefer to fix the problem completely. > > Best regards > Uwe >
Hello, Cc += Mark Brown who maintains regmap On Fri, Oct 04, 2019 at 09:09:51AM +0000, Benjamin GAIGNARD wrote: > > On 10/4/19 8:23 AM, Uwe Kleine-König wrote: > > Hello, > > > > On Thu, Oct 03, 2019 at 09:46:49PM -0700, Yizhuo wrote: > >> Inside function stm32_pwm_config(), variable "psc" and " arr" > >> could be uninitialized if regmap_read() returns -EINVALs. > >> However, they are used later in the if statement to decide > >> the return value which is potentially unsafe. > > Hi Yizhuo, > > like for the your patch in IIO trigger regmap_read could only failed > if the hardware block is no more clocked and in this case we won't > return of regmap_read. I'm not sure this is aligned with how regmap is supposed to be used. I think the driver making use of regmap is not supposed to make any assumptions about how and when a read or write access can or cannot fail and instead is supposed to check all return values. So IMHO the patch goes in the right direction. > Testing regmap_read() return value just add code but doesn't provide a > valid information. > If you really want to log all the possible errors cases please do it in > regmap code itself and > not in *all* the drivers. Best regards Uwe
On Fri, Oct 04, 2019 at 10:08:04PM +0200, Uwe Kleine-König wrote: > On Fri, Oct 04, 2019 at 09:09:51AM +0000, Benjamin GAIGNARD wrote: > > like for the your patch in IIO trigger regmap_read could only failed > > if the hardware block is no more clocked and in this case we won't > > return of regmap_read. > I'm not sure this is aligned with how regmap is supposed to be used. I > think the driver making use of regmap is not supposed to make any > assumptions about how and when a read or write access can or cannot fail > and instead is supposed to check all return values. So IMHO the patch > goes in the right direction. Yeah, if you're being very good about checking return values you really should check them all the time in case something comes up - you could get errors that don't come from the physical access, for example on read if a register is marked as unreadable, or if physical access is required but the map has been marked cache only. That said a lot of people will just not check anything since it's not common to see errors and errors that do occur tend to have other quite visible effects like I2C bus errors or system hangs.
diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c index 359b08596d9e..22c54df52977 100644 --- a/drivers/pwm/pwm-stm32.c +++ b/drivers/pwm/pwm-stm32.c @@ -346,9 +346,15 @@ static int stm32_pwm_config(struct stm32_pwm *priv, int ch, */ if (active_channels(priv) & ~(1 << ch * 4)) { u32 psc, arr; + int ret; - regmap_read(priv->regmap, TIM_PSC, &psc); - regmap_read(priv->regmap, TIM_ARR, &arr); + ret = regmap_read(priv->regmap, TIM_PSC, &psc); + if (ret) + return ret; + + ret = regmap_read(priv->regmap, TIM_ARR, &arr); + if (ret) + return ret; if ((psc != prescaler) || (arr != prd - 1)) return -EBUSY;
Inside function stm32_pwm_config(), variable "psc" and " arr" could be uninitialized if regmap_read() returns -EINVALs. However, they are used later in the if statement to decide the return value which is potentially unsafe. The same case happens in function stm32_pwm_detect_channels() with variable "ccer", but we cannot just return -EINVAL because the error code is not acceptable by the caller. Aslo, the variable "ccer" in functionstm32_pwm_detect_complementary() could also be uninitialized, since stm32_pwm_detect_complementary() returns void, the patch is not easy. Signed-off-by: Yizhuo <yzhai003@ucr.edu> --- drivers/pwm/pwm-stm32.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)