On Fri, Apr 05, 2013 at 06:51:21AM +0400, Vladimir Murzin wrote: > On Thu, Apr 04, 2013 at 12:29:25PM -0700, Sarah Sharp wrote: > > Someone else is already working on a patch to fix part of this: > > > > http://marc.info/?l=linux-usb&m=136509667003716&w=2 > > > > Please coordinate with them. > > > > Also, two people trying to solve the same simple bug and using almost > > exactly the same commit messages seems like too much of a coincidence. > > Are you working on the same project or did you both happen to run the > > same static code checking tool at the same time, or ? > > This problem was met by chance while trying buggy out-of-tree USB > stuff. So, we wanted to bypass xhci initialization gracefully instead > of being stuck with null pointer dereference while list traversing in > xhci_mem_cleanup(). It is why patch addressed not only initialization > of cancel_cmd_list and lpm_failed_devs, but bw tables too. With > proposed patch normal boot sequence is progressing. > > I've never heard of Sergio's patch, and never met him before :) > > What about picking up Sergio's patch and bw table part of this patch > as separate commits? I'm ok to take your patch instead too, since is already addressing what I'm doing. It's basically, my patch + similar change for bw tables, so it's an improvement :) Sarah, I'll propose to ignore my patch, and take this one instead. Regards, Sergio > > Best wishes > Vladimir Murzin > > > > Sarah Sharp > > > > On Thu, Apr 04, 2013 at 08:31:26PM +0400, Vladimir Murzin wrote: > > > Commits > > > 9574323 xHCI: test USB2 software LPM > > > b92cc66 xHCI: add aborting command ring function > > > introduce useful functions which involves lists manipulations. > > > > > > If for whatever reason we fall into fail path in xhci_mem_init() we > > > may access the lists in xhci_mem_cleanup() before they get > > > initialized. > > > > > > The same init-fail-cleanup case is applicable to bw tables too. > > > > > > Fix this by moving list initialization code to the beginning of > > > xhci_mem_init() > > > > > > Reported-by: Sergey Dyasly <dserrg@xxxxxxxxx> > > > Tested-by: Sergey Dyasly <dserrg@xxxxxxxxx> > > > Signed-off-by: Vladimir Murzin <murzin.v@xxxxxxxxx> > > > --- > > > drivers/usb/host/xhci-mem.c | 37 ++++++++++++++++++++----------------- > > > 1 file changed, 20 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > > > index 6dc238c..7b5d2f5 100644 > > > --- a/drivers/usb/host/xhci-mem.c > > > +++ b/drivers/usb/host/xhci-mem.c > > > @@ -2123,7 +2123,7 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags) > > > __le32 __iomem *addr; > > > u32 offset; > > > unsigned int num_ports; > > > - int i, j, port_index; > > > + int i, port_index; > > > > > > addr = &xhci->cap_regs->hcc_params; > > > offset = XHCI_HCC_EXT_CAPS(xhci_readl(xhci, addr)); > > > @@ -2138,18 +2138,6 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags) > > > if (!xhci->port_array) > > > return -ENOMEM; > > > > > > - xhci->rh_bw = kzalloc(sizeof(*xhci->rh_bw)*num_ports, flags); > > > - if (!xhci->rh_bw) > > > - return -ENOMEM; > > > - for (i = 0; i < num_ports; i++) { > > > - struct xhci_interval_bw_table *bw_table; > > > - > > > - INIT_LIST_HEAD(&xhci->rh_bw[i].tts); > > > - bw_table = &xhci->rh_bw[i].bw_table; > > > - for (j = 0; j < XHCI_MAX_INTERVAL; j++) > > > - INIT_LIST_HEAD(&bw_table->interval_bw[j].endpoints); > > > - } > > > - > > > /* > > > * For whatever reason, the first capability offset is from the > > > * capability register base, not from the HCCPARAMS register. > > > @@ -2253,7 +2241,25 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) > > > u64 val_64; > > > struct xhci_segment *seg; > > > u32 page_size, temp; > > > - int i; > > > + int i, j, num_ports; > > > + > > > + INIT_LIST_HEAD(&xhci->cancel_cmd_list); > > > + INIT_LIST_HEAD(&xhci->lpm_failed_devs); > > > + > > > + num_ports = HCS_MAX_PORTS(xhci_readl(xhci, &xhci->cap_regs->hcs_params1)); > > > + > > > + xhci->rh_bw = kzalloc(sizeof(*xhci->rh_bw)*num_ports, flags); > > > + if (!xhci->rh_bw) > > > + return -ENOMEM; > > > + > > > + for (i = 0; i < num_ports; i++) { > > > + struct xhci_interval_bw_table *bw_table; > > > + > > > + INIT_LIST_HEAD(&xhci->rh_bw[i].tts); > > > + bw_table = &xhci->rh_bw[i].bw_table; > > > + for (j = 0; j < XHCI_MAX_INTERVAL; j++) > > > + INIT_LIST_HEAD(&bw_table->interval_bw[j].endpoints); > > > + } > > > > > > page_size = xhci_readl(xhci, &xhci->op_regs->page_size); > > > xhci_dbg(xhci, "Supported page size register = 0x%x\n", page_size); > > > @@ -2333,7 +2339,6 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) > > > xhci->cmd_ring = xhci_ring_alloc(xhci, 1, 1, TYPE_COMMAND, flags); > > > if (!xhci->cmd_ring) > > > goto fail; > > > - INIT_LIST_HEAD(&xhci->cancel_cmd_list); > > > xhci_dbg(xhci, "Allocated command ring at %p\n", xhci->cmd_ring); > > > xhci_dbg(xhci, "First segment DMA is 0x%llx\n", > > > (unsigned long long)xhci->cmd_ring->first_seg->dma); > > > @@ -2444,8 +2449,6 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) > > > if (xhci_setup_port_arrays(xhci, flags)) > > > goto fail; > > > > > > - INIT_LIST_HEAD(&xhci->lpm_failed_devs); > > > - > > > /* Enable USB 3.0 device notifications for function remote wake, which > > > * is necessary for allowing USB 3.0 devices to do remote wakeup from > > > * U3 (device suspend). > > > -- > > > 1.7.10.4 > > > -- 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
