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.chen@xxxxxxxxxxxxx>
> > > > ---
> > > > 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.
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
