| Message ID | 20201023112412.GD282278@mwanda (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 063a3d67693ecde7f24ca00d4a131883edab1e73 |
| Headers | show |
| Series | [1/2] usb: typec: stusb160x: fix an IS_ERR() vs NULL check in probe | expand |
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
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
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 >
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
this is much better to read
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;
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(-)