On Mon, Apr 08, 2013 at 03:53:19PM -0700, Sarah Sharp wrote:
> 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()
>
> Hmm, in looking at this patch again, I don't think it's wise to fix the
> issue with accessing the bandwidth table lists in this manner. What
> will happen if someone decides to place more allocation code before the
> rh_bw allocation code? I will probably forget why we moved that code
> up, and then we'll run into the same bug again.
>
> Moving up the init code for xhci->cancel_cmd_list and
> xhci->lpm_failed_devs made sense, since it's allocated as part of the
> xhci_hcd structure. However, since rh_bw is dynamically allocated,
> xhci_mem_cleanup needs to be able to handle it being NULL.
>
> Instead, I think this patch should fix xhci_mem_cleanup to check for a
> NULL rh_bw structure here:
>
> num_ports = HCS_MAX_PORTS(xhci->hcs_params1);
> for (i = 0; i < num_ports; i++) {
> struct xhci_interval_bw_table *bwt = &xhci->rh_bw[i].bw_table;
> for (j = 0; j < XHCI_MAX_INTERVAL; j++) {
> struct list_head *ep = &bwt->interval_bw[j].endpoints;
> while (!list_empty(ep))
> list_del_init(ep->next);
> }
> }
>
> for (i = 0; i < num_ports; i++) {
> struct xhci_tt_bw_info *tt, *n;
> list_for_each_entry_safe(tt, n, &xhci->rh_bw[i].tts, tt_list) {
> list_del(&tt->tt_list);
> kfree(tt);
> }
> }
>
> Just skip those two for loops if rh_bw is NULL. Once rh_bw is non-null,
> we know xhci_mem_init will have initialized the bandwidth table lists.
>
> So, Sergio, I'm applying your v3 patch to my for-usb-linus-queue branch:
>
> http://git.kernel.org/cgit/linux/kernel/git/sarah/xhci.git/log/?h=for-usb-linus-queue
>
> Vladimir, please use that branch to create a new patch that fixes
> xhci_mem_cleanup.
>
> That way you both get a patch in. :)
Cool!
Thanks, Sarah! :)
Regards,
Sergio
>
> Sarah Sharp
>
> >
> > 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
