| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2DEEA3AB13739D45A22ADDB5086AEA0B013084C4@sshaexmb1.amd.com>
Date: Thu, 7 Apr 2011 17:58:14 +0800
From: "Xu, Andiry" <Andiry.Xu@....com>
To: "Roedel, Joerg" <Joerg.Roedel@....com>,
Greg Kroah-Hartman <gregkh@...e.de>,
Sarah Sharp <sarah.a.sharp@...ux.intel.com>
CC: <linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
"Roedel, Joerg" <Joerg.Roedel@....com>, <stable@...nel.org>,
"He, Alex" <Alex.He@....com>
Subject: RE: [PATCH] USB host: Fix lockdep warning in AMD PLL quirk
> -----Original Message-----
> From: Joerg Roedel [mailto:joerg.roedel@....com]
> Sent: Thursday, April 07, 2011 4:26 PM
> To: Greg Kroah-Hartman; Sarah Sharp; Xu, Andiry
> Cc: linux-usb@...r.kernel.org; linux-kernel@...r.kernel.org; Roedel,
Joerg;
> stable@...nel.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@...nel.org
> Signed-off-by: Joerg Roedel <joerg.roedel@....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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists