[v2] usb: gadget: dummy: fix nonsensical comparisons

Message ID 20170907141559.3739432-1-arnd@arndb.de
State Accepted
Commit 7661ca09b2ff98f48693f431bb01fed62830e433
Headers show
Series
  • [v2] usb: gadget: dummy: fix nonsensical comparisons
Related show

Commit Message

Arnd Bergmann Sept. 7, 2017, 2:14 p.m.
gcc-8 points out two comparisons that are clearly bogus
and almost certainly not what the author intended to write:

drivers/usb/gadget/udc/dummy_hcd.c: In function 'set_link_state_by_speed':
drivers/usb/gadget/udc/dummy_hcd.c:379:31: error: bitwise comparison always evaluates to false [-Werror=tautological-compare]
         USB_PORT_STAT_ENABLE) == 1 &&
                               ^~
drivers/usb/gadget/udc/dummy_hcd.c:381:25: error: bitwise comparison always evaluates to false [-Werror=tautological-compare]
      USB_SS_PORT_LS_U0) == 1 &&
                         ^~

I looked at the code for a bit and came up with a change that makes
it look like what the author probably meant here. This makes it
look reasonable to me and to gcc, shutting up the warning.

It does of course change behavior as the two conditions are actually
evaluated rather than being hardcoded to false, and I have made no
attempt at verifying that the changed logic makes sense in the context
of a USB HCD, so that part needs to be reviewed carefully.

Fixes: 1cd8fd2887e1 ("usb: gadget: dummy_hcd: add SuperSpeed support")
Cc: Tatyana Brokhman <tlinder@codeaurora.org>
Cc: Felipe Balbi <balbi@kernel.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
v2: simplify the expression as suggested by Alan Stern
---
 drivers/usb/gadget/udc/dummy_hcd.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

-- 
2.9.0

Comments

Alan Stern Sept. 7, 2017, 2:20 p.m. | #1
On Thu, 7 Sep 2017, Arnd Bergmann wrote:

> gcc-8 points out two comparisons that are clearly bogus

> and almost certainly not what the author intended to write:

> 

> drivers/usb/gadget/udc/dummy_hcd.c: In function 'set_link_state_by_speed':

> drivers/usb/gadget/udc/dummy_hcd.c:379:31: error: bitwise comparison always evaluates to false [-Werror=tautological-compare]

>          USB_PORT_STAT_ENABLE) == 1 &&

>                                ^~

> drivers/usb/gadget/udc/dummy_hcd.c:381:25: error: bitwise comparison always evaluates to false [-Werror=tautological-compare]

>       USB_SS_PORT_LS_U0) == 1 &&

>                          ^~

> 

> I looked at the code for a bit and came up with a change that makes

> it look like what the author probably meant here. This makes it

> look reasonable to me and to gcc, shutting up the warning.

> 

> It does of course change behavior as the two conditions are actually

> evaluated rather than being hardcoded to false, and I have made no

> attempt at verifying that the changed logic makes sense in the context

> of a USB HCD, so that part needs to be reviewed carefully.

> 

> Fixes: 1cd8fd2887e1 ("usb: gadget: dummy_hcd: add SuperSpeed support")

> Cc: Tatyana Brokhman <tlinder@codeaurora.org>

> Cc: Felipe Balbi <balbi@kernel.org>

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---

> v2: simplify the expression as suggested by Alan Stern

> ---

>  drivers/usb/gadget/udc/dummy_hcd.c | 9 ++++-----

>  1 file changed, 4 insertions(+), 5 deletions(-)

> 

> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c

> index a030d7923d7d..b1e21b3be6e1 100644

> --- a/drivers/usb/gadget/udc/dummy_hcd.c

> +++ b/drivers/usb/gadget/udc/dummy_hcd.c

> @@ -375,11 +375,10 @@ static void set_link_state_by_speed(struct dummy_hcd *dum_hcd)

>  			     USB_PORT_STAT_CONNECTION) == 0)

>  				dum_hcd->port_status |=

>  					(USB_PORT_STAT_C_CONNECTION << 16);

> -			if ((dum_hcd->port_status &

> -			     USB_PORT_STAT_ENABLE) == 1 &&

> -				(dum_hcd->port_status &

> -				 USB_SS_PORT_LS_U0) == 1 &&

> -				dum_hcd->rh_state != DUMMY_RH_SUSPENDED)

> +			if ((dum_hcd->port_status & USB_PORT_STAT_ENABLE) &&

> +			    (dum_hcd->port_status &

> +			     USB_PORT_STAT_LINK_STATE) == USB_SS_PORT_LS_U0 &&

> +			    dum_hcd->rh_state != DUMMY_RH_SUSPENDED)

>  				dum_hcd->active = 1;

>  		}

>  	} else {


Acked-by: Alan Stern <stern@rowland.harvard.edu>

Patch

diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
index a030d7923d7d..b1e21b3be6e1 100644
--- a/drivers/usb/gadget/udc/dummy_hcd.c
+++ b/drivers/usb/gadget/udc/dummy_hcd.c
@@ -375,11 +375,10 @@  static void set_link_state_by_speed(struct dummy_hcd *dum_hcd)
 			     USB_PORT_STAT_CONNECTION) == 0)
 				dum_hcd->port_status |=
 					(USB_PORT_STAT_C_CONNECTION << 16);
-			if ((dum_hcd->port_status &
-			     USB_PORT_STAT_ENABLE) == 1 &&
-				(dum_hcd->port_status &
-				 USB_SS_PORT_LS_U0) == 1 &&
-				dum_hcd->rh_state != DUMMY_RH_SUSPENDED)
+			if ((dum_hcd->port_status & USB_PORT_STAT_ENABLE) &&
+			    (dum_hcd->port_status &
+			     USB_PORT_STAT_LINK_STATE) == USB_SS_PORT_LS_U0 &&
+			    dum_hcd->rh_state != DUMMY_RH_SUSPENDED)
 				dum_hcd->active = 1;
 		}
 	} else {