[PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[Joerg Roedel]

Booting 2.6.38 on my test machine produces a lockdep warning
from the usb_amd_find_chipset_info() function:

 WARNING: at /data/lemmy/linux.trees.git/kernel/lockdep.c:2465 lockdep_trace_alloc+0x95/0xc2()
 Hardware name: Snook
 Modules linked in:
 Pid: 959, comm: work_for_cpu Not tainted 2.6.39-rc2+ #22
 Call Trace:
  [<ffffffff8103c0d4>] warn_slowpath_common+0x80/0x98
  [<ffffffff812387e6>] ? T.492+0x24/0x26
  [<ffffffff8103c101>] warn_slowpath_null+0x15/0x17
  [<ffffffff81068667>] lockdep_trace_alloc+0x95/0xc2
  [<ffffffff810ed9ac>] slab_pre_alloc_hook+0x18/0x3b
  [<ffffffff810ef227>] kmem_cache_alloc_trace+0x25/0xba
  [<ffffffff812387e6>] T.492+0x24/0x26
  [<ffffffff81238816>] pci_get_subsys+0x2e/0x73
 sr0: scsi3-mmc drive: 48x/48x writer dvd-ram cd/rw xa/form2 cdda tray
  [<ffffffff8123886c>] pci_get_device+0x11/0x13
  [<ffffffff814082a9>] usb_amd_find_chipset_info+0x3f/0x18a
...

It turns out that this function calls pci_get_device under a spin_lock
with irqs disabled, but the pci_get_device function is only allowed in
preemptible context.

This patch fixes this by using temporary variables in the quirk
algorithm and commiting them later to the struct under the lock. This
moves all pci_get_device() invocations out of the spin_lock and fixes
the lockdep warning for me.

Cc: stable@kernel.org
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/usb/host/pci-quirks.c |   67 +++++++++++++++++++++++-----------------
 1 files changed, 38 insertions(+), 29 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 1d586d4..d3e5cf3 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -84,64 +84,73 @@ int usb_amd_find_chipset_info(void)
 {
 	u8 rev = 0;
 	unsigned long flags;
+	struct pci_dev *nb_dev, *smbus_dev;
+	int nb_type, sb_type;
 
 	spin_lock_irqsave(&amd_lock, flags);
-
 	amd_chipset.probe_count++;
 	/* probe only once */
 	if (amd_chipset.probe_count > 1) {
 		spin_unlock_irqrestore(&amd_lock, flags);
 		return amd_chipset.probe_result;
 	}
+	spin_unlock_irqrestore(&amd_lock, flags);
 
-	amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL);
-	if (amd_chipset.smbus_dev) {
-		rev = amd_chipset.smbus_dev->revision;
+	sb_type   = 0;
+	smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL);
+	if (smbus_dev) {
+		rev = smbus_dev->revision;
 		if (rev >= 0x40)
-			amd_chipset.sb_type = 1;
+			sb_type = 1;
 		else if (rev >= 0x30 && rev <= 0x3b)
-			amd_chipset.sb_type = 3;
+			sb_type = 3;
 	} else {
-		amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-							0x780b, NULL);
-		if (!amd_chipset.smbus_dev) {
+		smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x780b, NULL);
+		if (smbus_dev) {
+			spin_lock_irqsave(&amd_lock, flags);
+			amd_chipset.smbus_dev = smbus_dev;
 			spin_unlock_irqrestore(&amd_lock, flags);
 			return 0;
 		}
-		rev = amd_chipset.smbus_dev->revision;
+		rev = smbus_dev->revision;
 		if (rev >= 0x11 && rev <= 0x18)
-			amd_chipset.sb_type = 2;
+			sb_type = 2;
 	}
 
-	if (amd_chipset.sb_type == 0) {
-		if (amd_chipset.smbus_dev) {
-			pci_dev_put(amd_chipset.smbus_dev);
-			amd_chipset.smbus_dev = NULL;
+	if (sb_type == 0) {
+		if (smbus_dev) {
+			pci_dev_put(smbus_dev);
+			smbus_dev = NULL;
 		}
-		spin_unlock_irqrestore(&amd_lock, flags);
 		return 0;
 	}
 
-	amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
-	if (amd_chipset.nb_dev) {
-		amd_chipset.nb_type = 1;
+	nb_type = 0;
+	nb_dev  = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
+	if (nb_dev) {
+		nb_type = 1;
 	} else {
-		amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-							0x1510, NULL);
-		if (amd_chipset.nb_dev) {
-			amd_chipset.nb_type = 2;
-		} else  {
-			amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-								0x9600, NULL);
-			if (amd_chipset.nb_dev)
-				amd_chipset.nb_type = 3;
+		nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x1510, NULL);
+		if (nb_dev) {
+			nb_type = 2;
+		} else {
+			nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
+						0x9600, NULL);
+			if (nb_dev)
+				nb_type = 3;
 		}
 	}
 
+	spin_lock_irqsave(&amd_lock, flags);
+	amd_chipset.smbus_dev    = smbus_dev;
+	amd_chipset.sb_type      = sb_type;
+	amd_chipset.nb_dev       = nb_dev;
+	amd_chipset.nb_type      = nb_type;
 	amd_chipset.probe_result = 1;
+	spin_unlock_irqrestore(&amd_lock, flags);
+
 	printk(KERN_DEBUG "QUIRK: Enable AMD PLL fix\n");
 
-	spin_unlock_irqrestore(&amd_lock, flags);
 	return amd_chipset.probe_result;
 }
 EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);
-- 
1.7.1

Re: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[Alan Stern]

On Wed, 6 Apr 2011, Joerg Roedel wrote:

> Booting 2.6.38 on my test machine produces a lockdep warning
> from the usb_amd_find_chipset_info() function:
> 
>  WARNING: at /data/lemmy/linux.trees.git/kernel/lockdep.c:2465 lockdep_trace_alloc+0x95/0xc2()
>  Hardware name: Snook
>  Modules linked in:
>  Pid: 959, comm: work_for_cpu Not tainted 2.6.39-rc2+ #22
>  Call Trace:
>   [<ffffffff8103c0d4>] warn_slowpath_common+0x80/0x98
>   [<ffffffff812387e6>] ? T.492+0x24/0x26
>   [<ffffffff8103c101>] warn_slowpath_null+0x15/0x17
>   [<ffffffff81068667>] lockdep_trace_alloc+0x95/0xc2
>   [<ffffffff810ed9ac>] slab_pre_alloc_hook+0x18/0x3b
>   [<ffffffff810ef227>] kmem_cache_alloc_trace+0x25/0xba
>   [<ffffffff812387e6>] T.492+0x24/0x26
>   [<ffffffff81238816>] pci_get_subsys+0x2e/0x73
>  sr0: scsi3-mmc drive: 48x/48x writer dvd-ram cd/rw xa/form2 cdda tray
>   [<ffffffff8123886c>] pci_get_device+0x11/0x13
>   [<ffffffff814082a9>] usb_amd_find_chipset_info+0x3f/0x18a
> ...
> 
> It turns out that this function calls pci_get_device under a spin_lock
> with irqs disabled, but the pci_get_device function is only allowed in
> preemptible context.
> 
> This patch fixes this by using temporary variables in the quirk
> algorithm and commiting them later to the struct under the lock. This
> moves all pci_get_device() invocations out of the spin_lock and fixes
> the lockdep warning for me.
> 
> Cc: stable@kernel.org
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  drivers/usb/host/pci-quirks.c |   67 +++++++++++++++++++++++-----------------
>  1 files changed, 38 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 1d586d4..d3e5cf3 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -84,64 +84,73 @@ int usb_amd_find_chipset_info(void)
>  {
>  	u8 rev = 0;
>  	unsigned long flags;
> +	struct pci_dev *nb_dev, *smbus_dev;
> +	int nb_type, sb_type;
>  
>  	spin_lock_irqsave(&amd_lock, flags);
> -
>  	amd_chipset.probe_count++;
>  	/* probe only once */
>  	if (amd_chipset.probe_count > 1) {
>  		spin_unlock_irqrestore(&amd_lock, flags);
>  		return amd_chipset.probe_result;
>  	}

The counter really should be a bool: Has the chipset already been
probed or not?  After all, nobody cares how many times this routine was 
called.

> +	spin_unlock_irqrestore(&amd_lock, flags);

This code now contains a bug: You incremented the probe_count _before_
doing the probe.  If another thread calls this routine right now, it
will get an incorrect result.

Fixing this up should be fairly easy.

Alan Stern

Re: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[Roedel, Joerg]

On Wed, Apr 06, 2011 at 11:16:07AM -0400, Alan Stern wrote:
> On Wed, 6 Apr 2011, Joerg Roedel wrote:
> >  	amd_chipset.probe_count++;
> >  	/* probe only once */
> >  	if (amd_chipset.probe_count > 1) {
> >  		spin_unlock_irqrestore(&amd_lock, flags);
> >  		return amd_chipset.probe_result;
> >  	}
> 
> The counter really should be a bool: Has the chipset already been
> probed or not?  After all, nobody cares how many times this routine was 
> called.

The quirk function needs a re-structuring anyway. The code-path is not
very straight-forward and readable. But for this fix I tried to keep the
changes small.

> 
> > +	spin_unlock_irqrestore(&amd_lock, flags);
> 
> This code now contains a bug: You incremented the probe_count _before_
> doing the probe.  If another thread calls this routine right now, it
> will get an incorrect result.
> 
> Fixing this up should be fairly easy.

Okay, I will do. Thanks for the feedback.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

RE: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[Xu, Andiry]

> -----Original Message-----
> From: Alan Stern [mailto:stern@rowland.harvard.edu]
> Sent: Wednesday, April 06, 2011 11:16 PM
> To: Roedel, Joerg
> Cc: Greg Kroah-Hartman; Sarah Sharp; Xu, Andiry;
linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org; stable@kernel.org
> Subject: Re: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk
> 
> On Wed, 6 Apr 2011, Joerg Roedel wrote:
> 
> > Booting 2.6.38 on my test machine produces a lockdep warning
> > from the usb_amd_find_chipset_info() function:
> >
> >  WARNING: at /data/lemmy/linux.trees.git/kernel/lockdep.c:2465
> lockdep_trace_alloc+0x95/0xc2()
> >  Hardware name: Snook
> >  Modules linked in:
> >  Pid: 959, comm: work_for_cpu Not tainted 2.6.39-rc2+ #22
> >  Call Trace:
> >   [<ffffffff8103c0d4>] warn_slowpath_common+0x80/0x98
> >   [<ffffffff812387e6>] ? T.492+0x24/0x26
> >   [<ffffffff8103c101>] warn_slowpath_null+0x15/0x17
> >   [<ffffffff81068667>] lockdep_trace_alloc+0x95/0xc2
> >   [<ffffffff810ed9ac>] slab_pre_alloc_hook+0x18/0x3b
> >   [<ffffffff810ef227>] kmem_cache_alloc_trace+0x25/0xba
> >   [<ffffffff812387e6>] T.492+0x24/0x26
> >   [<ffffffff81238816>] pci_get_subsys+0x2e/0x73
> >  sr0: scsi3-mmc drive: 48x/48x writer dvd-ram cd/rw xa/form2 cdda
tray
> >   [<ffffffff8123886c>] pci_get_device+0x11/0x13
> >   [<ffffffff814082a9>] usb_amd_find_chipset_info+0x3f/0x18a
> > ...
> >
> > It turns out that this function calls pci_get_device under a
spin_lock
> > with irqs disabled, but the pci_get_device function is only allowed
in
> > preemptible context.
> >
> > This patch fixes this by using temporary variables in the quirk
> > algorithm and commiting them later to the struct under the lock.
This
> > moves all pci_get_device() invocations out of the spin_lock and
fixes
> > the lockdep warning for me.
> >
> > Cc: stable@kernel.org
> > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> > ---
> >  drivers/usb/host/pci-quirks.c |   67
+++++++++++++++++++++++-----------
> ------
> >  1 files changed, 38 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-
> quirks.c
> > index 1d586d4..d3e5cf3 100644
> > --- a/drivers/usb/host/pci-quirks.c
> > +++ b/drivers/usb/host/pci-quirks.c
> > @@ -84,64 +84,73 @@ int usb_amd_find_chipset_info(void)
> >  {
> >  	u8 rev = 0;
> >  	unsigned long flags;
> > +	struct pci_dev *nb_dev, *smbus_dev;
> > +	int nb_type, sb_type;
> >
> >  	spin_lock_irqsave(&amd_lock, flags);
> > -
> >  	amd_chipset.probe_count++;
> >  	/* probe only once */
> >  	if (amd_chipset.probe_count > 1) {
> >  		spin_unlock_irqrestore(&amd_lock, flags);
> >  		return amd_chipset.probe_result;
> >  	}
> 
> The counter really should be a bool: Has the chipset already been
> probed or not?  After all, nobody cares how many times this routine
was
> called.
> 

The probe routine increases probe_count counter to remember how many
hosts calls it. Every host that calls probe routine in initialization
will call usb_amd_dev_put() in *hci_stop() and decrease probe_count, so
the last caller can safely free the resources.

Thanks,
Andiry
 

Re: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[Roedel, Joerg]

On Wed, Apr 06, 2011 at 11:16:07AM -0400, Alan Stern wrote:
> On Wed, 6 Apr 2011, Joerg Roedel wrote:

> The counter really should be a bool: Has the chipset already been
> probed or not?  After all, nobody cares how many times this routine was 
> called.
> 
> > +	spin_unlock_irqrestore(&amd_lock, flags);
> 
> This code now contains a bug: You incremented the probe_count _before_
> doing the probe.  If another thread calls this routine right now, it
> will get an incorrect result.
> 
> Fixing this up should be fairly easy.

Hmm, we can get rid of the amd_lock completly if every thread uses the
following call-order:

	usb_amd_find_chipset_info();
	usb_amd_quirk_pll_enable();
	usb_amd_quirk_pll_disable();
	usb_amd_dev_put();

In that case we can just change the probe_count and isoc_reqs into
atomic_t with some care and should be fine, no?
Problem is that I don't know if the above call-order is guaranteed.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[Joerg Roedel]

Booting 2.6.38 on my test machine produces a lockdep warning
from the usb_amd_find_chipset_info() function:

 WARNING: at /data/lemmy/linux.trees.git/kernel/lockdep.c:2465 lockdep_trace_alloc+0x95/0xc2()
 Hardware name: Snook
 Modules linked in:
 Pid: 959, comm: work_for_cpu Not tainted 2.6.39-rc2+ #22
 Call Trace:
  [<ffffffff8103c0d4>] warn_slowpath_common+0x80/0x98
  [<ffffffff812387e6>] ? T.492+0x24/0x26
  [<ffffffff8103c101>] warn_slowpath_null+0x15/0x17
  [<ffffffff81068667>] lockdep_trace_alloc+0x95/0xc2
  [<ffffffff810ed9ac>] slab_pre_alloc_hook+0x18/0x3b
  [<ffffffff810ef227>] kmem_cache_alloc_trace+0x25/0xba
  [<ffffffff812387e6>] T.492+0x24/0x26
  [<ffffffff81238816>] pci_get_subsys+0x2e/0x73
 sr0: scsi3-mmc drive: 48x/48x writer dvd-ram cd/rw xa/form2 cdda tray
  [<ffffffff8123886c>] pci_get_device+0x11/0x13
  [<ffffffff814082a9>] usb_amd_find_chipset_info+0x3f/0x18a
...

It turns out that this function calls pci_get_device under a spin_lock
with irqs disabled, but the pci_get_device function is only allowed in
preemptible context.

This patch fixes this by using temporary variables in the quirk
algorithm and commiting them later to the struct under the lock. This
moves all pci_get_device() invocations out of the spin_lock and fixes
the lockdep warning.

Cc: stable@kernel.org
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/usb/host/pci-quirks.c |   86 +++++++++++++++++++++++++----------------
 1 files changed, 52 insertions(+), 34 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 1d586d4..b5802f6 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -84,65 +84,83 @@ int usb_amd_find_chipset_info(void)
 {
 	u8 rev = 0;
 	unsigned long flags;
+	struct amd_chipset_info info;
+	int ret;
 
 	spin_lock_irqsave(&amd_lock, flags);
 
-	amd_chipset.probe_count++;
 	/* probe only once */
-	if (amd_chipset.probe_count > 1) {
+	if (amd_chipset.probe_count > 0) {
 		spin_unlock_irqrestore(&amd_lock, flags);
 		return amd_chipset.probe_result;
 	}
+	info = amd_chipset;
+	spin_unlock_irqrestore(&amd_lock, flags);
 
-	amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL);
-	if (amd_chipset.smbus_dev) {
-		rev = amd_chipset.smbus_dev->revision;
+	info.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL);
+	if (info.smbus_dev) {
+		rev = info.smbus_dev->revision;
 		if (rev >= 0x40)
-			amd_chipset.sb_type = 1;
+			info.sb_type = 1;
 		else if (rev >= 0x30 && rev <= 0x3b)
-			amd_chipset.sb_type = 3;
+			info.sb_type = 3;
 	} else {
-		amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-							0x780b, NULL);
-		if (!amd_chipset.smbus_dev) {
-			spin_unlock_irqrestore(&amd_lock, flags);
-			return 0;
+		info.smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD,
+						0x780b, NULL);
+		if (!info.smbus_dev) {
+			ret = 0;
+			goto commit;
 		}
-		rev = amd_chipset.smbus_dev->revision;
+
+		rev = info.smbus_dev->revision;
 		if (rev >= 0x11 && rev <= 0x18)
-			amd_chipset.sb_type = 2;
+			info.sb_type = 2;
 	}
 
-	if (amd_chipset.sb_type == 0) {
-		if (amd_chipset.smbus_dev) {
-			pci_dev_put(amd_chipset.smbus_dev);
-			amd_chipset.smbus_dev = NULL;
+	if (info.sb_type == 0) {
+		if (info.smbus_dev) {
+			pci_dev_put(info.smbus_dev);
+			info.smbus_dev = NULL;
 		}
-		spin_unlock_irqrestore(&amd_lock, flags);
-		return 0;
+		ret = 0;
+		goto commit;
 	}
 
-	amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
-	if (amd_chipset.nb_dev) {
-		amd_chipset.nb_type = 1;
+	info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
+	if (info.nb_dev) {
+		info.nb_type = 1;
 	} else {
-		amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-							0x1510, NULL);
-		if (amd_chipset.nb_dev) {
-			amd_chipset.nb_type = 2;
-		} else  {
-			amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-								0x9600, NULL);
-			if (amd_chipset.nb_dev)
-				amd_chipset.nb_type = 3;
+		info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x1510, NULL);
+		if (info.nb_dev) {
+			info.nb_type = 2;
+		} else {
+			info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
+						     0x9600, NULL);
+			if (info.nb_dev)
+				info.nb_type = 3;
 		}
 	}
 
-	amd_chipset.probe_result = 1;
+	ret = info.probe_result = 1;
 	printk(KERN_DEBUG "QUIRK: Enable AMD PLL fix\n");
 
+commit:
+
+	spin_lock_irqsave(&amd_lock, flags);
+	if (amd_chipset.probe_count > 0) {
+		/* race - someone else was faster - drop devices */
+		if (info.nb_dev)
+			pci_dev_put(info.nb_dev);
+		if (info.smbus_dev)
+			pci_dev_put(info.smbus_dev);
+	} else {
+		/* no race - commit the result */
+		info.probe_count++;
+		amd_chipset = info;
+	}
 	spin_unlock_irqrestore(&amd_lock, flags);
-	return amd_chipset.probe_result;
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);
 
-- 
1.7.1

RE: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[Xu, Andiry]

> -----Original Message-----
> From: Roedel, Joerg [mailto:Joerg.Roedel@amd.com]
> Sent: Thursday, April 07, 2011 3:51 PM
> To: Alan Stern
> Cc: Greg Kroah-Hartman; Sarah Sharp; Xu, Andiry;
linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org; stable@kernel.org
> Subject: Re: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk
> 
> On Wed, Apr 06, 2011 at 11:16:07AM -0400, Alan Stern wrote:
> > On Wed, 6 Apr 2011, Joerg Roedel wrote:
> 
> > The counter really should be a bool: Has the chipset already been
> > probed or not?  After all, nobody cares how many times this routine
was
> > called.
> >
> > > +	spin_unlock_irqrestore(&amd_lock, flags);
> >
> > This code now contains a bug: You incremented the probe_count
_before_
> > doing the probe.  If another thread calls this routine right now, it
> > will get an incorrect result.
> >
> > Fixing this up should be fairly easy.
> 
> Hmm, we can get rid of the amd_lock completly if every thread uses the
> following call-order:
> 
> 	usb_amd_find_chipset_info();
> 	usb_amd_quirk_pll_enable();
> 	usb_amd_quirk_pll_disable();
> 	usb_amd_dev_put();
> 

The correct order is:

usb_amd_find_chipset_info();
usb_amd_quirk_pll_disable();
usb_amd_quirk_pll_enable();
usb_amd_dev_put();

The pair of pll disable and enable may be called for multiple times.

Thanks,
Andiry

RE: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[Xu, Andiry]

> -----Original Message-----
> From: Joerg Roedel [mailto:joerg.roedel@amd.com]
> Sent: Thursday, April 07, 2011 4:26 PM
> To: Greg Kroah-Hartman; Sarah Sharp; Xu, Andiry
> Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Roedel,
Joerg;
> stable@kernel.org
> Subject: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk
> 
> Booting 2.6.38 on my test machine produces a lockdep warning
> from the usb_amd_find_chipset_info() function:
> 
>  WARNING: at /data/lemmy/linux.trees.git/kernel/lockdep.c:2465
> lockdep_trace_alloc+0x95/0xc2()
>  Hardware name: Snook
>  Modules linked in:
>  Pid: 959, comm: work_for_cpu Not tainted 2.6.39-rc2+ #22
>  Call Trace:
>   [<ffffffff8103c0d4>] warn_slowpath_common+0x80/0x98
>   [<ffffffff812387e6>] ? T.492+0x24/0x26
>   [<ffffffff8103c101>] warn_slowpath_null+0x15/0x17
>   [<ffffffff81068667>] lockdep_trace_alloc+0x95/0xc2
>   [<ffffffff810ed9ac>] slab_pre_alloc_hook+0x18/0x3b
>   [<ffffffff810ef227>] kmem_cache_alloc_trace+0x25/0xba
>   [<ffffffff812387e6>] T.492+0x24/0x26
>   [<ffffffff81238816>] pci_get_subsys+0x2e/0x73
>  sr0: scsi3-mmc drive: 48x/48x writer dvd-ram cd/rw xa/form2 cdda tray
>   [<ffffffff8123886c>] pci_get_device+0x11/0x13
>   [<ffffffff814082a9>] usb_amd_find_chipset_info+0x3f/0x18a
> ...
> 
> It turns out that this function calls pci_get_device under a spin_lock
> with irqs disabled, but the pci_get_device function is only allowed in
> preemptible context.
> 
> This patch fixes this by using temporary variables in the quirk
> algorithm and commiting them later to the struct under the lock. This
> moves all pci_get_device() invocations out of the spin_lock and fixes
> the lockdep warning.
> 
> Cc: stable@kernel.org
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  drivers/usb/host/pci-quirks.c |   86
+++++++++++++++++++++++++-----------
> -----
>  1 files changed, 52 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/usb/host/pci-quirks.c
b/drivers/usb/host/pci-quirks.c
> index 1d586d4..b5802f6 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -84,65 +84,83 @@ int usb_amd_find_chipset_info(void)
>  {
>  	u8 rev = 0;
>  	unsigned long flags;
> +	struct amd_chipset_info info;
> +	int ret;
> 
>  	spin_lock_irqsave(&amd_lock, flags);
> 
> -	amd_chipset.probe_count++;
>  	/* probe only once */
> -	if (amd_chipset.probe_count > 1) {
> +	if (amd_chipset.probe_count > 0) {
>  		spin_unlock_irqrestore(&amd_lock, flags);
>  		return amd_chipset.probe_result;
>  	}
> +	info = amd_chipset;
> +	spin_unlock_irqrestore(&amd_lock, flags);
> 
> -	amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI,
0x4385,
> NULL);
> -	if (amd_chipset.smbus_dev) {
> -		rev = amd_chipset.smbus_dev->revision;
> +	info.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385,
NULL);
> +	if (info.smbus_dev) {
> +		rev = info.smbus_dev->revision;
>  		if (rev >= 0x40)
> -			amd_chipset.sb_type = 1;
> +			info.sb_type = 1;
>  		else if (rev >= 0x30 && rev <= 0x3b)
> -			amd_chipset.sb_type = 3;
> +			info.sb_type = 3;
>  	} else {
> -		amd_chipset.smbus_dev =
pci_get_device(PCI_VENDOR_ID_AMD,
> -							0x780b, NULL);
> -		if (!amd_chipset.smbus_dev) {
> -			spin_unlock_irqrestore(&amd_lock, flags);
> -			return 0;
> +		info.smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD,
> +						0x780b, NULL);
> +		if (!info.smbus_dev) {
> +			ret = 0;
> +			goto commit;
>  		}
> -		rev = amd_chipset.smbus_dev->revision;
> +
> +		rev = info.smbus_dev->revision;
>  		if (rev >= 0x11 && rev <= 0x18)
> -			amd_chipset.sb_type = 2;
> +			info.sb_type = 2;
>  	}
> 
> -	if (amd_chipset.sb_type == 0) {
> -		if (amd_chipset.smbus_dev) {
> -			pci_dev_put(amd_chipset.smbus_dev);
> -			amd_chipset.smbus_dev = NULL;
> +	if (info.sb_type == 0) {
> +		if (info.smbus_dev) {
> +			pci_dev_put(info.smbus_dev);
> +			info.smbus_dev = NULL;
>  		}
> -		spin_unlock_irqrestore(&amd_lock, flags);
> -		return 0;
> +		ret = 0;
> +		goto commit;
>  	}
> 
> -	amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601,
NULL);
> -	if (amd_chipset.nb_dev) {
> -		amd_chipset.nb_type = 1;
> +	info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
> +	if (info.nb_dev) {
> +		info.nb_type = 1;
>  	} else {
> -		amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
> -							0x1510, NULL);
> -		if (amd_chipset.nb_dev) {
> -			amd_chipset.nb_type = 2;
> -		} else  {
> -			amd_chipset.nb_dev =
pci_get_device(PCI_VENDOR_ID_AMD,
> -								0x9600,
NULL);
> -			if (amd_chipset.nb_dev)
> -				amd_chipset.nb_type = 3;
> +		info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x1510,
NULL);
> +		if (info.nb_dev) {
> +			info.nb_type = 2;
> +		} else {
> +			info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
> +						     0x9600, NULL);
> +			if (info.nb_dev)
> +				info.nb_type = 3;
>  		}
>  	}
> 
> -	amd_chipset.probe_result = 1;
> +	ret = info.probe_result = 1;
>  	printk(KERN_DEBUG "QUIRK: Enable AMD PLL fix\n");
> 
> +commit:
> +
> +	spin_lock_irqsave(&amd_lock, flags);
> +	if (amd_chipset.probe_count > 0) {
> +		/* race - someone else was faster - drop devices */
> +		if (info.nb_dev)
> +			pci_dev_put(info.nb_dev);
> +		if (info.smbus_dev)
> +			pci_dev_put(info.smbus_dev);

You need to increase info.probe_count here and set
amd_chipset.probe_count with it, because it's decreased during every
host controller stop routine.


> +	} else {
> +		/* no race - commit the result */
> +		info.probe_count++;
> +		amd_chipset = info;
> +	}
>  	spin_unlock_irqrestore(&amd_lock, flags);
> -	return amd_chipset.probe_result;
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);
> 
> --
> 1.7.1

The original design target is to probe the chipset only once, so other
host controllers will not probe the chipset again. This patch allows
multiple threads to probe the chipset at the same time, and only one
thread is doing the valid work. It may add unnecessary cost.

Thanks,
Andiry

Re: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[Roedel, Joerg]

On Thu, Apr 07, 2011 at 05:58:14AM -0400, Xu, Andiry wrote:
> > +     spin_lock_irqsave(&amd_lock, flags);
> > +     if (amd_chipset.probe_count > 0) {
> > +             /* race - someone else was faster - drop devices */
> > +             if (info.nb_dev)
> > +                     pci_dev_put(info.nb_dev);
> > +             if (info.smbus_dev)
> > +                     pci_dev_put(info.smbus_dev);
> 
> You need to increase info.probe_count here and set
> amd_chipset.probe_count with it, because it's decreased during every
> host controller stop routine.

Right, the correct return value needs to be set too. I'll fix it.

> > +     } else {
> > +             /* no race - commit the result */
> > +             info.probe_count++;
> > +             amd_chipset = info;
> > +     }
> >       spin_unlock_irqrestore(&amd_lock, flags);
> > -     return amd_chipset.probe_result;
> > +
> > +     return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);
> >
> > --
> > 1.7.1
> 
> The original design target is to probe the chipset only once, so other
> host controllers will not probe the chipset again. This patch allows
> multiple threads to probe the chipset at the same time, and only one
> thread is doing the valid work. It may add unnecessary cost.

Not necessarily. With this patch more than one thread could do the valid
work at the same time. With the old code the other threads would
have spent this time spinning on the lock. So I don't think there is
much additional cost introduced with this patch. And as a plus, it fixes
a bug :-)
I'll send a fixed version soon.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[Roedel, Joerg]

On Thu, Apr 07, 2011 at 05:01:01AM -0400, Xu, Andiry wrote:
> > -----Original Message-----
> > From: Roedel, Joerg [mailto:Joerg.Roedel@amd.com]
> > Sent: Thursday, April 07, 2011 3:51 PM
> > To: Alan Stern
> > Cc: Greg Kroah-Hartman; Sarah Sharp; Xu, Andiry; linux-usb@vger.kernel.org;
> > linux-kernel@vger.kernel.org; stable@kernel.org
> > Subject: Re: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk
> >
> > On Wed, Apr 06, 2011 at 11:16:07AM -0400, Alan Stern wrote:
> > > On Wed, 6 Apr 2011, Joerg Roedel wrote:
> >
> > > The counter really should be a bool: Has the chipset already been
> > > probed or not?  After all, nobody cares how many times this routine was
> > > called.
> > >
> > > > + spin_unlock_irqrestore(&amd_lock, flags);
> > >
> > > This code now contains a bug: You incremented the probe_count _before_
> > > doing the probe.  If another thread calls this routine right now, it
> > > will get an incorrect result.
> > >
> > > Fixing this up should be fairly easy.
> >
> > Hmm, we can get rid of the amd_lock completly if every thread uses the
> > following call-order:
> >
> >       usb_amd_find_chipset_info();
> >       usb_amd_quirk_pll_enable();
> >       usb_amd_quirk_pll_disable();
> >       usb_amd_dev_put();
> >
> 
> The correct order is:
> 
> usb_amd_find_chipset_info();
> usb_amd_quirk_pll_disable();
> usb_amd_quirk_pll_enable();
> usb_amd_dev_put();
> 
> The pair of pll disable and enable may be called for multiple times.

So we could access the data structure without any locks if we want using
atomic_t for the probe_count and isoc_reqs members. But as I've seen
meanwhile the lock still needs to protect the access to the hardware in
the usb_amd_quirk_pll() function.
So its probably not worth the work, what do you think?

Regards,

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[Joerg Roedel]

Booting 2.6.38 on my test machine produces a lockdep warning
from the usb_amd_find_chipset_info() function:

 WARNING: at /data/lemmy/linux.trees.git/kernel/lockdep.c:2465 lockdep_trace_alloc+0x95/0xc2()
 Hardware name: Snook
 Modules linked in:
 Pid: 959, comm: work_for_cpu Not tainted 2.6.39-rc2+ #22
 Call Trace:
  [<ffffffff8103c0d4>] warn_slowpath_common+0x80/0x98
  [<ffffffff812387e6>] ? T.492+0x24/0x26
  [<ffffffff8103c101>] warn_slowpath_null+0x15/0x17
  [<ffffffff81068667>] lockdep_trace_alloc+0x95/0xc2
  [<ffffffff810ed9ac>] slab_pre_alloc_hook+0x18/0x3b
  [<ffffffff810ef227>] kmem_cache_alloc_trace+0x25/0xba
  [<ffffffff812387e6>] T.492+0x24/0x26
  [<ffffffff81238816>] pci_get_subsys+0x2e/0x73
 sr0: scsi3-mmc drive: 48x/48x writer dvd-ram cd/rw xa/form2 cdda tray
  [<ffffffff8123886c>] pci_get_device+0x11/0x13
  [<ffffffff814082a9>] usb_amd_find_chipset_info+0x3f/0x18a
...

It turns out that this function calls pci_get_device under a spin_lock
with irqs disabled, but the pci_get_device function is only allowed in
preemptible context.

This patch fixes this by using temporary variables in the quirk
algorithm and commiting them later to the struct under the lock. This
moves all pci_get_device() invocations out of the spin_lock and fixes
the lockdep warning.

Cc: stable@kernel.org
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/usb/host/pci-quirks.c |   90 +++++++++++++++++++++++++---------------
 1 files changed, 56 insertions(+), 34 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 1d586d4..c21b073 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -84,65 +84,87 @@ int usb_amd_find_chipset_info(void)
 {
 	u8 rev = 0;
 	unsigned long flags;
+	struct amd_chipset_info info;
+	int ret;
 
 	spin_lock_irqsave(&amd_lock, flags);
 
-	amd_chipset.probe_count++;
 	/* probe only once */
-	if (amd_chipset.probe_count > 1) {
+	if (amd_chipset.probe_count > 0) {
 		spin_unlock_irqrestore(&amd_lock, flags);
 		return amd_chipset.probe_result;
 	}
+	info = amd_chipset;
+	spin_unlock_irqrestore(&amd_lock, flags);
 
-	amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL);
-	if (amd_chipset.smbus_dev) {
-		rev = amd_chipset.smbus_dev->revision;
+	info.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL);
+	if (info.smbus_dev) {
+		rev = info.smbus_dev->revision;
 		if (rev >= 0x40)
-			amd_chipset.sb_type = 1;
+			info.sb_type = 1;
 		else if (rev >= 0x30 && rev <= 0x3b)
-			amd_chipset.sb_type = 3;
+			info.sb_type = 3;
 	} else {
-		amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-							0x780b, NULL);
-		if (!amd_chipset.smbus_dev) {
-			spin_unlock_irqrestore(&amd_lock, flags);
-			return 0;
+		info.smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD,
+						0x780b, NULL);
+		if (!info.smbus_dev) {
+			ret = 0;
+			goto commit;
 		}
-		rev = amd_chipset.smbus_dev->revision;
+
+		rev = info.smbus_dev->revision;
 		if (rev >= 0x11 && rev <= 0x18)
-			amd_chipset.sb_type = 2;
+			info.sb_type = 2;
 	}
 
-	if (amd_chipset.sb_type == 0) {
-		if (amd_chipset.smbus_dev) {
-			pci_dev_put(amd_chipset.smbus_dev);
-			amd_chipset.smbus_dev = NULL;
+	if (info.sb_type == 0) {
+		if (info.smbus_dev) {
+			pci_dev_put(info.smbus_dev);
+			info.smbus_dev = NULL;
 		}
-		spin_unlock_irqrestore(&amd_lock, flags);
-		return 0;
+		ret = 0;
+		goto commit;
 	}
 
-	amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
-	if (amd_chipset.nb_dev) {
-		amd_chipset.nb_type = 1;
+	info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
+	if (info.nb_dev) {
+		info.nb_type = 1;
 	} else {
-		amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-							0x1510, NULL);
-		if (amd_chipset.nb_dev) {
-			amd_chipset.nb_type = 2;
-		} else  {
-			amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-								0x9600, NULL);
-			if (amd_chipset.nb_dev)
-				amd_chipset.nb_type = 3;
+		info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x1510, NULL);
+		if (info.nb_dev) {
+			info.nb_type = 2;
+		} else {
+			info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
+						     0x9600, NULL);
+			if (info.nb_dev)
+				info.nb_type = 3;
 		}
 	}
 
-	amd_chipset.probe_result = 1;
+	ret = info.probe_result = 1;
 	printk(KERN_DEBUG "QUIRK: Enable AMD PLL fix\n");
 
+commit:
+
+	spin_lock_irqsave(&amd_lock, flags);
+	if (amd_chipset.probe_count > 0) {
+		/* race - someone else was faster - drop devices */
+		if (info.nb_dev)
+			pci_dev_put(info.nb_dev);
+		if (info.smbus_dev)
+			pci_dev_put(info.smbus_dev);
+
+		/* Mark that we where here */
+		amd_chipset.probe_count++;
+		ret = amd_chipset.probe_result;
+	} else {
+		/* no race - commit the result */
+		info.probe_count++;
+		amd_chipset = info;
+	}
 	spin_unlock_irqrestore(&amd_lock, flags);
-	return amd_chipset.probe_result;
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);
 
-- 
1.7.1

Re: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[Alan Stern]

On Thu, 7 Apr 2011, Joerg Roedel wrote:

> Booting 2.6.38 on my test machine produces a lockdep warning
> from the usb_amd_find_chipset_info() function:

When posting revised patches, you need to indicate this fact in the
email Subject: line.  For example:

	[PATCH v.2] USB host: Fix lockdep warning in AMD PLL quirk

Otherwise people reading the email won't know which patch it refers to.

Alan Stern

Re: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[Roedel, Joerg]

On Thu, Apr 07, 2011 at 10:35:33AM -0400, Alan Stern wrote:
> On Thu, 7 Apr 2011, Joerg Roedel wrote:
> 
> > Booting 2.6.38 on my test machine produces a lockdep warning
> > from the usb_amd_find_chipset_info() function:
> 
> When posting revised patches, you need to indicate this fact in the
> email Subject: line.  For example:
> 
> 	[PATCH v.2] USB host: Fix lockdep warning in AMD PLL quirk
> 
> Otherwise people reading the email won't know which patch it refers to.

Yes, I usually do this when I post patch series. For single patches I
just send what 'git format-patch' gives me. I should probably edit that
output in the future on single patches.
(This is btw. actually v3, sorry that I forgot to Cc you on v2)

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[Alan Stern]

On Thu, 7 Apr 2011, Roedel, Joerg wrote:

> So we could access the data structure without any locks if we want using
> atomic_t for the probe_count and isoc_reqs members. But as I've seen
> meanwhile the lock still needs to protect the access to the hardware in
> the usb_amd_quirk_pll() function.
> So its probably not worth the work, what do you think?

You might as well use the spinlock.

However, is there a good reason to zero out the amd_chipset members in
usb_amd_dev_put()?  Can these things be added and removed dynamically?  
If they can't then the data should remain valid indefinitely once it
has been probed, and you could call pci_dev_put() at the end of
usb_amd_find_chipset_info().

And if they can, is it valid to call pci_dev_put() in usb_amd_dev_put()  
while holding a spinlock?  You might want to move those calls to the
end of the function.

Alan Stern

Re: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[Joerg Roedel]

On Thu, Apr 07, 2011 at 11:01:02AM -0400, Alan Stern wrote:
> On Thu, 7 Apr 2011, Roedel, Joerg wrote:
> 
> > So we could access the data structure without any locks if we want using
> > atomic_t for the probe_count and isoc_reqs members. But as I've seen
> > meanwhile the lock still needs to protect the access to the hardware in
> > the usb_amd_quirk_pll() function.
> > So its probably not worth the work, what do you think?
> 
> You might as well use the spinlock.

Yes, since we need it anyway for protecting the hardware-access we can
leave everything as is (with the fix).

> However, is there a good reason to zero out the amd_chipset members in
> usb_amd_dev_put()?  Can these things be added and removed dynamically?  
> If they can't then the data should remain valid indefinitely once it
> has been probed, and you could call pci_dev_put() at the end of
> usb_amd_find_chipset_info().

Well, in a real system it is indeed very unlikely that the chipset is
hotplugged. But for formal correctness it is right to hold a reference
to the pci_dev struct as long as we rely on a pointer to it.

> And if they can, is it valid to call pci_dev_put() in usb_amd_dev_put()  
> while holding a spinlock?  You might want to move those calls to the
> end of the function.

I just had a look, pci_dev_put seems to be invalid in atomic context
too.  If the reference count drops to 0 (which is very unlikely for the
chipset devices) the device and its kobject are released. This causes a
uevent to be sent to userspace which does GFP_KERNEL allocations and all
the stuff.
So for formal correctness the pci_dev_put calls need to be moved out of
the spinlock too.

Regards,

	Joerg

[PATCH v4] USB host: Fix lockdep warning in AMD PLL quirk

[Joerg Roedel]

Booting 2.6.38 on my test machine produces a lockdep warning
from the usb_amd_find_chipset_info() function:

 WARNING: at /data/lemmy/linux.trees.git/kernel/lockdep.c:2465 lockdep_trace_alloc+0x95/0xc2()
 Hardware name: Snook
 Modules linked in:
 Pid: 959, comm: work_for_cpu Not tainted 2.6.39-rc2+ #22
 Call Trace:
  [<ffffffff8103c0d4>] warn_slowpath_common+0x80/0x98
  [<ffffffff812387e6>] ? T.492+0x24/0x26
  [<ffffffff8103c101>] warn_slowpath_null+0x15/0x17
  [<ffffffff81068667>] lockdep_trace_alloc+0x95/0xc2
  [<ffffffff810ed9ac>] slab_pre_alloc_hook+0x18/0x3b
  [<ffffffff810ef227>] kmem_cache_alloc_trace+0x25/0xba
  [<ffffffff812387e6>] T.492+0x24/0x26
  [<ffffffff81238816>] pci_get_subsys+0x2e/0x73
  [<ffffffff8123886c>] pci_get_device+0x11/0x13
  [<ffffffff814082a9>] usb_amd_find_chipset_info+0x3f/0x18a
...

It turns out that this function calls pci_get_device under a spin_lock
with irqs disabled, but the pci_get_device function is only allowed in
preemptible context.

This patch fixes the warning by making all data-structure
modifications on temporal storage and commiting this back
into the visible structure at the end. While at it, this
patch also moves the pci_dev_put calls out of the spinlocks
because this function might sleep too.

Cc: stable@kernel.org
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/usb/host/pci-quirks.c |  117 ++++++++++++++++++++++++++---------------
 1 files changed, 74 insertions(+), 43 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 1d586d4..dfc639a 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -84,65 +84,91 @@ int usb_amd_find_chipset_info(void)
 {
 	u8 rev = 0;
 	unsigned long flags;
+	struct amd_chipset_info info;
+	int ret;
 
 	spin_lock_irqsave(&amd_lock, flags);
 
-	amd_chipset.probe_count++;
 	/* probe only once */
-	if (amd_chipset.probe_count > 1) {
+	if (amd_chipset.probe_count > 0) {
 		spin_unlock_irqrestore(&amd_lock, flags);
 		return amd_chipset.probe_result;
 	}
+	info = amd_chipset;
+	spin_unlock_irqrestore(&amd_lock, flags);
 
-	amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL);
-	if (amd_chipset.smbus_dev) {
-		rev = amd_chipset.smbus_dev->revision;
+	info.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL);
+	if (info.smbus_dev) {
+		rev = info.smbus_dev->revision;
 		if (rev >= 0x40)
-			amd_chipset.sb_type = 1;
+			info.sb_type = 1;
 		else if (rev >= 0x30 && rev <= 0x3b)
-			amd_chipset.sb_type = 3;
+			info.sb_type = 3;
 	} else {
-		amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-							0x780b, NULL);
-		if (!amd_chipset.smbus_dev) {
-			spin_unlock_irqrestore(&amd_lock, flags);
-			return 0;
+		info.smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD,
+						0x780b, NULL);
+		if (!info.smbus_dev) {
+			ret = 0;
+			goto commit;
 		}
-		rev = amd_chipset.smbus_dev->revision;
+
+		rev = info.smbus_dev->revision;
 		if (rev >= 0x11 && rev <= 0x18)
-			amd_chipset.sb_type = 2;
+			info.sb_type = 2;
 	}
 
-	if (amd_chipset.sb_type == 0) {
-		if (amd_chipset.smbus_dev) {
-			pci_dev_put(amd_chipset.smbus_dev);
-			amd_chipset.smbus_dev = NULL;
+	if (info.sb_type == 0) {
+		if (info.smbus_dev) {
+			pci_dev_put(info.smbus_dev);
+			info.smbus_dev = NULL;
 		}
-		spin_unlock_irqrestore(&amd_lock, flags);
-		return 0;
+		ret = 0;
+		goto commit;
 	}
 
-	amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
-	if (amd_chipset.nb_dev) {
-		amd_chipset.nb_type = 1;
+	info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
+	if (info.nb_dev) {
+		info.nb_type = 1;
 	} else {
-		amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-							0x1510, NULL);
-		if (amd_chipset.nb_dev) {
-			amd_chipset.nb_type = 2;
-		} else  {
-			amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-								0x9600, NULL);
-			if (amd_chipset.nb_dev)
-				amd_chipset.nb_type = 3;
+		info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x1510, NULL);
+		if (info.nb_dev) {
+			info.nb_type = 2;
+		} else {
+			info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
+						     0x9600, NULL);
+			if (info.nb_dev)
+				info.nb_type = 3;
 		}
 	}
 
-	amd_chipset.probe_result = 1;
+	ret = info.probe_result = 1;
 	printk(KERN_DEBUG "QUIRK: Enable AMD PLL fix\n");
 
-	spin_unlock_irqrestore(&amd_lock, flags);
-	return amd_chipset.probe_result;
+commit:
+
+	spin_lock_irqsave(&amd_lock, flags);
+	if (amd_chipset.probe_count > 0) {
+		/* race - someone else was faster - drop devices */
+
+		/* Mark that we where here */
+		amd_chipset.probe_count++;
+		ret = amd_chipset.probe_result;
+
+		spin_unlock_irqrestore(&amd_lock, flags);
+
+		if (info.nb_dev)
+			pci_dev_put(info.nb_dev);
+		if (info.smbus_dev)
+			pci_dev_put(info.smbus_dev);
+
+	} else {
+		/* no race - commit the result */
+		info.probe_count++;
+		amd_chipset = info;
+		spin_unlock_irqrestore(&amd_lock, flags);
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);
 
@@ -284,8 +310,10 @@ EXPORT_SYMBOL_GPL(usb_amd_quirk_pll_enable);
 
 void usb_amd_dev_put(void)
 {
+	struct pci_dev *nb, *smbus;
 	unsigned long flags;
 
+
 	spin_lock_irqsave(&amd_lock, flags);
 
 	amd_chipset.probe_count--;
@@ -294,20 +322,23 @@ void usb_amd_dev_put(void)
 		return;
 	}
 
-	if (amd_chipset.nb_dev) {
-		pci_dev_put(amd_chipset.nb_dev);
-		amd_chipset.nb_dev = NULL;
-	}
-	if (amd_chipset.smbus_dev) {
-		pci_dev_put(amd_chipset.smbus_dev);
-		amd_chipset.smbus_dev = NULL;
-	}
+	/* save them to pci_dev_put outside of spinlock */
+	nb    = amd_chipset.nb_dev;
+	smbus = amd_chipset.smbus_dev;
+
+	amd_chipset.nb_dev = NULL;
+	amd_chipset.smbus_dev = NULL;
 	amd_chipset.nb_type = 0;
 	amd_chipset.sb_type = 0;
 	amd_chipset.isoc_reqs = 0;
 	amd_chipset.probe_result = 0;
 
 	spin_unlock_irqrestore(&amd_lock, flags);
+
+	if (nb)
+		pci_dev_put(nb);
+	if (smbus)
+		pci_dev_put(amd_chipset.smbus_dev);
 }
 EXPORT_SYMBOL_GPL(usb_amd_dev_put);
 
-- 
1.7.1

Re: [PATCH v4] USB host: Fix lockdep warning in AMD PLL quirk

[Alan Stern]

On Fri, 8 Apr 2011, Joerg Roedel wrote:

> Booting 2.6.38 on my test machine produces a lockdep warning
> from the usb_amd_find_chipset_info() function:
> 
>  WARNING: at /data/lemmy/linux.trees.git/kernel/lockdep.c:2465 lockdep_trace_alloc+0x95/0xc2()
>  Hardware name: Snook
>  Modules linked in:
>  Pid: 959, comm: work_for_cpu Not tainted 2.6.39-rc2+ #22
>  Call Trace:
>   [<ffffffff8103c0d4>] warn_slowpath_common+0x80/0x98
>   [<ffffffff812387e6>] ? T.492+0x24/0x26
>   [<ffffffff8103c101>] warn_slowpath_null+0x15/0x17
>   [<ffffffff81068667>] lockdep_trace_alloc+0x95/0xc2
>   [<ffffffff810ed9ac>] slab_pre_alloc_hook+0x18/0x3b
>   [<ffffffff810ef227>] kmem_cache_alloc_trace+0x25/0xba
>   [<ffffffff812387e6>] T.492+0x24/0x26
>   [<ffffffff81238816>] pci_get_subsys+0x2e/0x73
>   [<ffffffff8123886c>] pci_get_device+0x11/0x13
>   [<ffffffff814082a9>] usb_amd_find_chipset_info+0x3f/0x18a
> ...
> 
> It turns out that this function calls pci_get_device under a spin_lock
> with irqs disabled, but the pci_get_device function is only allowed in
> preemptible context.
> 
> This patch fixes the warning by making all data-structure
> modifications on temporal storage and commiting this back
> into the visible structure at the end. While at it, this
> patch also moves the pci_dev_put calls out of the spinlocks
> because this function might sleep too.

I see only a couple of small flaws...

> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 1d586d4..dfc639a 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -84,65 +84,91 @@ int usb_amd_find_chipset_info(void)
>  {
>  	u8 rev = 0;
>  	unsigned long flags;
> +	struct amd_chipset_info info;
> +	int ret;
>  
>  	spin_lock_irqsave(&amd_lock, flags);
>  
> -	amd_chipset.probe_count++;
>  	/* probe only once */
> -	if (amd_chipset.probe_count > 1) {
> +	if (amd_chipset.probe_count > 0) {

You need to increment probe_count here.

>  		spin_unlock_irqrestore(&amd_lock, flags);
>  		return amd_chipset.probe_result;
>  	}
> +	info = amd_chipset;

What's the point of this line?  You're just going to write over all the 
data in info anyway, so it doesn't matter what amd_chipset contains.  A 
memset would work just as well.

> @@ -284,8 +310,10 @@ EXPORT_SYMBOL_GPL(usb_amd_quirk_pll_enable);
>  
>  void usb_amd_dev_put(void)
>  {
> +	struct pci_dev *nb, *smbus;
>  	unsigned long flags;
>  
> +

Why add an extra blank line?

>  	spin_lock_irqsave(&amd_lock, flags);
>  
>  	amd_chipset.probe_count--;
> @@ -294,20 +322,23 @@ void usb_amd_dev_put(void)
>  		return;
>  	}
>  
> -	if (amd_chipset.nb_dev) {
> -		pci_dev_put(amd_chipset.nb_dev);
> -		amd_chipset.nb_dev = NULL;
> -	}
> -	if (amd_chipset.smbus_dev) {
> -		pci_dev_put(amd_chipset.smbus_dev);
> -		amd_chipset.smbus_dev = NULL;
> -	}
> +	/* save them to pci_dev_put outside of spinlock */
> +	nb    = amd_chipset.nb_dev;
> +	smbus = amd_chipset.smbus_dev;
> +
> +	amd_chipset.nb_dev = NULL;
> +	amd_chipset.smbus_dev = NULL;
>  	amd_chipset.nb_type = 0;
>  	amd_chipset.sb_type = 0;
>  	amd_chipset.isoc_reqs = 0;
>  	amd_chipset.probe_result = 0;

You could use memset instead.  However, in reality it shouldn't be
necessary to set any of these things to 0.

>  
>  	spin_unlock_irqrestore(&amd_lock, flags);
> +
> +	if (nb)
> +		pci_dev_put(nb);
> +	if (smbus)
> +		pci_dev_put(amd_chipset.smbus_dev);
>  }
>  EXPORT_SYMBOL_GPL(usb_amd_dev_put);

Alan Stern

Re: [PATCH v4] USB host: Fix lockdep warning in AMD PLL quirk

[Roedel, Joerg]

On Fri, Apr 08, 2011 at 10:52:59AM -0400, Alan Stern wrote:
> On Fri, 8 Apr 2011, Joerg Roedel wrote:
> > -	amd_chipset.probe_count++;
> >  	/* probe only once */
> > -	if (amd_chipset.probe_count > 1) {
> > +	if (amd_chipset.probe_count > 0) {
> 
> You need to increment probe_count here.

Argh, right.

> 
> >  		spin_unlock_irqrestore(&amd_lock, flags);
> >  		return amd_chipset.probe_result;
> >  	}
> > +	info = amd_chipset;
> 
> What's the point of this line?  You're just going to write over all the 
> data in info anyway, so it doesn't matter what amd_chipset contains.  A 
> memset would work just as well.

info is on-stack and needs to be initialized somehow because the
following code does not touch the whole data structure.

> 
> > @@ -284,8 +310,10 @@ EXPORT_SYMBOL_GPL(usb_amd_quirk_pll_enable);
> >  
> >  void usb_amd_dev_put(void)
> >  {
> > +	struct pci_dev *nb, *smbus;
> >  	unsigned long flags;
> >  
> > +
> 
> Why add an extra blank line?

Left-over from editing.

> 
> >  	spin_lock_irqsave(&amd_lock, flags);
> >  
> >  	amd_chipset.probe_count--;
> > @@ -294,20 +322,23 @@ void usb_amd_dev_put(void)
> >  		return;
> >  	}
> >  
> > -	if (amd_chipset.nb_dev) {
> > -		pci_dev_put(amd_chipset.nb_dev);
> > -		amd_chipset.nb_dev = NULL;
> > -	}
> > -	if (amd_chipset.smbus_dev) {
> > -		pci_dev_put(amd_chipset.smbus_dev);
> > -		amd_chipset.smbus_dev = NULL;
> > -	}
> > +	/* save them to pci_dev_put outside of spinlock */
> > +	nb    = amd_chipset.nb_dev;
> > +	smbus = amd_chipset.smbus_dev;
> > +
> > +	amd_chipset.nb_dev = NULL;
> > +	amd_chipset.smbus_dev = NULL;
> >  	amd_chipset.nb_type = 0;
> >  	amd_chipset.sb_type = 0;
> >  	amd_chipset.isoc_reqs = 0;
> >  	amd_chipset.probe_result = 0;
> 
> You could use memset instead.  However, in reality it shouldn't be
> necessary to set any of these things to 0.

Well, I left this code as it was before because this is meant as a fix
and I tried to keep it small. For a fix it is already really big...
But that can certainly be changed in a follow-on patch.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH v4] USB host: Fix lockdep warning in AMD PLL quirk

[Alan Stern]

On Fri, 8 Apr 2011, Roedel, Joerg wrote:

> > > +	info = amd_chipset;
> > 
> > What's the point of this line?  You're just going to write over all the 
> > data in info anyway, so it doesn't matter what amd_chipset contains.  A 
> > memset would work just as well.
> 
> info is on-stack and needs to be initialized somehow because the
> following code does not touch the whole data structure.

Then just use memset.

Alan Stern

Re: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[Borislav Petkov]

On Thu, Apr 07, 2011 at 03:14:04PM +0200, Joerg Roedel wrote:
> Booting 2.6.38 on my test machine produces a lockdep warning

You mean 2.6.39-rc2 right? I'm asking because I hit the same warning
with 39-rc2 now too but it didn't appear with .38 - so it has to have
snuck in after the merge window. If so, you don't need the stable tag.

Also, is this the final version wrt code design so that I can test too?

Thanks.

> from the usb_amd_find_chipset_info() function:
> 
>  WARNING: at /data/lemmy/linux.trees.git/kernel/lockdep.c:2465 lockdep_trace_alloc+0x95/0xc2()
>  Hardware name: Snook
>  Modules linked in:
>  Pid: 959, comm: work_for_cpu Not tainted 2.6.39-rc2+ #22
>  Call Trace:
>   [<ffffffff8103c0d4>] warn_slowpath_common+0x80/0x98
>   [<ffffffff812387e6>] ? T.492+0x24/0x26
>   [<ffffffff8103c101>] warn_slowpath_null+0x15/0x17
>   [<ffffffff81068667>] lockdep_trace_alloc+0x95/0xc2
>   [<ffffffff810ed9ac>] slab_pre_alloc_hook+0x18/0x3b
>   [<ffffffff810ef227>] kmem_cache_alloc_trace+0x25/0xba
>   [<ffffffff812387e6>] T.492+0x24/0x26
>   [<ffffffff81238816>] pci_get_subsys+0x2e/0x73
>  sr0: scsi3-mmc drive: 48x/48x writer dvd-ram cd/rw xa/form2 cdda tray
>   [<ffffffff8123886c>] pci_get_device+0x11/0x13
>   [<ffffffff814082a9>] usb_amd_find_chipset_info+0x3f/0x18a
> ...
> 
> It turns out that this function calls pci_get_device under a spin_lock
> with irqs disabled, but the pci_get_device function is only allowed in
> preemptible context.
> 
> This patch fixes this by using temporary variables in the quirk
> algorithm and commiting them later to the struct under the lock. This
> moves all pci_get_device() invocations out of the spin_lock and fixes
> the lockdep warning.
> 
> Cc: stable@kernel.org
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  drivers/usb/host/pci-quirks.c |   90 +++++++++++++++++++++++++---------------
>  1 files changed, 56 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 1d586d4..c21b073 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -84,65 +84,87 @@ int usb_amd_find_chipset_info(void)
>  {
>  	u8 rev = 0;
>  	unsigned long flags;
> +	struct amd_chipset_info info;
> +	int ret;
>  
>  	spin_lock_irqsave(&amd_lock, flags);
>  
> -	amd_chipset.probe_count++;
>  	/* probe only once */
> -	if (amd_chipset.probe_count > 1) {
> +	if (amd_chipset.probe_count > 0) {
>  		spin_unlock_irqrestore(&amd_lock, flags);
>  		return amd_chipset.probe_result;
>  	}
> +	info = amd_chipset;
> +	spin_unlock_irqrestore(&amd_lock, flags);
>  
> -	amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL);
> -	if (amd_chipset.smbus_dev) {
> -		rev = amd_chipset.smbus_dev->revision;
> +	info.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL);
> +	if (info.smbus_dev) {
> +		rev = info.smbus_dev->revision;
>  		if (rev >= 0x40)
> -			amd_chipset.sb_type = 1;
> +			info.sb_type = 1;
>  		else if (rev >= 0x30 && rev <= 0x3b)
> -			amd_chipset.sb_type = 3;
> +			info.sb_type = 3;
>  	} else {
> -		amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD,
> -							0x780b, NULL);
> -		if (!amd_chipset.smbus_dev) {
> -			spin_unlock_irqrestore(&amd_lock, flags);
> -			return 0;
> +		info.smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD,
> +						0x780b, NULL);
> +		if (!info.smbus_dev) {
> +			ret = 0;
> +			goto commit;
>  		}
> -		rev = amd_chipset.smbus_dev->revision;
> +
> +		rev = info.smbus_dev->revision;
>  		if (rev >= 0x11 && rev <= 0x18)
> -			amd_chipset.sb_type = 2;
> +			info.sb_type = 2;
>  	}
>  
> -	if (amd_chipset.sb_type == 0) {
> -		if (amd_chipset.smbus_dev) {
> -			pci_dev_put(amd_chipset.smbus_dev);
> -			amd_chipset.smbus_dev = NULL;
> +	if (info.sb_type == 0) {
> +		if (info.smbus_dev) {
> +			pci_dev_put(info.smbus_dev);
> +			info.smbus_dev = NULL;
>  		}
> -		spin_unlock_irqrestore(&amd_lock, flags);
> -		return 0;
> +		ret = 0;
> +		goto commit;
>  	}
>  
> -	amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
> -	if (amd_chipset.nb_dev) {
> -		amd_chipset.nb_type = 1;
> +	info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
> +	if (info.nb_dev) {
> +		info.nb_type = 1;
>  	} else {
> -		amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
> -							0x1510, NULL);
> -		if (amd_chipset.nb_dev) {
> -			amd_chipset.nb_type = 2;
> -		} else  {
> -			amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
> -								0x9600, NULL);
> -			if (amd_chipset.nb_dev)
> -				amd_chipset.nb_type = 3;
> +		info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x1510, NULL);
> +		if (info.nb_dev) {
> +			info.nb_type = 2;
> +		} else {
> +			info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
> +						     0x9600, NULL);
> +			if (info.nb_dev)
> +				info.nb_type = 3;
>  		}
>  	}
>  
> -	amd_chipset.probe_result = 1;
> +	ret = info.probe_result = 1;
>  	printk(KERN_DEBUG "QUIRK: Enable AMD PLL fix\n");
>  
> +commit:
> +
> +	spin_lock_irqsave(&amd_lock, flags);
> +	if (amd_chipset.probe_count > 0) {
> +		/* race - someone else was faster - drop devices */
> +		if (info.nb_dev)
> +			pci_dev_put(info.nb_dev);
> +		if (info.smbus_dev)
> +			pci_dev_put(info.smbus_dev);
> +
> +		/* Mark that we where here */
> +		amd_chipset.probe_count++;
> +		ret = amd_chipset.probe_result;
> +	} else {
> +		/* no race - commit the result */
> +		info.probe_count++;
> +		amd_chipset = info;
> +	}
>  	spin_unlock_irqrestore(&amd_lock, flags);
> -	return amd_chipset.probe_result;
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);
>  
> -- 
> 1.7.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Regards/Gruss,
    Boris.

Re: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

[Roedel, Joerg]

On Mon, Apr 11, 2011 at 02:26:00AM -0400, Borislav Petkov wrote:
> On Thu, Apr 07, 2011 at 03:14:04PM +0200, Joerg Roedel wrote:
> > Booting 2.6.38 on my test machine produces a lockdep warning
> 
> You mean 2.6.39-rc2 right? I'm asking because I hit the same warning
> with 39-rc2 now too but it didn't appear with .38 - so it has to have
> snuck in after the merge window. If so, you don't need the stable tag.
> 
> Also, is this the final version wrt code design so that I can test too?

I hit this in the KVM branch and git-describe told me it is 2.6.38
based. So I guess this happens on 2.6.38 too, but I havn't tested plain
2.6.38. I do this and sent out a new version shortly which you can use
then. The new version addresses Alan Stern's latest comments.

	Joerg

[PATCH v5] USB host: Fix lockdep warning in AMD PLL quirk

[Roedel, Joerg]

On Mon, Apr 11, 2011 at 02:26:00AM -0400, Borislav Petkov wrote:
> You mean 2.6.39-rc2 right? I'm asking because I hit the same warning
> with 39-rc2 now too but it didn't appear with .38 - so it has to have
> snuck in after the merge window. If so, you don't need the stable tag.

You were right, just tested plain 2.6.38 and it doesn't happen. So I
removed the stable tag. Here is the updated patch.

>From 6b9a9018cf0b847dcd906c269d663668e67a097d Mon Sep 17 00:00:00 2001
From: Joerg Roedel <joerg.roedel@amd.com>
Date: Wed, 6 Apr 2011 13:07:53 +0200
Subject: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

Booting 2.6.38 on my test machine produces a lockdep warning
from the usb_amd_find_chipset_info() function:

 WARNING: at /data/lemmy/linux.trees.git/kernel/lockdep.c:2465 lockdep_trace_alloc+0x95/0xc2()
 Hardware name: Snook
 Modules linked in:
 Pid: 959, comm: work_for_cpu Not tainted 2.6.39-rc2+ #22
 Call Trace:
  [<ffffffff8103c0d4>] warn_slowpath_common+0x80/0x98
  [<ffffffff812387e6>] ? T.492+0x24/0x26
  [<ffffffff8103c101>] warn_slowpath_null+0x15/0x17
  [<ffffffff81068667>] lockdep_trace_alloc+0x95/0xc2
  [<ffffffff810ed9ac>] slab_pre_alloc_hook+0x18/0x3b
  [<ffffffff810ef227>] kmem_cache_alloc_trace+0x25/0xba
  [<ffffffff812387e6>] T.492+0x24/0x26
  [<ffffffff81238816>] pci_get_subsys+0x2e/0x73
  [<ffffffff8123886c>] pci_get_device+0x11/0x13
  [<ffffffff814082a9>] usb_amd_find_chipset_info+0x3f/0x18a
...

It turns out that this function calls pci_get_device under a spin_lock
with irqs disabled, but the pci_get_device function is only allowed in
preemptible context.

This patch fixes the warning by making all data-structure
modifications on temporal storage and commiting this back
into the visible structure at the end. While at it, this
patch also moves the pci_dev_put calls out of the spinlocks
because this function might sleep too.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/usb/host/pci-quirks.c |  117 ++++++++++++++++++++++++++---------------
 1 files changed, 74 insertions(+), 43 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 1d586d4..c6eb69c 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -84,65 +84,92 @@ int usb_amd_find_chipset_info(void)
 {
 	u8 rev = 0;
 	unsigned long flags;
+	struct amd_chipset_info info;
+	int ret;
 
 	spin_lock_irqsave(&amd_lock, flags);
 
-	amd_chipset.probe_count++;
 	/* probe only once */
-	if (amd_chipset.probe_count > 1) {
+	if (amd_chipset.probe_count > 0) {
+		amd_chipset.probe_count++;
 		spin_unlock_irqrestore(&amd_lock, flags);
 		return amd_chipset.probe_result;
 	}
+	memset(&info, 0, sizeof(info));
+	spin_unlock_irqrestore(&amd_lock, flags);
 
-	amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL);
-	if (amd_chipset.smbus_dev) {
-		rev = amd_chipset.smbus_dev->revision;
+	info.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL);
+	if (info.smbus_dev) {
+		rev = info.smbus_dev->revision;
 		if (rev >= 0x40)
-			amd_chipset.sb_type = 1;
+			info.sb_type = 1;
 		else if (rev >= 0x30 && rev <= 0x3b)
-			amd_chipset.sb_type = 3;
+			info.sb_type = 3;
 	} else {
-		amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-							0x780b, NULL);
-		if (!amd_chipset.smbus_dev) {
-			spin_unlock_irqrestore(&amd_lock, flags);
-			return 0;
+		info.smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD,
+						0x780b, NULL);
+		if (!info.smbus_dev) {
+			ret = 0;
+			goto commit;
 		}
-		rev = amd_chipset.smbus_dev->revision;
+
+		rev = info.smbus_dev->revision;
 		if (rev >= 0x11 && rev <= 0x18)
-			amd_chipset.sb_type = 2;
+			info.sb_type = 2;
 	}
 
-	if (amd_chipset.sb_type == 0) {
-		if (amd_chipset.smbus_dev) {
-			pci_dev_put(amd_chipset.smbus_dev);
-			amd_chipset.smbus_dev = NULL;
+	if (info.sb_type == 0) {
+		if (info.smbus_dev) {
+			pci_dev_put(info.smbus_dev);
+			info.smbus_dev = NULL;
 		}
-		spin_unlock_irqrestore(&amd_lock, flags);
-		return 0;
+		ret = 0;
+		goto commit;
 	}
 
-	amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
-	if (amd_chipset.nb_dev) {
-		amd_chipset.nb_type = 1;
+	info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
+	if (info.nb_dev) {
+		info.nb_type = 1;
 	} else {
-		amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-							0x1510, NULL);
-		if (amd_chipset.nb_dev) {
-			amd_chipset.nb_type = 2;
-		} else  {
-			amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-								0x9600, NULL);
-			if (amd_chipset.nb_dev)
-				amd_chipset.nb_type = 3;
+		info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x1510, NULL);
+		if (info.nb_dev) {
+			info.nb_type = 2;
+		} else {
+			info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
+						     0x9600, NULL);
+			if (info.nb_dev)
+				info.nb_type = 3;
 		}
 	}
 
-	amd_chipset.probe_result = 1;
+	ret = info.probe_result = 1;
 	printk(KERN_DEBUG "QUIRK: Enable AMD PLL fix\n");
 
-	spin_unlock_irqrestore(&amd_lock, flags);
-	return amd_chipset.probe_result;
+commit:
+
+	spin_lock_irqsave(&amd_lock, flags);
+	if (amd_chipset.probe_count > 0) {
+		/* race - someone else was faster - drop devices */
+
+		/* Mark that we where here */
+		amd_chipset.probe_count++;
+		ret = amd_chipset.probe_result;
+
+		spin_unlock_irqrestore(&amd_lock, flags);
+
+		if (info.nb_dev)
+			pci_dev_put(info.nb_dev);
+		if (info.smbus_dev)
+			pci_dev_put(info.smbus_dev);
+
+	} else {
+		/* no race - commit the result */
+		info.probe_count++;
+		amd_chipset = info;
+		spin_unlock_irqrestore(&amd_lock, flags);
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);
 
@@ -284,6 +311,7 @@ EXPORT_SYMBOL_GPL(usb_amd_quirk_pll_enable);
 
 void usb_amd_dev_put(void)
 {
+	struct pci_dev *nb, *smbus;
 	unsigned long flags;
 
 	spin_lock_irqsave(&amd_lock, flags);
@@ -294,20 +322,23 @@ void usb_amd_dev_put(void)
 		return;
 	}
 
-	if (amd_chipset.nb_dev) {
-		pci_dev_put(amd_chipset.nb_dev);
-		amd_chipset.nb_dev = NULL;
-	}
-	if (amd_chipset.smbus_dev) {
-		pci_dev_put(amd_chipset.smbus_dev);
-		amd_chipset.smbus_dev = NULL;
-	}
+	/* save them to pci_dev_put outside of spinlock */
+	nb    = amd_chipset.nb_dev;
+	smbus = amd_chipset.smbus_dev;
+
+	amd_chipset.nb_dev = NULL;
+	amd_chipset.smbus_dev = NULL;
 	amd_chipset.nb_type = 0;
 	amd_chipset.sb_type = 0;
 	amd_chipset.isoc_reqs = 0;
 	amd_chipset.probe_result = 0;
 
 	spin_unlock_irqrestore(&amd_lock, flags);
+
+	if (nb)
+		pci_dev_put(nb);
+	if (smbus)
+		pci_dev_put(amd_chipset.smbus_dev);
 }
 EXPORT_SYMBOL_GPL(usb_amd_dev_put);
 
-- 
1.7.1


-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH v5] USB host: Fix lockdep warning in AMD PLL quirk

[Sergei Shtylyov]

Hello.

On 11-04-2011 10:59, Roedel, Joerg wrote:

>> You mean 2.6.39-rc2 right? I'm asking because I hit the same warning
>> with 39-rc2 now too but it didn't appear with .38 - so it has to have
>> snuck in after the merge window. If so, you don't need the stable tag.

> You were right, just tested plain 2.6.38 and it doesn't happen. So I
> removed the stable tag. Here is the updated patch.

>  From 6b9a9018cf0b847dcd906c269d663668e67a097d Mon Sep 17 00:00:00 2001
> From: Joerg Roedel<joerg.roedel@amd.com>
> Date: Wed, 6 Apr 2011 13:07:53 +0200
> Subject: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

> Booting 2.6.38 on my test machine produces a lockdep warning

    You've just said that this doesn't happen with plain 2.6.38...

> from the usb_amd_find_chipset_info() function:

>   WARNING: at /data/lemmy/linux.trees.git/kernel/lockdep.c:2465 lockdep_trace_alloc+0x95/0xc2()
>   Hardware name: Snook
>   Modules linked in:
>   Pid: 959, comm: work_for_cpu Not tainted 2.6.39-rc2+ #22
>   Call Trace:
>    [<ffffffff8103c0d4>] warn_slowpath_common+0x80/0x98
>    [<ffffffff812387e6>] ? T.492+0x24/0x26
>    [<ffffffff8103c101>] warn_slowpath_null+0x15/0x17
>    [<ffffffff81068667>] lockdep_trace_alloc+0x95/0xc2
>    [<ffffffff810ed9ac>] slab_pre_alloc_hook+0x18/0x3b
>    [<ffffffff810ef227>] kmem_cache_alloc_trace+0x25/0xba
>    [<ffffffff812387e6>] T.492+0x24/0x26
>    [<ffffffff81238816>] pci_get_subsys+0x2e/0x73
>    [<ffffffff8123886c>] pci_get_device+0x11/0x13
>    [<ffffffff814082a9>] usb_amd_find_chipset_info+0x3f/0x18a
> ...

> It turns out that this function calls pci_get_device under a spin_lock
> with irqs disabled, but the pci_get_device function is only allowed in
> preemptible context.

> This patch fixes the warning by making all data-structure
> modifications on temporal storage and commiting this back
> into the visible structure at the end. While at it, this
> patch also moves the pci_dev_put calls out of the spinlocks
> because this function might sleep too.

> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>   drivers/usb/host/pci-quirks.c |  117 ++++++++++++++++++++++++++---------------
>   1 files changed, 74 insertions(+), 43 deletions(-)

> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 1d586d4..c6eb69c 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
[...]
> @@ -294,20 +322,23 @@ void usb_amd_dev_put(void)
>   		return;
>   	}
>
> -	if (amd_chipset.nb_dev) {
> -		pci_dev_put(amd_chipset.nb_dev);
> -		amd_chipset.nb_dev = NULL;
> -	}
> -	if (amd_chipset.smbus_dev) {
> -		pci_dev_put(amd_chipset.smbus_dev);
> -		amd_chipset.smbus_dev = NULL;
> -	}
> +	/* save them to pci_dev_put outside of spinlock */
> +	nb    = amd_chipset.nb_dev;
> +	smbus = amd_chipset.smbus_dev;
> +
> +	amd_chipset.nb_dev = NULL;
> +	amd_chipset.smbus_dev = NULL;
>   	amd_chipset.nb_type = 0;
>   	amd_chipset.sb_type = 0;
>   	amd_chipset.isoc_reqs = 0;
>   	amd_chipset.probe_result = 0;
>
>   	spin_unlock_irqrestore(&amd_lock, flags);
> +
> +	if (nb)
> +		pci_dev_put(nb);
> +	if (smbus)
> +		pci_dev_put(amd_chipset.smbus_dev);

    Haven't you just set this to NULL?

WBR, Sergei

Re: [PATCH v5] USB host: Fix lockdep warning in AMD PLL quirk

[Alan Stern]

On Mon, 11 Apr 2011, Roedel, Joerg wrote:

> On Mon, Apr 11, 2011 at 02:26:00AM -0400, Borislav Petkov wrote:
> > You mean 2.6.39-rc2 right? I'm asking because I hit the same warning
> > with 39-rc2 now too but it didn't appear with .38 - so it has to have
> > snuck in after the merge window. If so, you don't need the stable tag.
> 
> You were right, just tested plain 2.6.38 and it doesn't happen. So I
> removed the stable tag. Here is the updated patch.

...

> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -84,65 +84,92 @@ int usb_amd_find_chipset_info(void)
>  {

...

> +commit:
> +
> +	spin_lock_irqsave(&amd_lock, flags);
> +	if (amd_chipset.probe_count > 0) {
> +		/* race - someone else was faster - drop devices */
> +
> +		/* Mark that we where here */
> +		amd_chipset.probe_count++;

This line should be moved above the "if" statement, since you always 
want to increment the count.

> +		ret = amd_chipset.probe_result;
> +
> +		spin_unlock_irqrestore(&amd_lock, flags);
> +
> +		if (info.nb_dev)
> +			pci_dev_put(info.nb_dev);
> +		if (info.smbus_dev)
> +			pci_dev_put(info.smbus_dev);
> +
> +	} else {
> +		/* no race - commit the result */
> +		info.probe_count++;

This isn't right, because info.probe_count was initialized to 0.  Maybe 
amd_chipset.probe_count should be made into a separate variable, not a 
part of the structure, like amd_lock.

> +		amd_chipset = info;
> +		spin_unlock_irqrestore(&amd_lock, flags);
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);

Alan Stern

Re: [PATCH v5] USB host: Fix lockdep warning in AMD PLL quirk

[Roedel, Joerg]

On Mon, Apr 11, 2011 at 11:49:30AM -0400, Alan Stern wrote:
> On Mon, 11 Apr 2011, Roedel, Joerg wrote:
> 
> > On Mon, Apr 11, 2011 at 02:26:00AM -0400, Borislav Petkov wrote:
> > > You mean 2.6.39-rc2 right? I'm asking because I hit the same warning
> > > with 39-rc2 now too but it didn't appear with .38 - so it has to have
> > > snuck in after the merge window. If so, you don't need the stable tag.
> > 
> > You were right, just tested plain 2.6.38 and it doesn't happen. So I
> > removed the stable tag. Here is the updated patch.
> 
> ...
> 
> > --- a/drivers/usb/host/pci-quirks.c
> > +++ b/drivers/usb/host/pci-quirks.c
> > @@ -84,65 +84,92 @@ int usb_amd_find_chipset_info(void)
> >  {
> 
> ...
> 
> > +commit:
> > +
> > +	spin_lock_irqsave(&amd_lock, flags);
> > +	if (amd_chipset.probe_count > 0) {
> > +		/* race - someone else was faster - drop devices */
> > +
> > +		/* Mark that we where here */
> > +		amd_chipset.probe_count++;
> 
> This line should be moved above the "if" statement, since you always 
> want to increment the count.

No, probe_count can't be incremented here because the probe is not
finished yet. If another thread jumps in after the lock is released and
detects probe_count > 0 while the probe hasn't happened the quirk will
fail. So we need to make sure that amd_chipset.probe_count does not
become > 0 before the probe is finished.

> 
> > +		ret = amd_chipset.probe_result;
> > +
> > +		spin_unlock_irqrestore(&amd_lock, flags);
> > +
> > +		if (info.nb_dev)
> > +			pci_dev_put(info.nb_dev);
> > +		if (info.smbus_dev)
> > +			pci_dev_put(info.smbus_dev);
> > +
> > +	} else {
> > +		/* no race - commit the result */
> > +		info.probe_count++;
> 
> This isn't right, because info.probe_count was initialized to 0.  Maybe 
> amd_chipset.probe_count should be made into a separate variable, not a 
> part of the structure, like amd_lock.

The purpose of the struct is structuring of data. In theory all of its
members could be turned into global variables. The amd_lock is different
because it does not only protect the struct but also access to the
hardware while the quirk is applied/unapplied.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH v5] USB host: Fix lockdep warning in AMD PLL quirk

[Alan Stern]

On Mon, 11 Apr 2011, Roedel, Joerg wrote:

> > > +commit:
> > > +
> > > +	spin_lock_irqsave(&amd_lock, flags);
> > > +	if (amd_chipset.probe_count > 0) {
> > > +		/* race - someone else was faster - drop devices */
> > > +
> > > +		/* Mark that we where here */
> > > +		amd_chipset.probe_count++;
> > 
> > This line should be moved above the "if" statement, since you always 
> > want to increment the count.
> 
> No, probe_count can't be incremented here because the probe is not
> finished yet.

I don't follow you.  Sure it is finished; this is the "commit" part of 
the probe.

>  If another thread jumps in after the lock is released and
> detects probe_count > 0 while the probe hasn't happened the quirk will
> fail. So we need to make sure that amd_chipset.probe_count does not
> become > 0 before the probe is finished.

I meant the increment should be done before the "if" statement but
after the spin_lock_irqsave().  That way nobody else can jump in at the
wrong time.

> > > +		ret = amd_chipset.probe_result;
> > > +
> > > +		spin_unlock_irqrestore(&amd_lock, flags);
> > > +
> > > +		if (info.nb_dev)
> > > +			pci_dev_put(info.nb_dev);
> > > +		if (info.smbus_dev)
> > > +			pci_dev_put(info.smbus_dev);
> > > +
> > > +	} else {
> > > +		/* no race - commit the result */
> > > +		info.probe_count++;
> > 
> > This isn't right, because info.probe_count was initialized to 0.  Maybe 
> > amd_chipset.probe_count should be made into a separate variable, not a 
> > part of the structure, like amd_lock.
> 
> The purpose of the struct is structuring of data. In theory all of its
> members could be turned into global variables. The amd_lock is different
> because it does not only protect the struct but also access to the
> hardware while the quirk is applied/unapplied.

Do it however you prefer.  But as it stands now, the patch is wrong.

Alan Stern

Re: [PATCH v5] USB host: Fix lockdep warning in AMD PLL quirk

[Roedel, Joerg]

On Mon, Apr 11, 2011 at 12:25:07PM -0400, Alan Stern wrote:
> On Mon, 11 Apr 2011, Roedel, Joerg wrote:
> 
> > > > +commit:
> > > > +
> > > > +	spin_lock_irqsave(&amd_lock, flags);
> > > > +	if (amd_chipset.probe_count > 0) {
> > > > +		/* race - someone else was faster - drop devices */
> > > > +
> > > > +		/* Mark that we where here */
> > > > +		amd_chipset.probe_count++;
> > > 
> > > This line should be moved above the "if" statement, since you always 
> > > want to increment the count.
> > 
> > No, probe_count can't be incremented here because the probe is not
> > finished yet.
> 
> I don't follow you.  Sure it is finished; this is the "commit" part of 
> the probe.

Nevermind, I thought you were refering to the spin-locked part at the
beginning of the function.

> >  If another thread jumps in after the lock is released and
> > detects probe_count > 0 while the probe hasn't happened the quirk will
> > fail. So we need to make sure that amd_chipset.probe_count does not
> > become > 0 before the probe is finished.
> 
> I meant the increment should be done before the "if" statement but
> after the spin_lock_irqsave().  That way nobody else can jump in at the
> wrong time.

In the real commit case the 

	amd_chipset = info;

line will overwrite the increment if the probe is done before the
if-statement. So incrementing amd_chipset.probe_count directly only
matters for the case where we detected a race.

> > > > +		ret = amd_chipset.probe_result;
> > > > +
> > > > +		spin_unlock_irqrestore(&amd_lock, flags);
> > > > +
> > > > +		if (info.nb_dev)
> > > > +			pci_dev_put(info.nb_dev);
> > > > +		if (info.smbus_dev)
> > > > +			pci_dev_put(info.smbus_dev);
> > > > +
> > > > +	} else {
> > > > +		/* no race - commit the result */
> > > > +		info.probe_count++;
> > > 
> > > This isn't right, because info.probe_count was initialized to 0.  Maybe 
> > > amd_chipset.probe_count should be made into a separate variable, not a 
> > > part of the structure, like amd_lock.
> > 
> > The purpose of the struct is structuring of data. In theory all of its
> > members could be turned into global variables. The amd_lock is different
> > because it does not only protect the struct but also access to the
> > hardware while the quirk is applied/unapplied.
> 
> Do it however you prefer.  But as it stands now, the patch is wrong.

Hmm, I see how it can be done differently, but no real bug.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH v5] USB host: Fix lockdep warning in AMD PLL quirk

[Alan Stern]

On Mon, 11 Apr 2011, Roedel, Joerg wrote:

> > > > > +		ret = amd_chipset.probe_result;
> > > > > +
> > > > > +		spin_unlock_irqrestore(&amd_lock, flags);
> > > > > +
> > > > > +		if (info.nb_dev)
> > > > > +			pci_dev_put(info.nb_dev);
> > > > > +		if (info.smbus_dev)
> > > > > +			pci_dev_put(info.smbus_dev);
> > > > > +
> > > > > +	} else {
> > > > > +		/* no race - commit the result */
> > > > > +		info.probe_count++;
> > > > 
> > > > This isn't right, because info.probe_count was initialized to 0.  Maybe 
> > > > amd_chipset.probe_count should be made into a separate variable, not a 
> > > > part of the structure, like amd_lock.
> > > 
> > > The purpose of the struct is structuring of data. In theory all of its
> > > members could be turned into global variables. The amd_lock is different
> > > because it does not only protect the struct but also access to the
> > > hardware while the quirk is applied/unapplied.
> > 
> > Do it however you prefer.  But as it stands now, the patch is wrong.
> 
> Hmm, I see how it can be done differently, but no real bug.

Never mind, you're right.  In the no-race case, the initial count is 
always going to be 0, and so incrementing info.probe_count is the right 
thing to do.

Alan Stern

[PATCH v6] USB host: Fix lockdep warning in AMD PLL quirk

[Roedel, Joerg]

On Mon, Apr 11, 2011 at 01:05:41PM -0400, Alan Stern wrote:
> On Mon, 11 Apr 2011, Roedel, Joerg wrote:

> > Hmm, I see how it can be done differently, but no real bug.
> 
> Never mind, you're right.  In the no-race case, the initial count is 
> always going to be 0, and so incrementing info.probe_count is the right 
> thing to do.

Okay, cool. So now that we have settled on this I removed the last
occurence of '2.6.38' from the changelog and here is the result. No
code changes since v5.

>From a6e00d237f20e940afdbcaafb27117fe8730466f Mon Sep 17 00:00:00 2001
From: Joerg Roedel <joerg.roedel@amd.com>
Date: Wed, 6 Apr 2011 13:07:53 +0200
Subject: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

Booting latest kernel on my test machine produces a lockdep
warning from the usb_amd_find_chipset_info() function:

 WARNING: at /data/lemmy/linux.trees.git/kernel/lockdep.c:2465 lockdep_trace_alloc+0x95/0xc2()
 Hardware name: Snook
 Modules linked in:
 Pid: 959, comm: work_for_cpu Not tainted 2.6.39-rc2+ #22
 Call Trace:
  [<ffffffff8103c0d4>] warn_slowpath_common+0x80/0x98
  [<ffffffff812387e6>] ? T.492+0x24/0x26
  [<ffffffff8103c101>] warn_slowpath_null+0x15/0x17
  [<ffffffff81068667>] lockdep_trace_alloc+0x95/0xc2
  [<ffffffff810ed9ac>] slab_pre_alloc_hook+0x18/0x3b
  [<ffffffff810ef227>] kmem_cache_alloc_trace+0x25/0xba
  [<ffffffff812387e6>] T.492+0x24/0x26
  [<ffffffff81238816>] pci_get_subsys+0x2e/0x73
  [<ffffffff8123886c>] pci_get_device+0x11/0x13
  [<ffffffff814082a9>] usb_amd_find_chipset_info+0x3f/0x18a
...

It turns out that this function calls pci_get_device under a spin_lock
with irqs disabled, but the pci_get_device function is only allowed in
preemptible context.

This patch fixes the warning by making all data-structure
modifications on temporal storage and commiting this back
into the visible structure at the end. While at it, this
patch also moves the pci_dev_put calls out of the spinlocks
because this function might sleep too.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/usb/host/pci-quirks.c |  117 ++++++++++++++++++++++++++---------------
 1 files changed, 74 insertions(+), 43 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 1d586d4..c6eb69c 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -84,65 +84,92 @@ int usb_amd_find_chipset_info(void)
 {
 	u8 rev = 0;
 	unsigned long flags;
+	struct amd_chipset_info info;
+	int ret;
 
 	spin_lock_irqsave(&amd_lock, flags);
 
-	amd_chipset.probe_count++;
 	/* probe only once */
-	if (amd_chipset.probe_count > 1) {
+	if (amd_chipset.probe_count > 0) {
+		amd_chipset.probe_count++;
 		spin_unlock_irqrestore(&amd_lock, flags);
 		return amd_chipset.probe_result;
 	}
+	memset(&info, 0, sizeof(info));
+	spin_unlock_irqrestore(&amd_lock, flags);
 
-	amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL);
-	if (amd_chipset.smbus_dev) {
-		rev = amd_chipset.smbus_dev->revision;
+	info.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL);
+	if (info.smbus_dev) {
+		rev = info.smbus_dev->revision;
 		if (rev >= 0x40)
-			amd_chipset.sb_type = 1;
+			info.sb_type = 1;
 		else if (rev >= 0x30 && rev <= 0x3b)
-			amd_chipset.sb_type = 3;
+			info.sb_type = 3;
 	} else {
-		amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-							0x780b, NULL);
-		if (!amd_chipset.smbus_dev) {
-			spin_unlock_irqrestore(&amd_lock, flags);
-			return 0;
+		info.smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD,
+						0x780b, NULL);
+		if (!info.smbus_dev) {
+			ret = 0;
+			goto commit;
 		}
-		rev = amd_chipset.smbus_dev->revision;
+
+		rev = info.smbus_dev->revision;
 		if (rev >= 0x11 && rev <= 0x18)
-			amd_chipset.sb_type = 2;
+			info.sb_type = 2;
 	}
 
-	if (amd_chipset.sb_type == 0) {
-		if (amd_chipset.smbus_dev) {
-			pci_dev_put(amd_chipset.smbus_dev);
-			amd_chipset.smbus_dev = NULL;
+	if (info.sb_type == 0) {
+		if (info.smbus_dev) {
+			pci_dev_put(info.smbus_dev);
+			info.smbus_dev = NULL;
 		}
-		spin_unlock_irqrestore(&amd_lock, flags);
-		return 0;
+		ret = 0;
+		goto commit;
 	}
 
-	amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
-	if (amd_chipset.nb_dev) {
-		amd_chipset.nb_type = 1;
+	info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
+	if (info.nb_dev) {
+		info.nb_type = 1;
 	} else {
-		amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-							0x1510, NULL);
-		if (amd_chipset.nb_dev) {
-			amd_chipset.nb_type = 2;
-		} else  {
-			amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-								0x9600, NULL);
-			if (amd_chipset.nb_dev)
-				amd_chipset.nb_type = 3;
+		info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x1510, NULL);
+		if (info.nb_dev) {
+			info.nb_type = 2;
+		} else {
+			info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
+						     0x9600, NULL);
+			if (info.nb_dev)
+				info.nb_type = 3;
 		}
 	}
 
-	amd_chipset.probe_result = 1;
+	ret = info.probe_result = 1;
 	printk(KERN_DEBUG "QUIRK: Enable AMD PLL fix\n");
 
-	spin_unlock_irqrestore(&amd_lock, flags);
-	return amd_chipset.probe_result;
+commit:
+
+	spin_lock_irqsave(&amd_lock, flags);
+	if (amd_chipset.probe_count > 0) {
+		/* race - someone else was faster - drop devices */
+
+		/* Mark that we where here */
+		amd_chipset.probe_count++;
+		ret = amd_chipset.probe_result;
+
+		spin_unlock_irqrestore(&amd_lock, flags);
+
+		if (info.nb_dev)
+			pci_dev_put(info.nb_dev);
+		if (info.smbus_dev)
+			pci_dev_put(info.smbus_dev);
+
+	} else {
+		/* no race - commit the result */
+		info.probe_count++;
+		amd_chipset = info;
+		spin_unlock_irqrestore(&amd_lock, flags);
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);
 
@@ -284,6 +311,7 @@ EXPORT_SYMBOL_GPL(usb_amd_quirk_pll_enable);
 
 void usb_amd_dev_put(void)
 {
+	struct pci_dev *nb, *smbus;
 	unsigned long flags;
 
 	spin_lock_irqsave(&amd_lock, flags);
@@ -294,20 +322,23 @@ void usb_amd_dev_put(void)
 		return;
 	}
 
-	if (amd_chipset.nb_dev) {
-		pci_dev_put(amd_chipset.nb_dev);
-		amd_chipset.nb_dev = NULL;
-	}
-	if (amd_chipset.smbus_dev) {
-		pci_dev_put(amd_chipset.smbus_dev);
-		amd_chipset.smbus_dev = NULL;
-	}
+	/* save them to pci_dev_put outside of spinlock */
+	nb    = amd_chipset.nb_dev;
+	smbus = amd_chipset.smbus_dev;
+
+	amd_chipset.nb_dev = NULL;
+	amd_chipset.smbus_dev = NULL;
 	amd_chipset.nb_type = 0;
 	amd_chipset.sb_type = 0;
 	amd_chipset.isoc_reqs = 0;
 	amd_chipset.probe_result = 0;
 
 	spin_unlock_irqrestore(&amd_lock, flags);
+
+	if (nb)
+		pci_dev_put(nb);
+	if (smbus)
+		pci_dev_put(amd_chipset.smbus_dev);
 }
 EXPORT_SYMBOL_GPL(usb_amd_dev_put);
 
-- 
1.7.1


-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH v6] USB host: Fix lockdep warning in AMD PLL quirk

[Sergei Shtylyov]

Hello.

On 12-04-2011 10:41, Roedel, Joerg wrote:

>>> Hmm, I see how it can be done differently, but no real bug.

>> Never mind, you're right.  In the no-race case, the initial count is
>> always going to be 0, and so incrementing info.probe_count is the right
>> thing to do.

> Okay, cool. So now that we have settled on this I removed the last
> occurence of '2.6.38' from the changelog and here is the result. No
> code changes since v5.

    I've pointed out the mistake in the code that you didn't fix... :-(

>  From a6e00d237f20e940afdbcaafb27117fe8730466f Mon Sep 17 00:00:00 2001
> From: Joerg Roedel<joerg.roedel@amd.com>
> Date: Wed, 6 Apr 2011 13:07:53 +0200
> Subject: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

> Booting latest kernel on my test machine produces a lockdep
> warning from the usb_amd_find_chipset_info() function:

>   WARNING: at /data/lemmy/linux.trees.git/kernel/lockdep.c:2465 lockdep_trace_alloc+0x95/0xc2()
>   Hardware name: Snook
>   Modules linked in:
>   Pid: 959, comm: work_for_cpu Not tainted 2.6.39-rc2+ #22
>   Call Trace:
>    [<ffffffff8103c0d4>] warn_slowpath_common+0x80/0x98
>    [<ffffffff812387e6>] ? T.492+0x24/0x26
>    [<ffffffff8103c101>] warn_slowpath_null+0x15/0x17
>    [<ffffffff81068667>] lockdep_trace_alloc+0x95/0xc2
>    [<ffffffff810ed9ac>] slab_pre_alloc_hook+0x18/0x3b
>    [<ffffffff810ef227>] kmem_cache_alloc_trace+0x25/0xba
>    [<ffffffff812387e6>] T.492+0x24/0x26
>    [<ffffffff81238816>] pci_get_subsys+0x2e/0x73
>    [<ffffffff8123886c>] pci_get_device+0x11/0x13
>    [<ffffffff814082a9>] usb_amd_find_chipset_info+0x3f/0x18a
> ...

> It turns out that this function calls pci_get_device under a spin_lock
> with irqs disabled, but the pci_get_device function is only allowed in
> preemptible context.

> This patch fixes the warning by making all data-structure
> modifications on temporal storage and commiting this back
> into the visible structure at the end. While at it, this
> patch also moves the pci_dev_put calls out of the spinlocks
> because this function might sleep too.

> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>   drivers/usb/host/pci-quirks.c |  117 ++++++++++++++++++++++++++---------------
>   1 files changed, 74 insertions(+), 43 deletions(-)

> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 1d586d4..c6eb69c 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
[...]
> @@ -294,20 +322,23 @@ void usb_amd_dev_put(void)
>   		return;
>   	}
>
> -	if (amd_chipset.nb_dev) {
> -		pci_dev_put(amd_chipset.nb_dev);
> -		amd_chipset.nb_dev = NULL;
> -	}
> -	if (amd_chipset.smbus_dev) {
> -		pci_dev_put(amd_chipset.smbus_dev);
> -		amd_chipset.smbus_dev = NULL;
> -	}
> +	/* save them to pci_dev_put outside of spinlock */
> +	nb    = amd_chipset.nb_dev;
> +	smbus = amd_chipset.smbus_dev;
> +
> +	amd_chipset.nb_dev = NULL;
> +	amd_chipset.smbus_dev = NULL;
>   	amd_chipset.nb_type = 0;
>   	amd_chipset.sb_type = 0;
>   	amd_chipset.isoc_reqs = 0;
>   	amd_chipset.probe_result = 0;
>
>   	spin_unlock_irqrestore(&amd_lock, flags);
> +
> +	if (nb)
> +		pci_dev_put(nb);
> +	if (smbus)
> +		pci_dev_put(amd_chipset.smbus_dev);

    Here it is. This pointer is NULL, it should be 'smbus' instead.

WBR, Sergei

Re: [PATCH v6] USB host: Fix lockdep warning in AMD PLL quirk

[Roedel, Joerg]

On Tue, Apr 12, 2011 at 06:59:27AM -0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 12-04-2011 10:41, Roedel, Joerg wrote:
> 
> >>> Hmm, I see how it can be done differently, but no real bug.
> 
> >> Never mind, you're right.  In the no-race case, the initial count is
> >> always going to be 0, and so incrementing info.probe_count is the right
> >> thing to do.
> 
> > Okay, cool. So now that we have settled on this I removed the last
> > occurence of '2.6.38' from the changelog and here is the result. No
> > code changes since v5.
> 
>     I've pointed out the mistake in the code that you didn't fix... :-(

Argh, sorry, I missed this comment from you. I'll fix this.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[PATCH v7] USB host: Fix lockdep warning in AMD PLL quirk

[Roedel, Joerg]

On Tue, Apr 12, 2011 at 06:59:27AM -0400, Sergei Shtylyov wrote:
> On 12-04-2011 10:41, Roedel, Joerg wrote:

> > +	if (nb)
> > +		pci_dev_put(nb);
> > +	if (smbus)
> > +		pci_dev_put(amd_chipset.smbus_dev);
> 
>     Here it is. This pointer is NULL, it should be 'smbus' instead.

Ok, this is fixed now too.

>From 7ee4b6274f5e4c1f238e9ff4faa0b9a43f5203e2 Mon Sep 17 00:00:00 2001
From: Joerg Roedel <joerg.roedel@amd.com>
Date: Wed, 6 Apr 2011 13:07:53 +0200
Subject: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk

Booting latest kernel on my test machine produces a lockdep
warning from the usb_amd_find_chipset_info() function:

 WARNING: at /data/lemmy/linux.trees.git/kernel/lockdep.c:2465 lockdep_trace_alloc+0x95/0xc2()
 Hardware name: Snook
 Modules linked in:
 Pid: 959, comm: work_for_cpu Not tainted 2.6.39-rc2+ #22
 Call Trace:
  [<ffffffff8103c0d4>] warn_slowpath_common+0x80/0x98
  [<ffffffff812387e6>] ? T.492+0x24/0x26
  [<ffffffff8103c101>] warn_slowpath_null+0x15/0x17
  [<ffffffff81068667>] lockdep_trace_alloc+0x95/0xc2
  [<ffffffff810ed9ac>] slab_pre_alloc_hook+0x18/0x3b
  [<ffffffff810ef227>] kmem_cache_alloc_trace+0x25/0xba
  [<ffffffff812387e6>] T.492+0x24/0x26
  [<ffffffff81238816>] pci_get_subsys+0x2e/0x73
  [<ffffffff8123886c>] pci_get_device+0x11/0x13
  [<ffffffff814082a9>] usb_amd_find_chipset_info+0x3f/0x18a
...

It turns out that this function calls pci_get_device under a spin_lock
with irqs disabled, but the pci_get_device function is only allowed in
preemptible context.

This patch fixes the warning by making all data-structure
modifications on temporal storage and commiting this back
into the visible structure at the end. While at it, this
patch also moves the pci_dev_put calls out of the spinlocks
because this function might sleep too.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/usb/host/pci-quirks.c |  117 ++++++++++++++++++++++++++---------------
 1 files changed, 74 insertions(+), 43 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 1d586d4..9b166d7 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -84,65 +84,92 @@ int usb_amd_find_chipset_info(void)
 {
 	u8 rev = 0;
 	unsigned long flags;
+	struct amd_chipset_info info;
+	int ret;
 
 	spin_lock_irqsave(&amd_lock, flags);
 
-	amd_chipset.probe_count++;
 	/* probe only once */
-	if (amd_chipset.probe_count > 1) {
+	if (amd_chipset.probe_count > 0) {
+		amd_chipset.probe_count++;
 		spin_unlock_irqrestore(&amd_lock, flags);
 		return amd_chipset.probe_result;
 	}
+	memset(&info, 0, sizeof(info));
+	spin_unlock_irqrestore(&amd_lock, flags);
 
-	amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL);
-	if (amd_chipset.smbus_dev) {
-		rev = amd_chipset.smbus_dev->revision;
+	info.smbus_dev = pci_get_device(PCI_VENDOR_ID_ATI, 0x4385, NULL);
+	if (info.smbus_dev) {
+		rev = info.smbus_dev->revision;
 		if (rev >= 0x40)
-			amd_chipset.sb_type = 1;
+			info.sb_type = 1;
 		else if (rev >= 0x30 && rev <= 0x3b)
-			amd_chipset.sb_type = 3;
+			info.sb_type = 3;
 	} else {
-		amd_chipset.smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-							0x780b, NULL);
-		if (!amd_chipset.smbus_dev) {
-			spin_unlock_irqrestore(&amd_lock, flags);
-			return 0;
+		info.smbus_dev = pci_get_device(PCI_VENDOR_ID_AMD,
+						0x780b, NULL);
+		if (!info.smbus_dev) {
+			ret = 0;
+			goto commit;
 		}
-		rev = amd_chipset.smbus_dev->revision;
+
+		rev = info.smbus_dev->revision;
 		if (rev >= 0x11 && rev <= 0x18)
-			amd_chipset.sb_type = 2;
+			info.sb_type = 2;
 	}
 
-	if (amd_chipset.sb_type == 0) {
-		if (amd_chipset.smbus_dev) {
-			pci_dev_put(amd_chipset.smbus_dev);
-			amd_chipset.smbus_dev = NULL;
+	if (info.sb_type == 0) {
+		if (info.smbus_dev) {
+			pci_dev_put(info.smbus_dev);
+			info.smbus_dev = NULL;
 		}
-		spin_unlock_irqrestore(&amd_lock, flags);
-		return 0;
+		ret = 0;
+		goto commit;
 	}
 
-	amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
-	if (amd_chipset.nb_dev) {
-		amd_chipset.nb_type = 1;
+	info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x9601, NULL);
+	if (info.nb_dev) {
+		info.nb_type = 1;
 	} else {
-		amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-							0x1510, NULL);
-		if (amd_chipset.nb_dev) {
-			amd_chipset.nb_type = 2;
-		} else  {
-			amd_chipset.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
-								0x9600, NULL);
-			if (amd_chipset.nb_dev)
-				amd_chipset.nb_type = 3;
+		info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD, 0x1510, NULL);
+		if (info.nb_dev) {
+			info.nb_type = 2;
+		} else {
+			info.nb_dev = pci_get_device(PCI_VENDOR_ID_AMD,
+						     0x9600, NULL);
+			if (info.nb_dev)
+				info.nb_type = 3;
 		}
 	}
 
-	amd_chipset.probe_result = 1;
+	ret = info.probe_result = 1;
 	printk(KERN_DEBUG "QUIRK: Enable AMD PLL fix\n");
 
-	spin_unlock_irqrestore(&amd_lock, flags);
-	return amd_chipset.probe_result;
+commit:
+
+	spin_lock_irqsave(&amd_lock, flags);
+	if (amd_chipset.probe_count > 0) {
+		/* race - someone else was faster - drop devices */
+
+		/* Mark that we where here */
+		amd_chipset.probe_count++;
+		ret = amd_chipset.probe_result;
+
+		spin_unlock_irqrestore(&amd_lock, flags);
+
+		if (info.nb_dev)
+			pci_dev_put(info.nb_dev);
+		if (info.smbus_dev)
+			pci_dev_put(info.smbus_dev);
+
+	} else {
+		/* no race - commit the result */
+		info.probe_count++;
+		amd_chipset = info;
+		spin_unlock_irqrestore(&amd_lock, flags);
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(usb_amd_find_chipset_info);
 
@@ -284,6 +311,7 @@ EXPORT_SYMBOL_GPL(usb_amd_quirk_pll_enable);
 
 void usb_amd_dev_put(void)
 {
+	struct pci_dev *nb, *smbus;
 	unsigned long flags;
 
 	spin_lock_irqsave(&amd_lock, flags);
@@ -294,20 +322,23 @@ void usb_amd_dev_put(void)
 		return;
 	}
 
-	if (amd_chipset.nb_dev) {
-		pci_dev_put(amd_chipset.nb_dev);
-		amd_chipset.nb_dev = NULL;
-	}
-	if (amd_chipset.smbus_dev) {
-		pci_dev_put(amd_chipset.smbus_dev);
-		amd_chipset.smbus_dev = NULL;
-	}
+	/* save them to pci_dev_put outside of spinlock */
+	nb    = amd_chipset.nb_dev;
+	smbus = amd_chipset.smbus_dev;
+
+	amd_chipset.nb_dev = NULL;
+	amd_chipset.smbus_dev = NULL;
 	amd_chipset.nb_type = 0;
 	amd_chipset.sb_type = 0;
 	amd_chipset.isoc_reqs = 0;
 	amd_chipset.probe_result = 0;
 
 	spin_unlock_irqrestore(&amd_lock, flags);
+
+	if (nb)
+		pci_dev_put(nb);
+	if (smbus)
+		pci_dev_put(smbus);
 }
 EXPORT_SYMBOL_GPL(usb_amd_dev_put);
 
-- 
1.7.1


-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH v7] USB host: Fix lockdep warning in AMD PLL quirk

[Alan Stern]

On Wed, 13 Apr 2011, Roedel, Joerg wrote:

> On Tue, Apr 12, 2011 at 06:59:27AM -0400, Sergei Shtylyov wrote:
> > On 12-04-2011 10:41, Roedel, Joerg wrote:
> 
> > > +	if (nb)
> > > +		pci_dev_put(nb);
> > > +	if (smbus)
> > > +		pci_dev_put(amd_chipset.smbus_dev);
> > 
> >     Here it is. This pointer is NULL, it should be 'smbus' instead.
> 
> Ok, this is fixed now too.
> 
> From 7ee4b6274f5e4c1f238e9ff4faa0b9a43f5203e2 Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <joerg.roedel@amd.com>
> Date: Wed, 6 Apr 2011 13:07:53 +0200
> Subject: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk
> 
> Booting latest kernel on my test machine produces a lockdep
> warning from the usb_amd_find_chipset_info() function:
> 
>  WARNING: at /data/lemmy/linux.trees.git/kernel/lockdep.c:2465 lockdep_trace_alloc+0x95/0xc2()
>  Hardware name: Snook
>  Modules linked in:
>  Pid: 959, comm: work_for_cpu Not tainted 2.6.39-rc2+ #22
>  Call Trace:
>   [<ffffffff8103c0d4>] warn_slowpath_common+0x80/0x98
>   [<ffffffff812387e6>] ? T.492+0x24/0x26
>   [<ffffffff8103c101>] warn_slowpath_null+0x15/0x17
>   [<ffffffff81068667>] lockdep_trace_alloc+0x95/0xc2
>   [<ffffffff810ed9ac>] slab_pre_alloc_hook+0x18/0x3b
>   [<ffffffff810ef227>] kmem_cache_alloc_trace+0x25/0xba
>   [<ffffffff812387e6>] T.492+0x24/0x26
>   [<ffffffff81238816>] pci_get_subsys+0x2e/0x73
>   [<ffffffff8123886c>] pci_get_device+0x11/0x13
>   [<ffffffff814082a9>] usb_amd_find_chipset_info+0x3f/0x18a
> ...
> 
> It turns out that this function calls pci_get_device under a spin_lock
> with irqs disabled, but the pci_get_device function is only allowed in
> preemptible context.
> 
> This patch fixes the warning by making all data-structure
> modifications on temporal storage and commiting this back
> into the visible structure at the end. While at it, this
> patch also moves the pci_dev_put calls out of the spinlocks
> because this function might sleep too.
> 
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>

Acked-by: Alan Stern <stern@rowland.harvard.edu>