[patch 0/4] Small fixes for 2.6.15-rc5

[patch 0/4] Small fixes for 2.6.15-rc5

[Greg Kroah-Hartman]

Here are some small fixes for your current 2.6.15-rc5 git tree.  They
have all been in the past -mm releases, and fix problems.  The PCI
patches were also sent by Andi to you, so you might receive them through
his emails too.

Here's the diffstat of the combined patches.

 arch/i386/pci/direct.c           |    8 +-
 arch/i386/pci/mmconfig.c         |  141 ++++++++++++++++++++++++++++++++-------
 arch/i386/pci/pci.h              |   14 +++
 arch/x86_64/pci/mmconfig.c       |  133 ++++++++++++++++++++++++++++++------
 drivers/i2c/busses/i2c-mv64xxx.c |   25 +++---
 drivers/usb/host/uhci-hcd.c      |    6 +
 6 files changed, 261 insertions(+), 66 deletions(-)

thanks,

greg k-h

[patch 1/4] i2c: Fix i2c-mv64xxx compilation error

[Greg Kroah-Hartman]

[-- Attachment #1: i2c-mv64xxx-compilation-error-fix.patch --]
[-- Type: text/plain, Size: 1268 bytes --]

From: "Mark A. Greer" <mgreer@mvista.com>

The busses/i2c-mv64xxx.c driver doesn't currently compile because of an
incorrect argument to dev_err().  This patch fixes that.

Signed-off-by: Mark A. Greer <mgreer@mvista.com>
Signed-off-by: Jean Delvare <khali@linux-fr.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/i2c/busses/i2c-mv64xxx.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

--- greg-2.6.orig/drivers/i2c/busses/i2c-mv64xxx.c
+++ greg-2.6/drivers/i2c/busses/i2c-mv64xxx.c
@@ -529,14 +529,15 @@ mv64xxx_i2c_probe(struct platform_device
 	i2c_set_adapdata(&drv_data->adapter, drv_data);
 
 	if (request_irq(drv_data->irq, mv64xxx_i2c_intr, 0,
-		MV64XXX_I2C_CTLR_NAME, drv_data)) {
-
-		dev_err(dev, "mv64xxx: Can't register intr handler "
-			"irq: %d\n", drv_data->irq);
+			MV64XXX_I2C_CTLR_NAME, drv_data)) {
+		dev_err(&drv_data->adapter.dev,
+			"mv64xxx: Can't register intr handler irq: %d\n",
+			drv_data->irq);
 		rc = -EINVAL;
 		goto exit_unmap_regs;
 	} else if ((rc = i2c_add_adapter(&drv_data->adapter)) != 0) {
-		dev_err(dev, "mv64xxx: Can't add i2c adapter, rc: %d\n", -rc);
+		dev_err(&drv_data->adapter.dev,
+			"mv64xxx: Can't add i2c adapter, rc: %d\n", -rc);
 		goto exit_free_irq;
 	}
 

--

[patch 2/4] i386/x86-64: Implement fallback for PCI mmconfig to type1

[Greg Kroah-Hartman]

[-- Attachment #1: i386-x86-64-implement-fallback-for-pci-mmconfig-to-type1.patch --]
[-- Type: text/plain, Size: 6284 bytes --]

From: Andi Kleen <ak@muc.de>


When there is no entry for a bus in MCFG fall back to type1.  This is
especially important on K8 systems where always some devices can't be
accessed using mmconfig (in particular the builtin northbridge doesn't
support it for its own devices)

To be complete needs also Jeff's _SEG fixes, but that is something for
after 2.6.15

Signed-off-by: Andi Kleen <ak@suse.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 arch/i386/pci/direct.c     |    4 ++--
 arch/i386/pci/mmconfig.c   |   24 ++++++++++++++++--------
 arch/i386/pci/pci.h        |    7 +++++++
 arch/x86_64/pci/mmconfig.c |   29 ++++++++++++++++++++---------
 4 files changed, 45 insertions(+), 19 deletions(-)

--- greg-2.6.orig/arch/i386/pci/direct.c
+++ greg-2.6/arch/i386/pci/direct.c
@@ -13,7 +13,7 @@
 #define PCI_CONF1_ADDRESS(bus, devfn, reg) \
 	(0x80000000 | (bus << 16) | (devfn << 8) | (reg & ~3))
 
-static int pci_conf1_read(unsigned int seg, unsigned int bus,
+int pci_conf1_read(unsigned int seg, unsigned int bus,
 			  unsigned int devfn, int reg, int len, u32 *value)
 {
 	unsigned long flags;
@@ -42,7 +42,7 @@ static int pci_conf1_read(unsigned int s
 	return 0;
 }
 
-static int pci_conf1_write(unsigned int seg, unsigned int bus,
+int pci_conf1_write(unsigned int seg, unsigned int bus,
 			   unsigned int devfn, int reg, int len, u32 value)
 {
 	unsigned long flags;
--- greg-2.6.orig/arch/i386/pci/pci.h
+++ greg-2.6/arch/i386/pci/pci.h
@@ -74,3 +74,10 @@ extern spinlock_t pci_config_lock;
 
 extern int (*pcibios_enable_irq)(struct pci_dev *dev);
 extern void (*pcibios_disable_irq)(struct pci_dev *dev);
+
+extern int pci_conf1_write(unsigned int seg, unsigned int bus,
+			   unsigned int devfn, int reg, int len, u32 value);
+extern int pci_conf1_read(unsigned int seg, unsigned int bus,
+			  unsigned int devfn, int reg, int len, u32 *value);
+
+
--- greg-2.6.orig/arch/i386/pci/mmconfig.c
+++ greg-2.6/arch/i386/pci/mmconfig.c
@@ -30,10 +30,8 @@ static u32 get_base_addr(unsigned int se
 	while (1) {
 		++cfg_num;
 		if (cfg_num >= pci_mmcfg_config_num) {
-			/* something bad is going on, no cfg table is found. */
-			/* so we fall back to the old way we used to do this */
-			/* and just rely on the first entry to be correct. */
-			return pci_mmcfg_config[0].base_address;
+			/* Not found - fallback to type 1 */
+			return 0;
 		}
 		cfg = &pci_mmcfg_config[cfg_num];
 		if (cfg->pci_segment_group_number != seg)
@@ -44,9 +42,9 @@ static u32 get_base_addr(unsigned int se
 	}
 }
 
-static inline void pci_exp_set_dev_base(unsigned int seg, int bus, int devfn)
+static inline void pci_exp_set_dev_base(unsigned int base, int bus, int devfn)
 {
-	u32 dev_base = get_base_addr(seg, bus) | (bus << 20) | (devfn << 12);
+	u32 dev_base = base | (bus << 20) | (devfn << 12);
 	if (dev_base != mmcfg_last_accessed_device) {
 		mmcfg_last_accessed_device = dev_base;
 		set_fixmap_nocache(FIX_PCIE_MCFG, dev_base);
@@ -57,13 +55,18 @@ static int pci_mmcfg_read(unsigned int s
 			  unsigned int devfn, int reg, int len, u32 *value)
 {
 	unsigned long flags;
+	u32 base;
 
 	if (!value || (bus > 255) || (devfn > 255) || (reg > 4095))
 		return -EINVAL;
 
+	base = get_base_addr(seg, bus);
+	if (!base)
+		return pci_conf1_read(seg,bus,devfn,reg,len,value);
+
 	spin_lock_irqsave(&pci_config_lock, flags);
 
-	pci_exp_set_dev_base(seg, bus, devfn);
+	pci_exp_set_dev_base(base, bus, devfn);
 
 	switch (len) {
 	case 1:
@@ -86,13 +89,18 @@ static int pci_mmcfg_write(unsigned int 
 			   unsigned int devfn, int reg, int len, u32 value)
 {
 	unsigned long flags;
+	u32 base;
 
 	if ((bus > 255) || (devfn > 255) || (reg > 4095)) 
 		return -EINVAL;
 
+	base = get_base_addr(seg, bus);
+	if (!base)
+		return pci_conf1_write(seg,bus,devfn,reg,len,value);
+
 	spin_lock_irqsave(&pci_config_lock, flags);
 
-	pci_exp_set_dev_base(seg, bus, devfn);
+	pci_exp_set_dev_base(base, bus, devfn);
 
 	switch (len) {
 	case 1:
--- greg-2.6.orig/arch/x86_64/pci/mmconfig.c
+++ greg-2.6/arch/x86_64/pci/mmconfig.c
@@ -19,7 +19,7 @@ struct mmcfg_virt {
 };
 static struct mmcfg_virt *pci_mmcfg_virt;
 
-static char *get_virt(unsigned int seg, int bus)
+static char *get_virt(unsigned int seg, unsigned bus)
 {
 	int cfg_num = -1;
 	struct acpi_table_mcfg_config *cfg;
@@ -27,10 +27,9 @@ static char *get_virt(unsigned int seg, 
 	while (1) {
 		++cfg_num;
 		if (cfg_num >= pci_mmcfg_config_num) {
-			/* something bad is going on, no cfg table is found. */
-			/* so we fall back to the old way we used to do this */
-			/* and just rely on the first entry to be correct. */
-			return pci_mmcfg_virt[0].virt;
+			/* Not found - fall back to type 1. This happens
+			   e.g. on the internal devices of a K8 northbridge. */
+			return NULL;
 		}
 		cfg = pci_mmcfg_virt[cfg_num].cfg;
 		if (cfg->pci_segment_group_number != seg)
@@ -43,18 +42,25 @@ static char *get_virt(unsigned int seg, 
 
 static inline char *pci_dev_base(unsigned int seg, unsigned int bus, unsigned int devfn)
 {
-
-	return get_virt(seg, bus) + ((bus << 20) | (devfn << 12));
+	char *addr = get_virt(seg, bus);
+	if (!addr)
+		return NULL;
+ 	return addr + ((bus << 20) | (devfn << 12));
 }
 
 static int pci_mmcfg_read(unsigned int seg, unsigned int bus,
 			  unsigned int devfn, int reg, int len, u32 *value)
 {
-	char *addr = pci_dev_base(seg, bus, devfn);
+	char *addr;
 
+	/* Why do we have this when nobody checks it. How about a BUG()!? -AK */
 	if (unlikely(!value || (bus > 255) || (devfn > 255) || (reg > 4095)))
 		return -EINVAL;
 
+	addr = pci_dev_base(seg, bus, devfn);
+	if (!addr)
+		return pci_conf1_read(seg,bus,devfn,reg,len,value);
+
 	switch (len) {
 	case 1:
 		*value = readb(addr + reg);
@@ -73,11 +79,16 @@ static int pci_mmcfg_read(unsigned int s
 static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
 			   unsigned int devfn, int reg, int len, u32 value)
 {
-	char *addr = pci_dev_base(seg, bus, devfn);
+	char *addr;
 
+	/* Why do we have this when nobody checks it. How about a BUG()!? -AK */
 	if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
 		return -EINVAL;
 
+	addr = pci_dev_base(seg, bus, devfn);
+	if (!addr)
+		return pci_conf1_write(seg,bus,devfn,reg,len,value);
+
 	switch (len) {
 	case 1:
 		writeb(value, addr + reg);

--

Re: [patch 2/4] i386/x86-64: Implement fallback for PCI mmconfig to type1

[Matthew Wilcox]

On Mon, Dec 12, 2005 at 12:01:23PM -0800, Greg Kroah-Hartman wrote:
> When there is no entry for a bus in MCFG fall back to type1.  This is
> especially important on K8 systems where always some devices can't be
> accessed using mmconfig (in particular the builtin northbridge doesn't
> support it for its own devices)
[...]
> -static int pci_conf1_read(unsigned int seg, unsigned int bus,
> +int pci_conf1_read(unsigned int seg, unsigned int bus,

I don't like this at all.  We already have a mechanism to use different
accessors per-bus (bus->ops->read()); calling the type1 accessors from
the mmconfig accessors just seems wrong.

> +	if (!base)
> +		return pci_conf1_read(seg,bus,devfn,reg,len,value);

Should be space after commas.

Re: [patch 2/4] i386/x86-64: Implement fallback for PCI mmconfig to type1

[Greg KH]

On Mon, Dec 12, 2005 at 01:26:43PM -0700, Matthew Wilcox wrote:
> On Mon, Dec 12, 2005 at 12:01:23PM -0800, Greg Kroah-Hartman wrote:
> > When there is no entry for a bus in MCFG fall back to type1.  This is
> > especially important on K8 systems where always some devices can't be
> > accessed using mmconfig (in particular the builtin northbridge doesn't
> > support it for its own devices)
> [...]
> > -static int pci_conf1_read(unsigned int seg, unsigned int bus,
> > +int pci_conf1_read(unsigned int seg, unsigned int bus,
> 
> I don't like this at all.  We already have a mechanism to use different
> accessors per-bus (bus->ops->read()); calling the type1 accessors from
> the mmconfig accessors just seems wrong.

>From what I can tell, it's too late in the callstack for us to change
the read ops for this device to be the other one.  The problem is (and
Andi can correct me if I'm wrong), some boxes basically have incomplete
MCFG acpi tables (the tables do not describe all PCI busses that are
present in the box).  But we don't realize this until we are about to do
the read function.

I remember I looked into trying to set this up at probe/init time, and
it was almost impossible to do so, due to the structure of the code.

However, I might have missed something, and if you can point how to do
this easier, please do.

As it is, this patch is needed to fix boxes that do not work at all with
the current kernel.

thanks,

greg k-h

Re: [patch 2/4] i386/x86-64: Implement fallback for PCI mmconfig to type1

[Andi Kleen]

Sorry Greg, Linus,

I also submitted the two PCI patches on my own with the x86-64
patchkit, hopefully the duplication doesn't cause too much trouble.

Greg KH <gregkh@suse.de> writes:
> 
> From what I can tell, it's too late in the callstack for us to change
> the read ops for this device to be the other one.  The problem is (and
> Andi can correct me if I'm wrong), some boxes basically have incomplete
> MCFG acpi tables (the tables do not describe all PCI busses that are
> present in the box).  But we don't realize this until we are about to do
> the read function.

It can happen with perfectly legal MCFG tables. If a bus is not listed
in MCFG then we must fallback to type1. This happens on AMD K8 systems
because the busses in the builtin northbridge don't support mmconfig,
only busses on external bridges do. In theory it could happen on
other systems too (although external northbridges typically support
mmconfig for everything if they do at all) 

In addition we have some boxes with broken MCFG tables who don't get
the tables right, this is what the next patch was trying to fix.

> I remember I looked into trying to set this up at probe/init time, and
> it was almost impossible to do so, due to the structure of the code.

Yes, I also didn't see an easy way to do it, although it would be probably DTRT. 

-Andi

[patch 3/4] x86_64/i386: Correct for broken MCFG tables on K8 systems

[Greg Kroah-Hartman]

[-- Attachment #1: x86_64-i386-correct-for-broken-mcfg-tables-on-k8-systems.patch --]
[-- Type: text/plain, Size: 5666 bytes --]

From: Andi Kleen <ak@muc.de>

They report all busses as MMCONFIG capable, but it never works for the
internal devices in the CPU's builtin northbridge.

This causes problems - some devices are not visible and on x86-64 the
IOMMU is not detected, causing boot failures on systems with >3GB of
RAM.  It also breaks AGP.

Workaround just probes all func 0 devices on bus 0 (the internal
northbridge is currently always on bus 0) and if they are not accessible
using MCFG they are put into a special fallback bitmap.

On systems where it isn't we assume the BIOS vendor supplied correct
MCFG.

Requires the earlier patch for mmconfig type1 fallback

Signed-off-by: Andi Kleen <ak@suse.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 arch/i386/pci/mmconfig.c   |   45 ++++++++++++++++++++++++++++++++++++++++++---
 arch/x86_64/pci/mmconfig.c |   36 ++++++++++++++++++++++++++++++++++--
 2 files changed, 76 insertions(+), 5 deletions(-)

--- greg-2.6.orig/arch/x86_64/pci/mmconfig.c
+++ greg-2.6/arch/x86_64/pci/mmconfig.c
@@ -8,10 +8,13 @@
 #include <linux/pci.h>
 #include <linux/init.h>
 #include <linux/acpi.h>
+#include <linux/bitmap.h>
 #include "pci.h"
 
 #define MMCONFIG_APER_SIZE (256*1024*1024)
 
+static DECLARE_BITMAP(fallback_slots, 32);
+
 /* Static virtual mapping of the MMCONFIG aperture */
 struct mmcfg_virt {
 	struct acpi_table_mcfg_config *cfg;
@@ -40,9 +43,12 @@ static char *get_virt(unsigned int seg, 
 	}
 }
 
-static inline char *pci_dev_base(unsigned int seg, unsigned int bus, unsigned int devfn)
+static char *pci_dev_base(unsigned int seg, unsigned int bus, unsigned int devfn)
 {
-	char *addr = get_virt(seg, bus);
+	char *addr;
+	if (seg == 0 && bus == 0 && test_bit(PCI_SLOT(devfn), &fallback_slots))
+		return NULL;
+	addr = get_virt(seg, bus);
 	if (!addr)
 		return NULL;
  	return addr + ((bus << 20) | (devfn << 12));
@@ -109,6 +115,30 @@ static struct pci_raw_ops pci_mmcfg = {
 	.write =	pci_mmcfg_write,
 };
 
+/* K8 systems have some devices (typically in the builtin northbridge)
+   that are only accessible using type1
+   Normally this can be expressed in the MCFG by not listing them
+   and assigning suitable _SEGs, but this isn't implemented in some BIOS.
+   Instead try to discover all devices on bus 0 that are unreachable using MM
+   and fallback for them.
+   We only do this for bus 0/seg 0 */
+static __init void unreachable_devices(void)
+{
+	int i;
+	for (i = 0; i < 32; i++) {
+		u32 val1;
+		char *addr;
+
+		pci_conf1_read(0, 0, PCI_DEVFN(i,0), 0, 4, &val1);
+		if (val1 == 0xffffffff)
+			continue;
+		addr = pci_dev_base(0, 0, PCI_DEVFN(i, 0));
+		if (addr == NULL || readl(addr) != val1) {
+			set_bit(i, &fallback_slots);
+		}
+	}
+}
+
 static int __init pci_mmcfg_init(void)
 {
 	int i;
@@ -139,6 +169,8 @@ static int __init pci_mmcfg_init(void)
 		printk(KERN_INFO "PCI: Using MMCONFIG at %x\n", pci_mmcfg_config[i].base_address);
 	}
 
+	unreachable_devices();
+
 	raw_pci_ops = &pci_mmcfg;
 	pci_probe = (pci_probe & ~PCI_PROBE_MASK) | PCI_PROBE_MMCONF;
 
--- greg-2.6.orig/arch/i386/pci/mmconfig.c
+++ greg-2.6/arch/i386/pci/mmconfig.c
@@ -19,14 +19,20 @@
 /* The base address of the last MMCONFIG device accessed */
 static u32 mmcfg_last_accessed_device;
 
+static DECLARE_BITMAP(fallback_slots, 32);
+
 /*
  * Functions for accessing PCI configuration space with MMCONFIG accesses
  */
-static u32 get_base_addr(unsigned int seg, int bus)
+static u32 get_base_addr(unsigned int seg, int bus, unsigned devfn)
 {
 	int cfg_num = -1;
 	struct acpi_table_mcfg_config *cfg;
 
+	if (seg == 0 && bus == 0 &&
+	    test_bit(PCI_SLOT(devfn), fallback_slots))
+		return 0;
+
 	while (1) {
 		++cfg_num;
 		if (cfg_num >= pci_mmcfg_config_num) {
@@ -60,7 +66,7 @@ static int pci_mmcfg_read(unsigned int s
 	if (!value || (bus > 255) || (devfn > 255) || (reg > 4095))
 		return -EINVAL;
 
-	base = get_base_addr(seg, bus);
+	base = get_base_addr(seg, bus, devfn);
 	if (!base)
 		return pci_conf1_read(seg,bus,devfn,reg,len,value);
 
@@ -94,7 +100,7 @@ static int pci_mmcfg_write(unsigned int 
 	if ((bus > 255) || (devfn > 255) || (reg > 4095)) 
 		return -EINVAL;
 
-	base = get_base_addr(seg, bus);
+	base = get_base_addr(seg, bus, devfn);
 	if (!base)
 		return pci_conf1_write(seg,bus,devfn,reg,len,value);
 
@@ -124,6 +130,37 @@ static struct pci_raw_ops pci_mmcfg = {
 	.write =	pci_mmcfg_write,
 };
 
+/* K8 systems have some devices (typically in the builtin northbridge)
+   that are only accessible using type1
+   Normally this can be expressed in the MCFG by not listing them
+   and assigning suitable _SEGs, but this isn't implemented in some BIOS.
+   Instead try to discover all devices on bus 0 that are unreachable using MM
+   and fallback for them.
+   We only do this for bus 0/seg 0 */
+static __init void unreachable_devices(void)
+{
+	int i;
+	unsigned long flags;
+
+	for (i = 0; i < 32; i++) {
+		u32 val1;
+		u32 addr;
+
+		pci_conf1_read(0, 0, PCI_DEVFN(i, 0), 0, 4, &val1);
+		if (val1 == 0xffffffff)
+			continue;
+
+		/* Locking probably not needed, but safer */
+		spin_lock_irqsave(&pci_config_lock, flags);
+		addr = get_base_addr(0, 0, PCI_DEVFN(i, 0));
+		if (addr != 0)
+			pci_exp_set_dev_base(addr, 0, PCI_DEVFN(i, 0));
+		if (addr == 0 || readl((u32 *)addr) != val1)
+			set_bit(i, fallback_slots);
+		spin_unlock_irqrestore(&pci_config_lock, flags);
+	}
+}
+
 static int __init pci_mmcfg_init(void)
 {
 	if ((pci_probe & PCI_PROBE_MMCONF) == 0)
@@ -139,6 +176,8 @@ static int __init pci_mmcfg_init(void)
 	raw_pci_ops = &pci_mmcfg;
 	pci_probe = (pci_probe & ~PCI_PROBE_MASK) | PCI_PROBE_MMCONF;
 
+	unreachable_devices();
+
  out:
 	return 0;
 }

--

[patch 4/4] UHCI: add missing memory barriers

[Greg Kroah-Hartman]

[-- Attachment #1: uhci-add-missing-memory-barriers.patch --]
[-- Type: text/plain, Size: 965 bytes --]

From: Alan Stern <stern@rowland.harvard.edu>

This patch (as617) adds a couple of memory barriers that Ben H. forgot in
his recent suspend/resume fix.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/usb/host/uhci-hcd.c |    2 ++
 1 file changed, 2 insertions(+)

--- greg-2.6.orig/drivers/usb/host/uhci-hcd.c
+++ greg-2.6/drivers/usb/host/uhci-hcd.c
@@ -717,6 +717,7 @@ static int uhci_suspend(struct usb_hcd *
 	 * at the source, so we must turn off PIRQ.
 	 */
 	pci_write_config_word(to_pci_dev(uhci_dev(uhci)), USBLEGSUP, 0);
+	mb();
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 	uhci->hc_inaccessible = 1;
 	hcd->poll_rh = 0;
@@ -738,6 +739,7 @@ static int uhci_resume(struct usb_hcd *h
 	 * really don't want to keep a stale HCD_FLAG_HW_ACCESSIBLE=0
 	 */
 	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+	mb();
 
 	if (uhci->rh_state == UHCI_RH_RESET)	/* Dead */
 		return 0;

--

Re: [patch 4/4] UHCI: add missing memory barriers

[Jeff Garzik]

Greg Kroah-Hartman wrote:
> From: Alan Stern <stern@rowland.harvard.edu>
> 
> This patch (as617) adds a couple of memory barriers that Ben H. forgot in
> his recent suspend/resume fix.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> ---
>  drivers/usb/host/uhci-hcd.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> --- greg-2.6.orig/drivers/usb/host/uhci-hcd.c
> +++ greg-2.6/drivers/usb/host/uhci-hcd.c
> @@ -717,6 +717,7 @@ static int uhci_suspend(struct usb_hcd *
>  	 * at the source, so we must turn off PIRQ.
>  	 */
>  	pci_write_config_word(to_pci_dev(uhci_dev(uhci)), USBLEGSUP, 0);
> +	mb();
>  	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
>  	uhci->hc_inaccessible = 1;
>  	hcd->poll_rh = 0;
> @@ -738,6 +739,7 @@ static int uhci_resume(struct usb_hcd *h
>  	 * really don't want to keep a stale HCD_FLAG_HW_ACCESSIBLE=0
>  	 */
>  	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +	mb();

Are these just guesses, or what?

Why not smp_mb__before_clear_bit() or smp_mb__after_clear_bit() ?

	Jeff

Re: [patch 4/4] UHCI: add missing memory barriers

[Greg KH]

On Mon, Dec 12, 2005 at 07:27:45PM -0500, Jeff Garzik wrote:
> Greg Kroah-Hartman wrote:
> >From: Alan Stern <stern@rowland.harvard.edu>
> >
> >This patch (as617) adds a couple of memory barriers that Ben H. forgot in
> >his recent suspend/resume fix.
> >
> >Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> >Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> >
> >---
> > drivers/usb/host/uhci-hcd.c |    2 ++
> > 1 file changed, 2 insertions(+)
> >
> >--- greg-2.6.orig/drivers/usb/host/uhci-hcd.c
> >+++ greg-2.6/drivers/usb/host/uhci-hcd.c
> >@@ -717,6 +717,7 @@ static int uhci_suspend(struct usb_hcd *
> > 	 * at the source, so we must turn off PIRQ.
> > 	 */
> > 	pci_write_config_word(to_pci_dev(uhci_dev(uhci)), USBLEGSUP, 0);
> >+	mb();
> > 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> > 	uhci->hc_inaccessible = 1;
> > 	hcd->poll_rh = 0;
> >@@ -738,6 +739,7 @@ static int uhci_resume(struct usb_hcd *h
> > 	 * really don't want to keep a stale HCD_FLAG_HW_ACCESSIBLE=0
> > 	 */
> > 	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> >+	mb();
> 
> Are these just guesses, or what?
> 
> Why not smp_mb__before_clear_bit() or smp_mb__after_clear_bit() ?

I don't know, Alan?

Re: [patch 4/4] UHCI: add missing memory barriers

[Alan Stern]

On Mon, 12 Dec 2005, Jeff Garzik wrote:

> Greg Kroah-Hartman wrote:
> > From: Alan Stern <stern@rowland.harvard.edu>
> > 
> > This patch (as617) adds a couple of memory barriers that Ben H. forgot in
> > his recent suspend/resume fix.
> > 
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> > 
> > ---
> >  drivers/usb/host/uhci-hcd.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > --- greg-2.6.orig/drivers/usb/host/uhci-hcd.c
> > +++ greg-2.6/drivers/usb/host/uhci-hcd.c
> > @@ -717,6 +717,7 @@ static int uhci_suspend(struct usb_hcd *
> >  	 * at the source, so we must turn off PIRQ.
> >  	 */
> >  	pci_write_config_word(to_pci_dev(uhci_dev(uhci)), USBLEGSUP, 0);
> > +	mb();
> >  	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> >  	uhci->hc_inaccessible = 1;
> >  	hcd->poll_rh = 0;
> > @@ -738,6 +739,7 @@ static int uhci_resume(struct usb_hcd *h
> >  	 * really don't want to keep a stale HCD_FLAG_HW_ACCESSIBLE=0
> >  	 */
> >  	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> > +	mb();
> 
> Are these just guesses, or what?

There's no need for sarcasm.  No, they are not guesses.  Ask Ben 
Herrenschmidt if you don't believe me.

> Why not smp_mb__before_clear_bit() or smp_mb__after_clear_bit() ?

Because the code needs to synchronize not with another CPU, but with a USB 
host controller.  Those barriers are necessary even on a UP system.


By the way, what's the idea with this proliferation of little
not-all-that-helpful routines, like smp_mb__before_clear_bit()?  Are there
architectures on which

	smp_mb__before_clear_bit(...);

is significantly superior to

	smp_mb();
	clear_bit(...);

?  (It's certainly not easier to type.)  Is this difference worth noting,
considering how infrequently clear_bit() gets used?

Alan Stern

[patch 0/4] 4 patches for 2.6.15

[Greg Kroah-Hartman]

Hi,

Here are 4 patches against your latest 2.6.15-git tree.  They fix the
following problems and should all go in for 2.6.15:
	- kernel oops in pci code for some thinkpads and other boxes.
	- kernel oops if pci express and pci express hotplug is built
	  into the kernel, instead of a module.
	- build breakage in a i2c driver.
	- memory barriers are needed for USB uhci driver in
	  suspend/resume path, as they were accidentally dropped in a
	  previous patch.

thanks,

greg k-h

[patch 4/4] UHCI: add missing memory barriers

[Greg Kroah-Hartman]

[-- Attachment #1: uhci-add-missing-memory-barriers.patch --]
[-- Type: text/plain, Size: 969 bytes --]

From: Alan Stern <stern@rowland.harvard.edu>

This patch (as617) adds a couple of memory barriers that Ben H. forgot in
his recent suspend/resume fix.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/usb/host/uhci-hcd.c |    2 ++
 1 file changed, 2 insertions(+)

--- gregkh-2.6.orig/drivers/usb/host/uhci-hcd.c
+++ gregkh-2.6/drivers/usb/host/uhci-hcd.c
@@ -717,6 +717,7 @@ static int uhci_suspend(struct usb_hcd *
 	 * at the source, so we must turn off PIRQ.
 	 */
 	pci_write_config_word(to_pci_dev(uhci_dev(uhci)), USBLEGSUP, 0);
+	mb();
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 	uhci->hc_inaccessible = 1;
 	hcd->poll_rh = 0;
@@ -738,6 +739,7 @@ static int uhci_resume(struct usb_hcd *h
 	 * really don't want to keep a stale HCD_FLAG_HW_ACCESSIBLE=0
 	 */
 	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+	mb();
 
 	if (uhci->rh_state == UHCI_RH_RESET)	/* Dead */
 		return 0;

--