diff mbox series

[2/2] usb: typec: stusb160x: fix some signedness bugs

Message ID 20201023112412.GD282278@mwanda
State New
Headers show
Series [1/2] usb: typec: stusb160x: fix an IS_ERR() vs NULL check in probe | expand

Commit Message

Dan Carpenter Oct. 23, 2020, 11:24 a.m. UTC
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 <dan.carpenter@oracle.com>
---
 drivers/usb/typec/stusb160x.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Amelie DELAUNAY Oct. 28, 2020, 12:26 p.m. UTC | #1
Hi Dan,

On 10/23/20 1:24 PM, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
> ---
>   drivers/usb/typec/stusb160x.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/stusb160x.c b/drivers/usb/typec/stusb160x.c
> index f7369e371dd4..da7f1957bcb3 100644
> --- a/drivers/usb/typec/stusb160x.c
> +++ b/drivers/usb/typec/stusb160x.c
> @@ -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;

	}

> @@ -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;
> 

	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
Dan Carpenter Oct. 28, 2020, 1:23 p.m. UTC | #2
On Wed, Oct 28, 2020 at 01:26:16PM +0100, Amelie DELAUNAY wrote:
> > @@ -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;
> > 
> 
> 	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.

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

regards,
dan carpenter
Amelie DELAUNAY Oct. 28, 2020, 2:38 p.m. UTC | #3
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

On 10/28/20 2:23 PM, Dan Carpenter wrote:
> On Wed, Oct 28, 2020 at 01:26:16PM +0100, Amelie DELAUNAY wrote:

>>> @@ -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;

>>>

>>

>> 	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.

> 

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

> be too late...

> 

> regards,

> dan carpenter

>
Greg KH Oct. 28, 2020, 3:40 p.m. UTC | #4
On Wed, Oct 28, 2020 at 03:38:54PM +0100, Amelie DELAUNAY wrote:
> 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
diff mbox series

Patch

diff --git a/drivers/usb/typec/stusb160x.c b/drivers/usb/typec/stusb160x.c
index f7369e371dd4..da7f1957bcb3 100644
--- a/drivers/usb/typec/stusb160x.c
+++ b/drivers/usb/typec/stusb160x.c
@@ -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;