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

On Wednesday 02 May 2007, Dale Farnsworth wrote:

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

+#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);

+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()?

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

<snip>

+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.

	Arnd <><