Skip to contentSkip to conversation

usb: typec: stusb160x: fix some signedness bugs

PATCH Applied Dan Carpenter 4 participants 6 messages Oct 23, 2020 – Oct 29, 2020

Subsystems (3)
USB Kernel janitors USB Type-C class

v1 of 2 Applied Latest is v2 →

lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 2/2 usb: typec: stusb160x: fix some signedness bugs

These variables are enums but in this situation GCC will treat them as unsigned so the conditions are never true.

Fixes da0cb6310094 ("usb: typec: add support for STUSB160x Type-C controller family")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/usb/typec/stusb160x.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/usb/typec/stusb160x.c +4 -3
index f7369e371dd4..da7f1957bcb3 100644
@@ -545,7 +545,7 @@ static int stusb160x_get_fw_caps(struct stusb160x *chip,
 	ret = fwnode_property_read_string(fwnode, "power-role", &cap_str);
 	if (!ret) {
 		chip->port_type = typec_find_port_power_role(cap_str);
-		if (chip->port_type < 0) {
+		if ((int)chip->port_type < 0) {
 			ret = chip->port_type;
 			return ret;
 		}

@@ -567,9 +567,10 @@ static int stusb160x_get_fw_caps(struct stusb160x *chip,
 	if (!ret) {
 		chip->pwr_opmode = typec_find_pwr_opmode(cap_str);
 		/* Power delivery not yet supported */
-		if (chip->pwr_opmode < 0 ||
+		if ((int)chip->pwr_opmode < 0 ||
 		    chip->pwr_opmode = TYPEC_PWR_MODE_PD) {
-			ret = chip->pwr_opmode < 0 ? chip->pwr_opmode : -EINVAL;
+			ret = (int)chip->pwr_opmode < 0 ? chip->pwr_opmode :
+							  -EINVAL;
 			dev_err(chip->dev, "bad power operation mode: %d\n",
 				chip->pwr_opmode);
 			return ret;
Signature · 1 line
2.28.0
#1 lore raw

Hi Dan,

Quoted 14 lines · Dan Carpenter

These variables are enums but in this situation GCC will treat them as unsigned so the conditions are never true.

Fixes da0cb6310094 ("usb: typec: add support for STUSB160x Type-C controller family")
Signed-off-by Dan Carpenter <[email protected]>
---
  drivers/usb/typec/stusb160x.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)
drivers/usb/typec/stusb160x.c +0 -0
index f7369e371dd4..da7f1957bcb3 100644

drivers/usb/typec/stusb160x.c static int stusb160x_get_fw_caps(struct stusb160x *chip, · line 551

Quoted 8 lines
drivers/usb/typec/stusb160x.c +1 -1
@@ -545,7 +545,7 @@ static int stusb160x_get_fw_caps(struct stusb160x *chip,
  	ret = fwnode_property_read_string(fwnode, "power-role", &cap_str);
  	if (!ret) {
  		chip->port_type = typec_find_port_power_role(cap_str);
-		if (chip->port_type < 0) {
+		if ((int)chip->port_type < 0) {
  			ret = chip->port_type;
  			return ret;
  		}

I was preparing a patch for this one, and it uses the ret instead of the cast:

	ret = fwnode_property_read_string(fwnode, "power-role", &cap_str);
	if (!ret) {
		ret = typec_find_port_power_role(cap_str);
		if (ret < 0)
			return ret;
		chip->port_type = ret;

	}
Quoted 4 lines
drivers/usb/typec/stusb160x.c +0 -0
@@ -567,9 +567,10 @@ static int stusb160x_get_fw_caps(struct stusb160x *chip,
  	if (!ret) {
  		chip->pwr_opmode = typec_find_pwr_opmode(cap_str);

drivers/usb/typec/stusb160x.c static int stusb160x_get_fw_caps(struct stusb160x *chip, · line 576

Quoted 10 lines
drivers/usb/typec/stusb160x.c +3 -2
  		/* Power delivery not yet supported */
-		if (chip->pwr_opmode < 0 ||
+		if ((int)chip->pwr_opmode < 0 ||
  		    chip->pwr_opmode = TYPEC_PWR_MODE_PD) {
-			ret = chip->pwr_opmode < 0 ? chip->pwr_opmode : -EINVAL;
+			ret = (int)chip->pwr_opmode < 0 ? chip->pwr_opmode :
+							  -EINVAL;
  			dev_err(chip->dev, "bad power operation mode: %d\n",
  				chip->pwr_opmode);
  			return ret;

	if (!ret) {
		ret = typec_find_pwr_opmode(cap_str);
		/* Power delivery not yet supported */
		if (ret < 0 || ret = TYPEC_PWR_MODE_PD) {
			dev_err(chip->dev, "bad power operation mode: %d\n", ret);
			return -EINVAL;
		}
		chip->pwr_opmode = ret;
	}

So, which fix sounds better ? IMHO using ret make the code more readable.

Regards,
Amelie

author
#2 lore raw
Quoted 26 lines · Amelie DELAUNAY
Quoted 4 lines
drivers/usb/typec/stusb160x.c +0 -0
@@ -567,9 +567,10 @@ static int stusb160x_get_fw_caps(struct stusb160x *chip,
  	if (!ret) {
  		chip->pwr_opmode = typec_find_pwr_opmode(cap_str);

drivers/usb/typec/stusb160x.c static int stusb160x_get_fw_caps(struct stusb160x *chip, · line 576

Quoted 10 lines
drivers/usb/typec/stusb160x.c +3 -2
  		/* Power delivery not yet supported */
-		if (chip->pwr_opmode < 0 ||
+		if ((int)chip->pwr_opmode < 0 ||
  		    chip->pwr_opmode = TYPEC_PWR_MODE_PD) {
-			ret = chip->pwr_opmode < 0 ? chip->pwr_opmode : -EINVAL;
+			ret = (int)chip->pwr_opmode < 0 ? chip->pwr_opmode :
+							  -EINVAL;
  			dev_err(chip->dev, "bad power operation mode: %d\n",
  				chip->pwr_opmode);
  			return ret;

	if (!ret) {
		ret = typec_find_pwr_opmode(cap_str);
		/* Power delivery not yet supported */
		if (ret < 0 || ret = TYPEC_PWR_MODE_PD) {
			dev_err(chip->dev, "bad power operation mode: %d\n", ret);
			return -EINVAL;
		}
		chip->pwr_opmode = ret;
	}
Quoted 1 line

So, which fix sounds better ? IMHO using ret make the code more readable.

Yeah. Your patch is nicer, but Greg *just* merged mine so it might be too late...

regards,
dan carpenter

#3 lore raw

Hi Greg,

I know I'm a bit late for the review, but is it still possible to fixup this patch in your usb-linus branch?

Regards,
Amelie

Quoted 35 lines · Dan Carpenter
Quoted 26 lines · Amelie DELAUNAY
Quoted 4 lines
drivers/usb/typec/stusb160x.c +0 -0
@@ -567,9 +567,10 @@ static int stusb160x_get_fw_caps(struct stusb160x *chip,
   	if (!ret) {
   		chip->pwr_opmode = typec_find_pwr_opmode(cap_str);

drivers/usb/typec/stusb160x.c static int stusb160x_get_fw_caps(struct stusb160x *chip, · line 576

Quoted 10 lines
drivers/usb/typec/stusb160x.c +3 -2
   		/* Power delivery not yet supported */
-		if (chip->pwr_opmode < 0 ||
+		if ((int)chip->pwr_opmode < 0 ||
   		    chip->pwr_opmode = TYPEC_PWR_MODE_PD) {
-			ret = chip->pwr_opmode < 0 ? chip->pwr_opmode : -EINVAL;
+			ret = (int)chip->pwr_opmode < 0 ? chip->pwr_opmode :
+							  -EINVAL;
   			dev_err(chip->dev, "bad power operation mode: %d\n",
   				chip->pwr_opmode);
   			return ret;

	if (!ret) {
		ret = typec_find_pwr_opmode(cap_str);
		/* Power delivery not yet supported */
		if (ret < 0 || ret = TYPEC_PWR_MODE_PD) {
			dev_err(chip->dev, "bad power operation mode: %d\n", ret);
			return -EINVAL;
		}
		chip->pwr_opmode = ret;
	}
Quoted 1 line

So, which fix sounds better ? IMHO using ret make the code more readable.

Yeah. Your patch is nicer, but Greg *just* merged mine so it might be too late...

regards,
dan carpenter

maintainer
#4 lore raw
Quoted 4 lines · Amelie DELAUNAY

Hi Greg,

I know I'm a bit late for the review, but is it still possible to fixup this patch in your usb-linus branch?

Send me a patch on top of the existing one and I will be glad to review and take it if needed.

thanks,

greg k-h

#5 lore raw
In reply to Amelie DELAUNAY

this is much better to read
________________________________________
Von: Amelie DELAUNAY [[email protected]]
Gesendet: Mittwoch, 28. Oktober 2020 13:26
An: Dan Carpenter; Heikki Krogerus
Cc: Greg Kroah-Hartman; [email protected]; [email protected]
Betreff: Re: [PATCH 2/2] usb: typec: stusb160x: fix some signedness bugs

Hi Dan,

Quoted 14 lines · Dan Carpenter

These variables are enums but in this situation GCC will treat them as unsigned so the conditions are never true.

Fixes da0cb6310094 ("usb: typec: add support for STUSB160x Type-C controller family")
Signed-off-by Dan Carpenter <[email protected]>
---
  drivers/usb/typec/stusb160x.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)
drivers/usb/typec/stusb160x.c +0 -0
index f7369e371dd4..da7f1957bcb3 100644

drivers/usb/typec/stusb160x.c static int stusb160x_get_fw_caps(struct stusb160x *chip, · line 551

Quoted 8 lines
drivers/usb/typec/stusb160x.c +1 -1
@@ -545,7 +545,7 @@ static int stusb160x_get_fw_caps(struct stusb160x *chip,
      ret = fwnode_property_read_string(fwnode, "power-role", &cap_str);
      if (!ret) {
              chip->port_type = typec_find_port_power_role(cap_str);
-             if (chip->port_type < 0) {
+             if ((int)chip->port_type < 0) {
                      ret = chip->port_type;
                      return ret;
              }

I was preparing a patch for this one, and it uses the ret instead of the cast:

        ret = fwnode_property_read_string(fwnode, "power-role", &cap_str);
        if (!ret) {
                ret = typec_find_port_power_role(cap_str);
                if (ret < 0)
                        return ret;
                chip->port_type = ret;

        }
Quoted 4 lines
drivers/usb/typec/stusb160x.c +0 -0
@@ -567,9 +567,10 @@ static int stusb160x_get_fw_caps(struct stusb160x *chip,
      if (!ret) {
              chip->pwr_opmode = typec_find_pwr_opmode(cap_str);

drivers/usb/typec/stusb160x.c static int stusb160x_get_fw_caps(struct stusb160x *chip, · line 576

Quoted 10 lines
drivers/usb/typec/stusb160x.c +3 -2
              /* Power delivery not yet supported */
-             if (chip->pwr_opmode < 0 ||
+             if ((int)chip->pwr_opmode < 0 ||
                  chip->pwr_opmode == TYPEC_PWR_MODE_PD) {
-                     ret = chip->pwr_opmode < 0 ? chip->pwr_opmode : -EINVAL;
+                     ret = (int)chip->pwr_opmode < 0 ? chip->pwr_opmode :
+                                                       -EINVAL;
                      dev_err(chip->dev, "bad power operation mode: %d\n",
                              chip->pwr_opmode);
                      return ret;

        if (!ret) {
                ret = typec_find_pwr_opmode(cap_str);
                /* Power delivery not yet supported */
                if (ret < 0 || ret == TYPEC_PWR_MODE_PD) {
                        dev_err(chip->dev, "bad power operation mode: %d\n", ret);
                        return -EINVAL;
                }
                chip->pwr_opmode = ret;
        }

So, which fix sounds better ? IMHO using ret make the code more readable.

Regards,
Amelie