On Fri, Mar 15, 2013 at 10:18:57AM -0700, Sarah Sharp wrote:
> On Fri, Mar 15, 2013 at 10:47:50AM -0400, Alan Stern wrote:
> > On Fri, 15 Mar 2013, Peter Chen wrote:
> > 
> > > On Thu, Mar 14, 2013 at 10:59:45AM -0400, Alan Stern wrote:
> > > > On Thu, 14 Mar 2013, Peter Chen wrote:
> > > > 
> > > > > /home/b29397/work/code/git/linus/linux-2.6/drivers/usb/host/xhci-ring.c:
> > > > >  In function ‘handle_port_status’:
> > > > > /home/b29397/work/code/git/linus/linux-2.6/drivers/usb/host/xhci-ring.c:1580:
> > > > >  warning: ‘hcd’ may be used uninitialized in this function
> > > > > 
> > > > > Signed-off-by: Peter Chen <peter.c...@freescale.com>
> > > > > ---
> > > > >  drivers/usb/host/xhci-ring.c |    4 ++--
> > > > >  1 files changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/usb/host/xhci-ring.c 
> > > > > b/drivers/usb/host/xhci-ring.c
> > > > > index 8828754..17dace0 100644
> > > > > --- a/drivers/usb/host/xhci-ring.c
> > > > > +++ b/drivers/usb/host/xhci-ring.c
> > > > > @@ -1588,6 +1588,8 @@ static void handle_port_status(struct xhci_hcd 
> > > > > *xhci,
> > > > >       __le32 __iomem **port_array;
> > > > >       bool bogus_port_status = false;
> > > > >  
> > > > > +     /* Find the right roothub. */
> > > > > +     hcd = xhci_to_hcd(xhci);
> > > > >       /* Port status change events always have a successful 
> > > > > completion code */
> > > > >       if (GET_COMP_CODE(le32_to_cpu(event->generic.field[2])) != 
> > > > > COMP_SUCCESS) {
> > > > >               xhci_warn(xhci, "WARN: xHC returned failed port status 
> > > > > event\n");
> > > > > @@ -1629,8 +1631,6 @@ static void handle_port_status(struct xhci_hcd 
> > > > > *xhci,
> > > > >        * into the index into the ports on the correct split roothub, 
> > > > > and the
> > > > >        * correct bus_state structure.
> > > > >        */
> > > > > -     /* Find the right roothub. */
> > > > > -     hcd = xhci_to_hcd(xhci);
> > > > >       if ((major_revision == 0x03) != (hcd->speed == HCD_USB3))
> > > > >               hcd = xhci->shared_hcd;
> > > > 
> > > > You forgot to move these last two lines along with the first two.
> > > 
> > > No, major_revision is got above it.
> > > 
> > > In fact, the old logic is no problem due to flag bogus_port_status.
> > 
> > This doesn't matter.  Your change makes the code more fragile and
> > harder to understand, because in your version "hcd" is initialized in
> > two separate places.  The initialization should remain a single block
> > of code.
> > 
> > "Works okay" isn't good enough.  A lot of programmers seem to have
> > trouble absorbing this concept.  The code needs to be _clear_, because
> > it needs to continue to work correctly after other people make changes.  
> > If those other people can't understand it then they are likely to break
> > the code.
> 
> I agree with Alan.  The code will be more clear if the check for the
> shared host controller is moved up.  It especially doesn't make any
> sense to move the comment "Find the right roothub" without the following
> conditional to actually find the right roothub.
> 
> And which compiler/kernel were you building this on?  I don't see this
> warning when I compile 3.9-rc1 from Greg's usb-linus branch.
> 

It may because I use make allmodconfig to build kernel (v3.9 greg's usb-next)

try below commands to reproduce it:

$ make allmodconfig
$ make drivers/usb/

I am not familiar with i386/686 compiler:

$ gcc --version
gcc (Ubuntu 4.4.3-4ubuntu5.1) 4.4.3
Copyright (C) 2009 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ cc --version
cc (Ubuntu 4.4.3-4ubuntu5.1) 4.4.3
Copyright (C) 2009 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

-- 

Best Regards,
Peter Chen

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to