[PATCH] USB: xhci: Use GFP_ATOMIC under spin_lock

[PATCH] USB: xhci: Use GFP_ATOMIC under spin_lock

[David Sterba]

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 <dsterba@suse.cz>
CC: Sarah Sharp <sarah.a.sharp@linux.intel.com>

---
Exists in v2.6.37-rc1 and current linux-next

diff -u -p a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
--- a/drivers/usb/host/xhci.c 2010-10-27 11:27:23.534310054 +0200
+++ b/drivers/usb/host/xhci.c 2010-11-02 15:53:03.000000000 +0100
@@ -240,7 +240,7 @@ static int xhci_setup_msix(struct xhci_h

        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;

Re: [PATCH] USB: xhci: Use GFP_ATOMIC under spin_lock

[Sarah Sharp]

On Wed, Nov 03, 2010 at 09:19:03AM +0100, Jiri Slaby wrote:
> On 11/02/2010 11:47 PM, David Sterba wrote:
> > 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.
> 
> Hi, I already reported that [1] and this is not enough. There are other
> sleepy calls like request_irq inside...
> 
> [1] http://lkml.org/lkml/2010/10/23/17

Andiry is looking into this.  Andiry, perhaps you don't need to take the
xHCI spinlock in xhci_resume()?  If that function is being called
because the PCI device is being resumed, you know nothing else is going
to touch the xHCI host controller.  (Except maybe the BIOS, but it isn't
going to respect xhci->lock at all.)  The USB core certainly won't touch
the host controller until it's resumed.  Maybe we could get an interrupt
with a port status change, but I think it's unlikely...

Alan, can you think of any reason the xHCI driver would need to grab its
host controller spinlock on PCI resume?

Sarah Sharp

Re: [PATCH] USB: xhci: Use GFP_ATOMIC under spin_lock

[Alan Stern]

On Thu, 4 Nov 2010, Sarah Sharp wrote:

> On Wed, Nov 03, 2010 at 09:19:03AM +0100, Jiri Slaby wrote:
> > On 11/02/2010 11:47 PM, David Sterba wrote:
> > > 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.
> > 
> > Hi, I already reported that [1] and this is not enough. There are other
> > sleepy calls like request_irq inside...
> > 
> > [1] http://lkml.org/lkml/2010/10/23/17
> 
> Andiry is looking into this.  Andiry, perhaps you don't need to take the
> xHCI spinlock in xhci_resume()?  If that function is being called
> because the PCI device is being resumed, you know nothing else is going
> to touch the xHCI host controller.  (Except maybe the BIOS, but it isn't
> going to respect xhci->lock at all.)  The USB core certainly won't touch
> the host controller until it's resumed.  Maybe we could get an interrupt
> with a port status change, but I think it's unlikely...
> 
> Alan, can you think of any reason the xHCI driver would need to grab its
> host controller spinlock on PCI resume?

Maybe an interrupt isn't as unlikely as all that.  To be safe you 
should acquire the spinlock.  The real question is why these other 
routines are performing actions that could block in a path that holds 
the lock.  Maybe the spinlock should be acquired _after_ doing the 
potentially-blocking operations.

Alan Stern