usb: usb251xb: Remove unnecessary comparison of unsigned integer with >= 0

PATCH In review Gustavo A. R. Silva 3 participants 5 messages Mar 18, 2019 – Mar 19, 2019

Subsystems (2)

USB Microchip USB251XB driver

lore

Share & tools

lore

Gustavo A. R. Silva

author

2019-03-18 14:50 UTC #0 lore raw

PATCH 1/1 usb: usb251xb: Remove unnecessary comparison of unsigned integer with >= 0

There is no need to compare *port* with >= 0 because such comparison of an unsigned value is always true.

Fix this by removing such comparison.

Addresses-Coverity-ID: 1443949 ("Unsigned compared against 0")

Fixes 02a50b875046 ("usb: usb251xb: add usb data lane port swap feature")

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/usb/misc/usb251xb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/usb/misc/usb251xb.c +1 -1

index 4d72b7d1d383..2c8e2cad7e10 100644
@@ -547,7 +547,7 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	 */
 	hub->port_swap = USB251XB_DEF_PORT_SWAP;
 	of_property_for_each_u32(np, "swap-dx-lanes", prop, p, port) {
-		if ((port >= 0) && (port <= data->port_cnt))
+		if (port <= data->port_cnt)
 			hub->port_swap |= BIT(port);
 	}

Marco Felsch

2019-03-18 15:34 UTC #1 lore raw

Hi Silva,

Quoted 2 lines · Gustavo A. R. Silva

There is no need to compare *port* with >= 0 because such comparison of an unsigned value is always true.

Absolutly.. It seems that it was an copy'n'paste failure. Thanks for fixing that.

Regards,
Marco

Quoted 26 lines

Fix this by removing such comparison.
Addresses-Coverity-ID: 1443949 ("Unsigned compared against 0")
Fixes 02a50b875046 ("usb: usb251xb: add usb data lane port swap feature")
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/usb/misc/usb251xb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/misc/usb251xb.c +1 -1
index 4d72b7d1d383..2c8e2cad7e10 100644
@@ -547,7 +547,7 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	 */
 	hub->port_swap = USB251XB_DEF_PORT_SWAP;
 	of_property_for_each_u32(np, "swap-dx-lanes", prop, p, port) {
-		if ((port >= 0) && (port <= data->port_cnt))
+		if (port <= data->port_cnt)
 			hub->port_swap |= BIT(port);
 	}
 
Signature · 1 line
2.21.0

Richard Leitner

maintainer Reviewed-by

2019-03-18 15:46 UTC #2 lore raw

Hi Marco, Hi Gustavo,
thanks for the fix, please feel free to add

Reviewed-by Richard Leitner <[email protected]>

Quoted 8 lines · Marco Felsch

Hi Silva,
Quoted 2 lines · Gustavo A. R. Silva
There is no need to compare *port* with >= 0 because such comparison of an unsigned value is always true.
Absolutly.. It seems that it was an copy'n'paste failure. Thanks for fixing that.

A small note on further improvement from my side:

According the datasheet, when you set swap-dx-lanes to 0 (-> setting
port to 0 -> setting bit 0 of PRTSP) " the upstream port DP/DM is
swapped".

IMHO this is not straight-forward, so maybe we want to additionally

a) mention it somewhere in the documentation
b) check on port >= 1
c) something else?

Any opinions on that from your side? ... I'd prefer "a"...

regards;Richard.L

Quoted 31 lines

Regards,
Marco
Quoted 26 lines
Fix this by removing such comparison.
Addresses-Coverity-ID: 1443949 ("Unsigned compared against 0")
Fixes 02a50b875046 ("usb: usb251xb: add usb data lane port swap feature")
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
  drivers/usb/misc/usb251xb.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/misc/usb251xb.c +1 -1
index 4d72b7d1d383..2c8e2cad7e10 100644
@@ -547,7 +547,7 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
  	 */
  	hub->port_swap = USB251XB_DEF_PORT_SWAP;
  	of_property_for_each_u32(np, "swap-dx-lanes", prop, p, port) {
-		if ((port >= 0) && (port <= data->port_cnt))
+		if (port <= data->port_cnt)
  			hub->port_swap |= BIT(port);
  	}
 
Signature · 1 line
2.21.0

Marco Felsch

2019-03-18 16:48 UTC #3 lore raw

Hi Richard,

Quoted 12 lines · Richard Leitner

Hi Marco, Hi Gustavo,
thanks for the fix, please feel free to add
Reviewed-by Richard Leitner <[email protected]>
Quoted 6 lines · Marco Felsch
Hi Silva,
Quoted 2 lines · Gustavo A. R. Silva
There is no need to compare *port* with >= 0 because such comparison of an unsigned value is always true.

Quoted 7 lines

Quoted 2 lines
Absolutly.. It seems that it was an copy'n'paste failure. Thanks for fixing that.
A small note on further improvement from my side:
According the datasheet, when you set swap-dx-lanes to 0 (-> setting port to
0 -> setting bit 0 of PRTSP) " the upstream port DP/DM is swapped".

Yes I know that's not intuitve and may confuses the users.

Quoted 3 lines

IMHO this is not straight-forward, so maybe we want to additionally
a) mention it somewhere in the documentation

IMHO this is the correct place.

Quoted 1 line

b) check on port >= 1

I think this isn't doable since we shouldn't break the existing dt.

Regards,
Marco

Quoted 37 lines

c) something else?
Any opinions on that from your side? ... I'd prefer "a"...
regards;Richard.L
Quoted 30 lines
Regards,
Marco
Quoted 25 lines
Fix this by removing such comparison.
Addresses-Coverity-ID: 1443949 ("Unsigned compared against 0")
Fixes 02a50b875046 ("usb: usb251xb: add usb data lane port swap feature")
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
  drivers/usb/misc/usb251xb.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/misc/usb251xb.c +1 -1
index 4d72b7d1d383..2c8e2cad7e10 100644
@@ -547,7 +547,7 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
  	 */
  	hub->port_swap = USB251XB_DEF_PORT_SWAP;
  	of_property_for_each_u32(np, "swap-dx-lanes", prop, p, port) {
-		if ((port >= 0) && (port <= data->port_cnt))
+		if (port <= data->port_cnt)
  			hub->port_swap |= BIT(port);
  	}
Signature · 1 line
2.21.0

Richard Leitner

maintainer

2019-03-19 09:33 UTC #4 lore raw

Quoted 10 lines · Marco Felsch

Quoted 4 lines · Richard Leitner
A small note on further improvement from my side:
According the datasheet, when you set swap-dx-lanes to 0 (-> setting port to
0 -> setting bit 0 of PRTSP) " the upstream port DP/DM is swapped".
Yes I know that's not intuitve and may confuses the users.
Quoted 2 lines
IMHO this is not straight-forward, so maybe we want to additionally

Quoted 7 lines

Quoted 1 line
a) mention it somewhere in the documentation
IMHO this is the correct place.
Quoted 1 line
b) check on port >= 1
I think this isn't doable since we shouldn't break the existing dt.

ACK.

@Marco/Gustavo: does one of you wants to provide a patch for this?

Quoted 7 lines

Regards,
Marco
Quoted 3 lines
c) something else?
Any opinions on that from your side? ... I'd prefer "a"...

regards;Richard.L