From ef821ae70fc35a76bdce7e07c70a1a7c2c33cdb9 Mon Sep 17 00:00:00 2001 From: David Sterba Date: Tue, 2 Nov 2010 23:47:18 +0100 Subject: USB: xhci: Use GFP_ATOMIC under spin_lock coccinelle check scripts/coccinelle/locks/call_kern.cocci found that in drivers/usb/host/xhci.c an allocation with GFP_KERNEL is done with locks held: xhci_resume spin_lock_irq(xhci->lock) xhci_setup_msix kmalloc(GFP_KERNEL) Change it to GFP_ATOMIC. Signed-off-by: David Sterba CC: Sarah Sharp Signed-off-by: Greg Kroah-Hartman --- drivers/usb/host/xhci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/usb/host/xhci.c') diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 5d7d4e951ea4..8ddc965718ab 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -240,7 +240,7 @@ static int xhci_setup_msix(struct xhci_hcd *xhci) xhci->msix_entries = kmalloc((sizeof(struct msix_entry))*xhci->msix_count, - GFP_KERNEL); + GFP_ATOMIC); if (!xhci->msix_entries) { xhci_err(xhci, "Failed to allocate MSI-X entries\n"); return -ENOMEM; -- cgit v1.2.1 From 868719752d60fb04a3714d77fdeb780b4d585faf Mon Sep 17 00:00:00 2001 From: Greg Kroah-Hartman Date: Thu, 11 Nov 2010 09:41:02 -0800 Subject: Revert "USB: xhci: Use GFP_ATOMIC under spin_lock" This reverts commit ef821ae70fc35a76bdce7e07c70a1a7c2c33cdb9. The correct thing to do is to drop the spinlock, not change the GFP flag here. Thanks to Sarah for pointing out I shouldn't have taken this patch in the first place. Cc: Sarah Sharp Cc: David Sterba Signed-off-by: Greg Kroah-Hartman --- drivers/usb/host/xhci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/usb/host/xhci.c') diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 8ddc965718ab..5d7d4e951ea4 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -240,7 +240,7 @@ static int xhci_setup_msix(struct xhci_hcd *xhci) xhci->msix_entries = kmalloc((sizeof(struct msix_entry))*xhci->msix_count, - GFP_ATOMIC); + GFP_KERNEL); if (!xhci->msix_entries) { xhci_err(xhci, "Failed to allocate MSI-X entries\n"); return -ENOMEM; -- cgit v1.2.1 From 74bb844af8ec6a4c360b1fc7feb139801c0cacfd Mon Sep 17 00:00:00 2001 From: Andiry Xu Date: Wed, 27 Oct 2010 16:44:52 +0800 Subject: xHCI: release spinlock when setup interrupt Jiri Slaby reports spinlock is held while calling kmalloc(GFP_KERNEL) and request_irq() in xhci_resume(). Release the spinlock when setup interrupt. Reported-by: Jiri Slaby Signed-off-by: Andiry Xu Signed-off-by: Sarah Sharp --- drivers/usb/host/xhci.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'drivers/usb/host/xhci.c') diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 5d7d4e951ea4..7c8d70fbe113 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -714,6 +714,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated) return retval; } + spin_unlock_irq(&xhci->lock); /* Re-setup MSI-X */ if (hcd->irq) free_irq(hcd->irq, hcd); @@ -736,6 +737,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated) hcd->irq = pdev->irq; } + spin_lock_irq(&xhci->lock); /* step 4: set Run/Stop bit */ command = xhci_readl(xhci, &xhci->op_regs->command); command |= CMD_RUN; -- cgit v1.2.1 From 898213200cbadc570ef4248a6d90430c4a9c2908 Mon Sep 17 00:00:00 2001 From: Sarah Sharp Date: Fri, 12 Nov 2010 11:59:31 -0800 Subject: xhci: Fix command ring replay after resume. Andiry's xHCI bus suspend patch introduced the possibly of a host controller replaying old commands on the command ring, if the host successfully restores the registers after a resume. After a resume from suspend, the xHCI driver must restore the registers, including the command ring pointer. I had suggested that Andiry set the command ring pointer to the current command ring dequeue pointer, so that the driver wouldn't have to zero the command ring. Unfortunately, setting the command ring pointer to the current dequeue pointer won't work because the register assumes the pointer is 64-byte aligned, and TRBs on the command ring are 16-byte aligned. The lower seven bits will always be masked off, leading to the written pointer being up to 3 TRBs behind the intended pointer. Here's a log excerpt. On init, the xHCI driver places a vendor-specific command on the command ring: [ 215.750958] xhci_hcd 0000:01:00.0: Vendor specific event TRB type = 48 [ 215.750960] xhci_hcd 0000:01:00.0: NEC firmware version 30.25 [ 215.750962] xhci_hcd 0000:01:00.0: Command ring deq = 0x3781e010 (DMA) When we resume, the command ring dequeue pointer to be written should have been 0x3781e010. Instead, it's 0x3781e000: [ 235.557846] xhci_hcd 0000:01:00.0: // Setting command ring address to 0x3781e001 [ 235.557848] xhci_hcd 0000:01:00.0: `MEM_WRITE_DWORD(3'b000, 64'hffffc900100bc038, 64'h3781e001, 4'hf); [ 235.557850] xhci_hcd 0000:01:00.0: `MEM_WRITE_DWORD(3'b000, 32'hffffc900100bc020, 32'h204, 4'hf); [ 235.557866] usb usb9: root hub lost power or was reset (I can't see the results of this bug because the xHCI restore always fails on this box, and the xHCI driver re-allocates everything.) The fix is to zero the command ring and put the software and hardware enqueue and dequeue pointer back to the beginning of the ring. We do this before the system suspends, to be paranoid and prevent the BIOS from starting the host without clearing the command ring pointer, which might cause the host to muck with stale memory. (The pointer isn't required to be in the suspend power well, but it could be.) The command ring pointer is set again after the host resumes. Signed-off-by: Sarah Sharp Tested-by: Andiry Xu --- drivers/usb/host/xhci.c | 71 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 61 insertions(+), 10 deletions(-) (limited to 'drivers/usb/host/xhci.c') diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 7c8d70fbe113..06fca0835b52 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -577,6 +577,65 @@ static void xhci_restore_registers(struct xhci_hcd *xhci) xhci_write_64(xhci, xhci->s3.erst_base, &xhci->ir_set->erst_base); } +static void xhci_set_cmd_ring_deq(struct xhci_hcd *xhci) +{ + u64 val_64; + + /* step 2: initialize command ring buffer */ + val_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring); + val_64 = (val_64 & (u64) CMD_RING_RSVD_BITS) | + (xhci_trb_virt_to_dma(xhci->cmd_ring->deq_seg, + xhci->cmd_ring->dequeue) & + (u64) ~CMD_RING_RSVD_BITS) | + xhci->cmd_ring->cycle_state; + xhci_dbg(xhci, "// Setting command ring address to 0x%llx\n", + (long unsigned long) val_64); + xhci_write_64(xhci, val_64, &xhci->op_regs->cmd_ring); +} + +/* + * The whole command ring must be cleared to zero when we suspend the host. + * + * The host doesn't save the command ring pointer in the suspend well, so we + * need to re-program it on resume. Unfortunately, the pointer must be 64-byte + * aligned, because of the reserved bits in the command ring dequeue pointer + * register. Therefore, we can't just set the dequeue pointer back in the + * middle of the ring (TRBs are 16-byte aligned). + */ +static void xhci_clear_command_ring(struct xhci_hcd *xhci) +{ + struct xhci_ring *ring; + struct xhci_segment *seg; + + ring = xhci->cmd_ring; + seg = ring->deq_seg; + do { + memset(seg->trbs, 0, SEGMENT_SIZE); + seg = seg->next; + } while (seg != ring->deq_seg); + + /* Reset the software enqueue and dequeue pointers */ + ring->deq_seg = ring->first_seg; + ring->dequeue = ring->first_seg->trbs; + ring->enq_seg = ring->deq_seg; + ring->enqueue = ring->dequeue; + + /* + * Ring is now zeroed, so the HW should look for change of ownership + * when the cycle bit is set to 1. + */ + ring->cycle_state = 1; + + /* + * Reset the hardware dequeue pointer. + * Yes, this will need to be re-written after resume, but we're paranoid + * and want to make sure the hardware doesn't access bogus memory + * because, say, the BIOS or an SMI started the host without changing + * the command ring pointers. + */ + xhci_set_cmd_ring_deq(xhci); +} + /* * Stop HC (not bus-specific) * @@ -604,6 +663,7 @@ int xhci_suspend(struct xhci_hcd *xhci) spin_unlock_irq(&xhci->lock); return -ETIMEDOUT; } + xhci_clear_command_ring(xhci); /* step 3: save registers */ xhci_save_registers(xhci); @@ -635,7 +695,6 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated) u32 command, temp = 0; struct usb_hcd *hcd = xhci_to_hcd(xhci); struct pci_dev *pdev = to_pci_dev(hcd->self.controller); - u64 val_64; int old_state, retval; old_state = hcd->state; @@ -648,15 +707,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated) /* step 1: restore register */ xhci_restore_registers(xhci); /* step 2: initialize command ring buffer */ - val_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring); - val_64 = (val_64 & (u64) CMD_RING_RSVD_BITS) | - (xhci_trb_virt_to_dma(xhci->cmd_ring->deq_seg, - xhci->cmd_ring->dequeue) & - (u64) ~CMD_RING_RSVD_BITS) | - xhci->cmd_ring->cycle_state; - xhci_dbg(xhci, "// Setting command ring address to 0x%llx\n", - (long unsigned long) val_64); - xhci_write_64(xhci, val_64, &xhci->op_regs->cmd_ring); + xhci_set_cmd_ring_deq(xhci); /* step 3: restore state and start state*/ /* step 3: set CRS flag */ command = xhci_readl(xhci, &xhci->op_regs->command); -- cgit v1.2.1