PCI Hotplug: acpiphp wants a 64-bit _SUN

Hi Jesse,

Here are two patches for acpiphp.

Although Willy's patch 27663c5855b10af9ec67bc7dfba001426ba21222 (ACPI: Change acpi_evaluate_integer to support 64-bit on 32-bit kernels) is now in, the problem we were initially trying to fix before discovering that patch still exists.

The problem is that on certain HP systems, the _SUN method does utilize the full 64 bits. Declaring sun in struct acpiphp_slot as a u32 leads to name collisions on our systems.

The first patch from Justin changes the declaration of sun to unsigned long long. It has been tested and verified that it fixes the name collision issue on our machines.

I also did a build test in a 32-bit x86 environment to make sure we don't get any compiler warnings, and it was clean.

Please consider that patch for 2.6.28. It's late-breaking, but I feel is low-risk.

The second patch is a mere whitespace patch. Since I was in the file anyway, I figured I'd clean it up a bit.

[setting 'let c_space_errors=1' in a .vimrc shows all kinds of wonderful stuff ;) ]

Thanks.

/ac

From: Justin Chen <[email protected]>

Certain HP machines require the full 64 bits of _SUN as allowed by the ACPI spec. Without this change, we get name collisions in the lower 32 bits of the _SUN returned by firmware.

---

index f9e244d..9bcb6cb 100644
@@ -113,7 +113,7 @@ struct acpiphp_slot {
 
 	u8		device;		/* pci device# */
 
-	u32		sun;		/* ACPI _SUN (slot unique number) */
+	unsigned long long sun;		/* ACPI _SUN (slot unique number) */
 	u32		flags;		/* see below */
 };
 

index 95b536a..43c10bd 100644
@@ -337,7 +337,7 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
 	slot->hotplug_slot->info->cur_bus_speed = PCI_SPEED_UNKNOWN;
 
 	acpiphp_slot->slot = slot;
-	snprintf(name, SLOT_NAME_SIZE, "%u", slot->acpi_slot->sun);
+	snprintf(name, SLOT_NAME_SIZE, "%llu", slot->acpi_slot->sun);
 
 	retval = pci_hp_register(slot->hotplug_slot,
 					acpiphp_slot->bridge->pci_bus,

index 955aae4..3affc64 100644
@@ -255,13 +255,13 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 
 		bridge->nr_slots++;
 
-		dbg("found ACPI PCI Hotplug slot %d at PCI %04x:%02x:%02x\n",
+		dbg("found ACPI PCI Hotplug slot %llu at PCI %04x:%02x:%02x\n",
 				slot->sun, pci_domain_nr(bridge->pci_bus),
 				bridge->pci_bus->number, slot->device);
 		retval = acpiphp_register_hotplug_slot(slot);
 		if (retval) {
 			if (retval == -EBUSY)
-				warn("Slot %d already registered by another "
+				warn("Slot %llu already registered by another "
 					"hotplug driver\n", slot->sun);
 			else
 				warn("acpiphp_register_hotplug_slot failed "

It's very late in the cycle, so I'd like to get acks from at least a couple of other testers for this one... Any volunteers?

Thanks,
Jesse

Testers or reviewers? Here's my review (done thinking out loud style):

$ grep -w sun drivers/pci/hotplug/acpiphp*
1. drivers/pci/hotplug/acpiphp_core.c:     snprintf(name, SLOT_NAME_SIZE, "%u", slot->acpi_slot->sun);
2. drivers/pci/hotplug/acpiphp_glue.c:     unsigned long long adr, sun;
3. drivers/pci/hotplug/acpiphp_glue.c:     status = acpi_evaluate_integer(handle, "_SUN", NULL, &sun);
4. drivers/pci/hotplug/acpiphp_glue.c:             sun = bridge->nr_slots+1;
5. drivers/pci/hotplug/acpiphp_glue.c:                     if (slot->sun != sun)
6. drivers/pci/hotplug/acpiphp_glue.c:             slot->sun = sun;
7. drivers/pci/hotplug/acpiphp_glue.c: slot->sun, pci_domain_nr(bridge->pci_bus),
8. drivers/pci/hotplug/acpiphp_glue.c: "hotplug driver\n", slot->sun);
9. drivers/pci/hotplug/acpiphp.h:  u32             sun;            /* ACPI _SUN (slot unique number) */
10. drivers/pci/hotplug/acpiphp_ibm.c:#define hpslot_to_sun(A) (((struct
slot *)((A)->private))->acpi_slot->sun)
11. drivers/pci/hotplug/acpiphp_ibm.c:              u8  sun;

#1 is a printk ... string changed to %llu. Fine.

#2 is a declaration of a local variable, not related.
#3 is a use of said local variable, fine.
#4 ditto
#5 compares said local variable to the newly widened element. Good. #6 is an assignment. Good.
#7 is also a printk. Also changed to %llu. Good.
#8 Ditto.
#9 is the change in question.

#10 and #11 are in the acpiphp_ibm driver. This driver will truncate the stored value of 'sun' to 32-bit ... however, it then compares the result for equality with an 8-bit value. We do no harm here by storing a 64-bit value in sun instead of a 32-bit value. If IBM want to produce machines that use more than 8 bits for slot number, they will have to change their firmware interface anyway -- their ACPI table uses 8-bit wide data types for it.

Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Well, review is good too; I stuffed this one into my for-linus branch (the whitespace one went into linux-next), thanks for checking it out.

Jesse Barnes, Intel Open Source Technology Center

* Ville Syrjälä <[email protected]>:

Some 64-bit architectures typedef u64 to unsigned long, and some to unsigned long long.

I'd prefer not to have to cast it to a (ULL) every time I want to printk.

Thanks.

/ac

Clean up whitespace.

Setting 'let c_space_errors=1' in .vimrc shows all sorts of ugliness. ;)

---

index f9e244d..1cc5eee 100644
@@ -44,7 +44,7 @@
 	do {							\
 		if (acpiphp_debug)				\
 			printk(KERN_DEBUG "%s: " format,	\
-				MY_NAME , ## arg); 		\
+				MY_NAME , ## arg);		\
 	} while (0)
 #define err(format, arg...) printk(KERN_ERR "%s: " format, MY_NAME , ## arg)
 #define info(format, arg...) printk(KERN_INFO "%s: " format, MY_NAME , ## arg)

index 955aae4..03ce8dc 100644
@@ -160,9 +160,9 @@ static int post_dock_fixups(struct notifier_block *nb, unsigned long val,
 
 	if (((buses >> 8) & 0xff) != bus->secondary) {
 		buses = (buses & 0xff000000)
-	     		| ((unsigned int)(bus->primary)     <<  0)
-	     		| ((unsigned int)(bus->secondary)   <<  8)
-	     		| ((unsigned int)(bus->subordinate) << 16);
+			| ((unsigned int)(bus->primary)     <<  0)
+			| ((unsigned int)(bus->secondary)   <<  8)
+			| ((unsigned int)(bus->subordinate) << 16);
 		pci_write_config_dword(bus->self, PCI_PRIMARY_BUS, buses);
 	}
 	return NOTIFY_OK;

Yay whitespace cleanups. :) Applied to linux-next.

Jesse Barnes, Intel Open Source Technology Center