Re: [PATCH 10/13] powerpc: Add arch/powerpc mv64x60 PCI setup

On Thu, May 03, 2007 at 09:17:46AM +0200, Arnd Bergmann wrote:

On Wednesday 02 May 2007, Dale Farnsworth wrote:

quoted

Index: linux-2.6-powerpc-df/arch/powerpc/sysdev/Makefile
===================================================================

--- linux-2.6-powerpc-df.orig/arch/powerpc/sysdev/Makefile
+++ linux-2.6-powerpc-df/arch/powerpc/sysdev/Makefile

@@ -16,6 +16,10 @@ obj-$(CONFIG_TSI108_BRIDGE)	+= tsi108_pc
 obj-$(CONFIG_QUICC_ENGINE)	+= qe_lib/
 obj-$(CONFIG_MV64X60)		+= mv64x60_pic.o mv64x60_dev.o
 
+ifeq ($(CONFIG_PCI),y)
+obj-$(CONFIG_MV64X60)		+= mv64x60_pci.o
+endif
+

I'd write this as

mv64x60-$(CONFIG_INDIRECT_PCI)	+= mv64x60_pci.o
obj-$(CONFIG_MV64X60)		+= mv64x60-y

though that doesn't make much difference any more

quoted

+#ifdef CONFIG_SYSFS
+/* 32-bit hex or dec stringified number + '\n' */
+#define MV64X60_VAL_LEN_MAX		11
+#define MV64X60_PCICFG_CPCI_HOTSWAP	0x68
+
+DECLARE_MUTEX(mv64x60_hs_lock);

Please avoid using struct semephores in new code, we now have struct mutex
for this, which gets defined as

static DEFINE_MUTEX(mv64x60_hs_mutex);

This is existing code being moved over from arch/ppc. I'll make the change.

quoted

+static ssize_t mv64x60_hs_reg_read(struct kobject *kobj, char *buf, loff_t off,
+				   size_t count)
+{
+	u32 v;
+	int save_exclude;
+
+	if (off > 0)
+		return 0;
+	if (count < MV64X60_VAL_LEN_MAX)
+		return -EINVAL;
+
+	if (down_interruptible(&mv64x60_hs_lock))
+		return -ERESTARTSYS;
+	save_exclude = mv64x60_pci_exclude_bridge;
+	mv64x60_pci_exclude_bridge = 0;
+	early_read_config_dword(mv64x60_primary_hose, 0, PCI_DEVFN(0, 0),
+				MV64X60_PCICFG_CPCI_HOTSWAP, &v);

Why do you use early_read_config_dword, not pci_read_config_dword()?

As above, this is existing code being moved over from arch/ppc.
I'll make the change.

quoted

+	mv64x60_pci_exclude_bridge = save_exclude;
+	up(&mv64x60_hs_lock);
+
+	return sprintf(buf, "0x%08x\n", v);
+}

<snip>

quoted

+static int mv64x60_exclude_device(u_char bus, u_char devfn)
+{
+       if ((bus == 0 || bus == mv64x60_pci2_busno) &&
+           PCI_SLOT(devfn) == 0 && mv64x60_pci_exclude_bridge)
+               return PCIBIOS_DEVICE_NOT_FOUND;
+
+       return PCIBIOS_SUCCESSFUL;
+}

The locking here looks wrong. If you call mv64x60_exclude_device() from one thread
thread while another one is calling mv64x60_hs_reg_read(), the bridge will
not be excluded.

Good catch.  Again, it's existing code, but clearly there's a race here.
I'm guessing that the mutex above was just to prevent nesting of setting
mv64x60_pci_exclude_bridge. A fix for the race doesn't look easy.
I'll look into it.

Thanks,
-Dale