[PATCH 0/2] PCI Hotplug: acpiphp wants a 64-bit _SUN

[PATCH 0/2] PCI Hotplug: acpiphp wants a 64-bit _SUN

[Alex Chiang]

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

[PATCH 1/2] PCI Hotplug: acpiphp wants a 64-bit _SUN

[Alex Chiang]

From: Justin Chen <justin.chen@hp.com>

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.

Signed-off-by: Justin Chen <justin.chen@hp.com>
Signed-off-by: Alex Chiang <achiang@hp.com>
---
diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index f9e244d..9bcb6cb 100644
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -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 */
 };
 
diff --git a/drivers/pci/hotplug/acpiphp_core.c b/drivers/pci/hotplug/acpiphp_core.c
index 95b536a..43c10bd 100644
--- a/drivers/pci/hotplug/acpiphp_core.c
+++ b/drivers/pci/hotplug/acpiphp_core.c
@@ -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,
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 955aae4..3affc64 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -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 "

Re: [PATCH 1/2] PCI Hotplug: acpiphp wants a 64-bit _SUN

[Jesse Barnes]

On Thursday, December 11, 2008 10:16 am Alex Chiang wrote:
> From: Justin Chen <justin.chen@hp.com>
>
> 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.
>
> Signed-off-by: Justin Chen <justin.chen@hp.com>
> Signed-off-by: Alex Chiang <achiang@hp.com>

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

Re: [PATCH 1/2] PCI Hotplug: acpiphp wants a 64-bit _SUN

[Matthew Wilcox]

On Thu, Dec 11, 2008 at 10:35:01AM -0800, Jesse Barnes wrote:
> On Thursday, December 11, 2008 10:16 am Alex Chiang wrote:
> > From: Justin Chen <justin.chen@hp.com>
> >
> > 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.
> >
> > Signed-off-by: Justin Chen <justin.chen@hp.com>
> > Signed-off-by: Alex Chiang <achiang@hp.com>
> 
> 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?

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.

Acked-by: Matthew Wilcox <willy@linux.intel.com>

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

Re: [PATCH 1/2] PCI Hotplug: acpiphp wants a 64-bit _SUN

[Jesse Barnes]

On Thursday, December 11, 2008 11:03 am Matthew Wilcox wrote:
> On Thu, Dec 11, 2008 at 10:35:01AM -0800, Jesse Barnes wrote:
> > On Thursday, December 11, 2008 10:16 am Alex Chiang wrote:
> > > From: Justin Chen <justin.chen@hp.com>
> > >
> > > 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.
> > >
> > > Signed-off-by: Justin Chen <justin.chen@hp.com>
> > > Signed-off-by: Alex Chiang <achiang@hp.com>
> >
> > 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?
>
> Testers or reviewers?  Here's my review (done thinking out loud style):

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

Re: [PATCH 1/2] PCI Hotplug: acpiphp wants a 64-bit _SUN

[Ville Syrjälä]

On Thu, Dec 11, 2008 at 11:16:44AM -0700, Alex Chiang wrote:
> From: Justin Chen <justin.chen@hp.com>
> 
> 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.
> 
> Signed-off-by: Justin Chen <justin.chen@hp.com>
> Signed-off-by: Alex Chiang <achiang@hp.com>
> ---
> diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
> index f9e244d..9bcb6cb 100644
> --- a/drivers/pci/hotplug/acpiphp.h
> +++ b/drivers/pci/hotplug/acpiphp.h
> @@ -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) */

Why not make it u64 if that's what ACPI says it should be?

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

Re: [PATCH 1/2] PCI Hotplug: acpiphp wants a 64-bit _SUN

[Alex Chiang]

* Ville Syrjälä <syrjala@sci.fi>:
> On Thu, Dec 11, 2008 at 11:16:44AM -0700, Alex Chiang wrote:
> > From: Justin Chen <justin.chen@hp.com>
> > 
> > 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.
> > 
> > Signed-off-by: Justin Chen <justin.chen@hp.com>
> > Signed-off-by: Alex Chiang <achiang@hp.com>
> > ---
> > diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
> > index f9e244d..9bcb6cb 100644
> > --- a/drivers/pci/hotplug/acpiphp.h
> > +++ b/drivers/pci/hotplug/acpiphp.h
> > @@ -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) */
> 
> Why not make it u64 if that's what ACPI says it should be?

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

[PATCH 2/2] PCI Hotplug: acpiphp whitespace cleanup

[Alex Chiang]

Clean up whitespace.

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

Signed-off-by: Alex Chiang <achiang@hp.com>
---
diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index f9e244d..1cc5eee 100644
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -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)
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 955aae4..03ce8dc 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -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;

Re: [PATCH 2/2] PCI Hotplug: acpiphp whitespace cleanup

[Jesse Barnes]

On Thursday, December 11, 2008 10:17 am Alex Chiang wrote:
> Clean up whitespace.
>
> Setting 'let c_space_errors=1' in .vimrc shows all sorts of
> ugliness. ;)

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

-- 
Jesse Barnes, Intel Open Source Technology Center