diff mbox series

hwmon: (pmbus) Move boolean error condition check to generating code

Message ID 20200910103036.143702-1-linux@roeck-us.net
State Accepted
Headers show
Series hwmon: (pmbus) Move boolean error condition check to generating code | expand

Commit Message

Guenter Roeck Sept. 10, 2020, 10:30 a.m. UTC
0-day rightfully complains about a sometimes uninitialized variable
in pmbus_get_boolean().

drivers/hwmon/pmbus/pmbus_core.c:903:13: warning:
		variable 'ret' is used uninitialized whenever 'if' condition is true
	} else if (!s1 || !s2) {

While that is technically true, it won't be hit in the field since the
condition indicates a programming error. Move the check of that condition
into the code generating the attribute entry, and refuse generating the
attribute if the condition is true. Swap the condition check in
pmbus_get_boolean() to ensure that static analyzers don't get a hiccup
(because we check if s1 and s2 are NULL, static analyzers may believe
that they can be NULL independently of each other).

Reported-by: kernel test robot <lkp@intel.com>
Cc: Alex Qiu <xqiu@google.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/pmbus/pmbus_core.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Alex Qiu Sept. 10, 2020, 3:57 p.m. UTC | #1
On Thu, Sep 10, 2020 at 3:30 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> 0-day rightfully complains about a sometimes uninitialized variable
> in pmbus_get_boolean().
>
> drivers/hwmon/pmbus/pmbus_core.c:903:13: warning:
>                 variable 'ret' is used uninitialized whenever 'if' condition is true
>         } else if (!s1 || !s2) {
>
> While that is technically true, it won't be hit in the field since the
> condition indicates a programming error. Move the check of that condition
> into the code generating the attribute entry, and refuse generating the
> attribute if the condition is true. Swap the condition check in
> pmbus_get_boolean() to ensure that static analyzers don't get a hiccup
> (because we check if s1 and s2 are NULL, static analyzers may believe
> that they can be NULL independently of each other).
>
> Reported-by: kernel test robot <lkp@intel.com>
> Cc: Alex Qiu <xqiu@google.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Alex Qiu <xqiu@google.com>
> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 5113b5eca17a..6be08696189c 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -898,11 +898,7 @@ static int pmbus_get_boolean(struct i2c_client *client, struct pmbus_boolean *b,
>                 pmbus_update_sensor_data(client, s2);
>
>         regval = status & mask;
> -       if (!s1 && !s2) {
> -               ret = !!regval;
> -       } else if (!s1 || !s2) {
> -               WARN(1, "Bad boolean descriptor %p: s1=%p, s2=%p\n", b, s1, s2);
> -       } else {
> +       if (s1 && s2) {
>                 s64 v1, v2;
>
>                 if (s1->data < 0) {
> @@ -917,6 +913,8 @@ static int pmbus_get_boolean(struct i2c_client *client, struct pmbus_boolean *b,
>                 v1 = pmbus_reg2data(data, s1);
>                 v2 = pmbus_reg2data(data, s2);
>                 ret = !!(regval && v1 >= v2);
> +       } else {
> +               ret = !!regval;
>         }
>  unlock:
>         mutex_unlock(&data->update_lock);
> @@ -1044,6 +1042,9 @@ static int pmbus_add_boolean(struct pmbus_data *data,
>         struct pmbus_boolean *boolean;
>         struct sensor_device_attribute *a;
>
> +       if (WARN((s1 && !s2) || (!s1 && s2), "Bad s1/s2 parameters\n"))
> +               return -EINVAL;
> +
>         boolean = devm_kzalloc(data->dev, sizeof(*boolean), GFP_KERNEL);
>         if (!boolean)
>                 return -ENOMEM;
> --
> 2.17.1
>

Thanks!

- Alex Qiu
diff mbox series

Patch

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 5113b5eca17a..6be08696189c 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -898,11 +898,7 @@  static int pmbus_get_boolean(struct i2c_client *client, struct pmbus_boolean *b,
 		pmbus_update_sensor_data(client, s2);
 
 	regval = status & mask;
-	if (!s1 && !s2) {
-		ret = !!regval;
-	} else if (!s1 || !s2) {
-		WARN(1, "Bad boolean descriptor %p: s1=%p, s2=%p\n", b, s1, s2);
-	} else {
+	if (s1 && s2) {
 		s64 v1, v2;
 
 		if (s1->data < 0) {
@@ -917,6 +913,8 @@  static int pmbus_get_boolean(struct i2c_client *client, struct pmbus_boolean *b,
 		v1 = pmbus_reg2data(data, s1);
 		v2 = pmbus_reg2data(data, s2);
 		ret = !!(regval && v1 >= v2);
+	} else {
+		ret = !!regval;
 	}
 unlock:
 	mutex_unlock(&data->update_lock);
@@ -1044,6 +1042,9 @@  static int pmbus_add_boolean(struct pmbus_data *data,
 	struct pmbus_boolean *boolean;
 	struct sensor_device_attribute *a;
 
+	if (WARN((s1 && !s2) || (!s1 && s2), "Bad s1/s2 parameters\n"))
+		return -EINVAL;
+
 	boolean = devm_kzalloc(data->dev, sizeof(*boolean), GFP_KERNEL);
 	if (!boolean)
 		return -ENOMEM;