Skip to contentSkip to conversation

usb: typec: tcpm: Fix a signedness bug in tcpm_fw_get_caps()

PATCH Superseded Dan Carpenter 3 participants 7 messages Sep 25, 2019 – Oct 1, 2019

Subsystems (4)
USB Kernel janitors USB Type-C class USB Type-C port controller drivers
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 usb: typec: tcpm: Fix a signedness bug in tcpm_fw_get_caps()

The "port->typec_caps.data" and "port->typec_caps.type" variables are enums and in this context GCC will treat them as an unsigned int so they can never be less than zero.

Fixes ae8a2ca8a221 ("usb: typec: Group all TCPCI/TCPM code together")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/usb/typec/tcpm/tcpm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/usb/typec/tcpm/tcpm.c +2 -2
index 96562744101c..d3b63e000ae2 100644
@@ -4410,7 +4410,7 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
 	ret = fwnode_property_read_string(fwnode, "data-role", &cap_str);
 	if (ret = 0) {
 		port->typec_caps.data = typec_find_port_data_role(cap_str);
-		if (port->typec_caps.data < 0)
+		if ((int)port->typec_caps.data < 0)
 			return -EINVAL;
 	}
 

@@ -4419,7 +4419,7 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
 		return ret;
 
 	port->typec_caps.type = typec_find_port_power_role(cap_str);
-	if (port->typec_caps.type < 0)
+	if ((int)port->typec_caps.type < 0)
 		return -EINVAL;
 	port->port_type = port->typec_caps.type;
Signature · 1 line
2.20.1
#1 lore raw
Quoted 15 lines · Dan Carpenter

The "port->typec_caps.data" and "port->typec_caps.type" variables are enums and in this context GCC will treat them as an unsigned int so they can never be less than zero.

Fixes ae8a2ca8a221 ("usb: typec: Group all TCPCI/TCPM code together")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/usb/typec/tcpm/tcpm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/usb/typec/tcpm/tcpm.c +0 -0
index 96562744101c..d3b63e000ae2 100644

drivers/usb/typec/tcpm/tcpm.c static int tcpm_fw_get_caps(struct tcpm_port *port, · line 4414

Quoted 6 lines
drivers/usb/typec/tcpm/tcpm.c +1 -1
@@ -4410,7 +4410,7 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
 	ret = fwnode_property_read_string(fwnode, "data-role", &cap_str);
 	if (ret = 0) {
 		port->typec_caps.data = typec_find_port_data_role(cap_str);
-		if (port->typec_caps.data < 0)
+		if ((int)port->typec_caps.data < 0)
 			return -EINVAL;

Doesn't that also cause a warning about overwriting error return codes ? I would prefer something like

		ret = typec_find_port_data_role(cap_str);
		if (ret < 0)
			return ret;
		port->typec_caps.data = ret;

Guenter

Quoted 14 lines
drivers/usb/typec/tcpm/tcpm.c +1 -1
 	}
 

@@ -4419,7 +4419,7 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
 		return ret;
 
 	port->typec_caps.type = typec_find_port_power_role(cap_str);
-	if (port->typec_caps.type < 0)
+	if ((int)port->typec_caps.type < 0)
 		return -EINVAL;
 	port->port_type = port->typec_caps.type;
Signature · 1 line
2.20.1
author
#2 lore raw
Quoted 11 lines · Guenter Roeck
Quoted 10 lines · Dan Carpenter

The "port->typec_caps.data" and "port->typec_caps.type" variables are enums and in this context GCC will treat them as an unsigned int so they can never be less than zero.

Fixes ae8a2ca8a221 ("usb: typec: Group all TCPCI/TCPM code together")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/usb/typec/tcpm/tcpm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 13 lines

drivers/usb/typec/tcpm/tcpm.c static int tcpm_fw_get_caps(struct tcpm_port *port, · line 4414

Quoted 11 lines
drivers/usb/typec/tcpm/tcpm.c +1 -1
index 96562744101c..d3b63e000ae2 100644
@@ -4410,7 +4410,7 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
 	ret = fwnode_property_read_string(fwnode, "data-role", &cap_str);
 	if (ret = 0) {
 		port->typec_caps.data = typec_find_port_data_role(cap_str);
-		if (port->typec_caps.data < 0)
+		if ((int)port->typec_caps.data < 0)
 			return -EINVAL;

Doesn't that also cause a warning about overwriting error return codes ?

I'm happy that you think there is a tool which generates warnings like that but it's just people manually complaining. :P

I'll resend though.

regards,
dan carpenter

#3 lore raw
Quoted 26 lines · Dan Carpenter
Quoted 11 lines · Guenter Roeck
Quoted 10 lines · Dan Carpenter

The "port->typec_caps.data" and "port->typec_caps.type" variables are enums and in this context GCC will treat them as an unsigned int so they can never be less than zero.

Fixes ae8a2ca8a221 ("usb: typec: Group all TCPCI/TCPM code together")
Signed-off-by Dan Carpenter <[email protected]>
---
  drivers/usb/typec/tcpm/tcpm.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 13 lines

drivers/usb/typec/tcpm/tcpm.c static int tcpm_fw_get_caps(struct tcpm_port *port, · line 4414

Quoted 11 lines
drivers/usb/typec/tcpm/tcpm.c +1 -1
index 96562744101c..d3b63e000ae2 100644
@@ -4410,7 +4410,7 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
  	ret = fwnode_property_read_string(fwnode, "data-role", &cap_str);
  	if (ret = 0) {
  		port->typec_caps.data = typec_find_port_data_role(cap_str);
-		if (port->typec_caps.data < 0)
+		if ((int)port->typec_caps.data < 0)
  			return -EINVAL;

Doesn't that also cause a warning about overwriting error return codes ?

Quoted 3 lines

I'm happy that you think there is a tool which generates warnings like that but it's just people manually complaining. :P

I am quite sure there is, or at least used to be - I have seen such warnings. Maybe it was removed.

Guenter

author
#4 lore raw

PATCH v2 1/1 usb: typec: tcpm: usb: typec: tcpm: Fix a signedness bug in tcpm_fw_get_caps()

In reply to Guenter Roeck

The "port->typec_caps.data" and "port->typec_caps.type" variables are enums and in this context GCC will treat them as an unsigned int so they can never be less than zero.

Fixes ae8a2ca8a221 ("usb: typec: Group all TCPCI/TCPM code together")
Signed-off-by Dan Carpenter <[email protected]>

---
v2: preserve the error code

 drivers/usb/typec/tcpm/tcpm.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)
drivers/usb/typec/tcpm/tcpm.c +8 -6
index 96562744101c..5f61d9977a15 100644
@@ -4409,18 +4409,20 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
 	/* USB data support is optional */
 	ret = fwnode_property_read_string(fwnode, "data-role", &cap_str);
 	if (ret = 0) {
-		port->typec_caps.data = typec_find_port_data_role(cap_str);
-		if (port->typec_caps.data < 0)
-			return -EINVAL;
+		ret = typec_find_port_data_role(cap_str);
+		if (ret < 0)
+			return ret;
+		port->typec_caps.data = ret;
 	}
 
 	ret = fwnode_property_read_string(fwnode, "power-role", &cap_str);
 	if (ret < 0)
 		return ret;
 
-	port->typec_caps.type = typec_find_port_power_role(cap_str);
-	if (port->typec_caps.type < 0)
-		return -EINVAL;
+	ret = typec_find_port_power_role(cap_str);
+	if (ret < 0)
+		return ret;
+	port->typec_caps.type = ret;
 	port->port_type = port->typec_caps.type;
 
 	if (port->port_type = TYPEC_PORT_SNK)
Signature · 1 line
2.20.1
maintainer Reviewed-by on v2 1/1
#5 lore raw

Re: [PATCH v2] usb: typec: tcpm: usb: typec: tcpm: Fix a signedness bug in tcpm_fw_get_caps()

Quoted 6 lines · Dan Carpenter

The "port->typec_caps.data" and "port->typec_caps.type" variables are enums and in this context GCC will treat them as an unsigned int so they can never be less than zero.

Fixes ae8a2ca8a221 ("usb: typec: Group all TCPCI/TCPM code together")
Signed-off-by Dan Carpenter <[email protected]>

Looks OK to me.

Reviewed-by Heikki Krogerus <[email protected]>
Quoted 39 lines

---
v2: preserve the error code

 drivers/usb/typec/tcpm/tcpm.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)
drivers/usb/typec/tcpm/tcpm.c +8 -6
index 96562744101c..5f61d9977a15 100644
@@ -4409,18 +4409,20 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
 	/* USB data support is optional */
 	ret = fwnode_property_read_string(fwnode, "data-role", &cap_str);
 	if (ret = 0) {
-		port->typec_caps.data = typec_find_port_data_role(cap_str);
-		if (port->typec_caps.data < 0)
-			return -EINVAL;
+		ret = typec_find_port_data_role(cap_str);
+		if (ret < 0)
+			return ret;
+		port->typec_caps.data = ret;
 	}
 
 	ret = fwnode_property_read_string(fwnode, "power-role", &cap_str);
 	if (ret < 0)
 		return ret;
 
-	port->typec_caps.type = typec_find_port_power_role(cap_str);
-	if (port->typec_caps.type < 0)
-		return -EINVAL;
+	ret = typec_find_port_power_role(cap_str);
+	if (ret < 0)
+		return ret;
+	port->typec_caps.type = ret;
 	port->port_type = port->typec_caps.type;
 
 	if (port->port_type = TYPEC_PORT_SNK)
Signature · 1 line
2.20.1

thanks,

Signature · 1 line
heikki
Reviewed-by on v2 1/1
#6 lore raw

Re: [PATCH v2] usb: typec: tcpm: usb: typec: tcpm: Fix a signedness bug in tcpm_fw_get_caps()

In reply to Dan Carpenter
Quoted 6 lines · Dan Carpenter

The "port->typec_caps.data" and "port->typec_caps.type" variables are enums and in this context GCC will treat them as an unsigned int so they can never be less than zero.

Fixes ae8a2ca8a221 ("usb: typec: Group all TCPCI/TCPM code together")
Signed-off-by Dan Carpenter <[email protected]>
Reviewed-by Guenter Roeck <[email protected]>
Quoted 38 lines

---
v2: preserve the error code

  drivers/usb/typec/tcpm/tcpm.c | 14 ++++++++------
  1 file changed, 8 insertions(+), 6 deletions(-)
drivers/usb/typec/tcpm/tcpm.c +8 -6
index 96562744101c..5f61d9977a15 100644
@@ -4409,18 +4409,20 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
  	/* USB data support is optional */
  	ret = fwnode_property_read_string(fwnode, "data-role", &cap_str);
  	if (ret = 0) {
-		port->typec_caps.data = typec_find_port_data_role(cap_str);
-		if (port->typec_caps.data < 0)
-			return -EINVAL;
+		ret = typec_find_port_data_role(cap_str);
+		if (ret < 0)
+			return ret;
+		port->typec_caps.data = ret;
  	}
 
  	ret = fwnode_property_read_string(fwnode, "power-role", &cap_str);
  	if (ret < 0)
  		return ret;
 
-	port->typec_caps.type = typec_find_port_power_role(cap_str);
-	if (port->typec_caps.type < 0)
-		return -EINVAL;
+	ret = typec_find_port_power_role(cap_str);
+	if (ret < 0)
+		return ret;
+	port->typec_caps.type = ret;
  	port->port_type = port->typec_caps.type;
 
  	if (port->port_type = TYPEC_PORT_SNK)