On Mon, Mar 18, 2013 at 11:33:24AM -0400, Alan Stern wrote:
> On Mon, 18 Mar 2013, Peter Chen 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.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 you. How about below version:
> >
> > drivers/usb/host/xhci-ring.c | 39 ++++++++++++++++++++-------------------
> > 1 files changed, 20 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index 8828754..6138af2 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -1588,25 +1588,18 @@ static void handle_port_status(struct xhci_hcd *xhci,
> > __le32 __iomem **port_array;
> > bool bogus_port_status = false;
> >
> > - /* 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");
> > - xhci->error_bitmask |= 1 << 8;
> > - }
> > - port_id = GET_PORT_ID(le32_to_cpu(event->generic.field[0]));
> > - xhci_dbg(xhci, "Port Status Change Event for port %d\n", port_id);
> > -
> > - max_ports = HCS_MAX_PORTS(xhci->hcs_params1);
> > - if ((port_id <= 0) || (port_id > max_ports)) {
> > - xhci_warn(xhci, "Invalid port id %d\n", port_id);
> > - bogus_port_status = true;
> > - goto cleanup;
> > - }
> > -
> > /* Figure out which usb_hcd this port is attached to:
> > * is it a USB 3.0 port or a USB 2.0/1.1 port?
> > */
> > + port_id = GET_PORT_ID(le32_to_cpu(event->generic.field[0]));
> > + xhci_dbg(xhci, "Port Status Change Event for port %d\n", port_id);
> > +
> > major_revision = xhci->port_array[port_id - 1];
> > + /* Find the right roothub. */
> > + hcd = xhci_to_hcd(xhci);
> > + if ((major_revision == 0x03) != (hcd->speed == HCD_USB3))
> > + hcd = xhci->shared_hcd;
> > +
> > if (major_revision == 0) {
> > xhci_warn(xhci, "Event for port %u not in "
> > "Extended Capabilities, ignoring.\n",
> > @@ -1621,6 +1614,18 @@ static void handle_port_status(struct xhci_hcd *xhci,
> > bogus_port_status = true;
> > goto cleanup;
> > }
> > + /* 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");
> > + xhci->error_bitmask |= 1 << 8;
> > + }
> > +
> > + max_ports = HCS_MAX_PORTS(xhci->hcs_params1);
> > + if ((port_id <= 0) || (port_id > max_ports)) {
> > + xhci_warn(xhci, "Invalid port id %d\n", port_id);
> > + bogus_port_status = true;
> > + goto cleanup;
> > + }
> >
> > /*
> > * Hardware port IDs reported by a Port Status Change Event include USB
> > @@ -1629,10 +1634,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;
> > bus_state = &xhci->bus_state[hcd_index(hcd)];
> > if (hcd->speed == HCD_USB3)
> > port_array = xhci->usb3_ports;
>
> I'm not very familiar with this code, but it looks like you have
> combined two patches into one.
>
> If all you want to do is fix the compiler warning, why not simply move
> the four lines of code from where they are to the start of the routine?
The driver has to use the port_id to figure out what speed the port is
set up for, so that the driver can pick the right hcd. That's why this
patch revision is a bit more complicated than the previous patch.
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
