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?
Alan Stern
--
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
