Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755417AbdIGOUT (ORCPT ); Thu, 7 Sep 2017 10:20:19 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:46670 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1755307AbdIGOUS (ORCPT ); Thu, 7 Sep 2017 10:20:18 -0400 Date: Thu, 7 Sep 2017 10:20:17 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Arnd Bergmann cc: Felipe Balbi , Greg Kroah-Hartman , Tatyana Brokhman , Peter Chen , , Subject: Re: [PATCH] [v2] usb: gadget: dummy: fix nonsensical comparisons In-Reply-To: <20170907141559.3739432-1-arnd@arndb.de> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2413 Lines: 54 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 > Cc: Felipe Balbi > Signed-off-by: Arnd Bergmann > --- > 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