PCI, ACPI: Physical PCI slot objects

Rename the slot to be the contents of the 'path' sysfs attribute, and delete the attribute. The mapping from pci address to slot name is supposed to be done through the 'address' file, which will be provided automatically later in this series of patches.

Signed-off-by Alex Chiang <[email protected]>

Signed-off-by Matthew Wilcox <[email protected]>

---
 drivers/pci/hotplug/sgi_hotplug.c |   32 +-------------------------------
 1 files changed, 1 insertions(+), 31 deletions(-)

(quoted diff) +0 -0

b/drivers/pci/hotplug/sgi_hotplug.c index ef07c36..693519e 100644

drivers/pci/hotplug/sgi_hotplug.c +1 -31

@@ -91,21 +91,6 @@ static struct hotplug_slot_ops sn_hotplug_slot_ops = {
 
 static DEFINE_MUTEX(sn_hotplug_mutex);
 
-static ssize_t path_show (struct hotplug_slot *bss_hotplug_slot,
-	       		  char *buf)
-{
-	int retval = -ENOENT;
-	struct slot *slot = bss_hotplug_slot->private;
-
-	if (!slot)
-		return retval;
-
-	retval = sprintf (buf, "%s\n", slot->physical_path);
-	return retval;
-}
-
-static struct hotplug_slot_attribute sn_slot_path_attr = __ATTR_RO(path);
-
 static int sn_pci_slot_valid(struct pci_bus *pci_bus, int device)
 {
 	struct pcibus_info *pcibus_info;

@@ -173,18 +158,10 @@ static int sn_hp_slot_private_alloc(struct
 hotplug_slot *bss_hotplug_slot, return -ENOMEM;
 	bss_hotplug_slot->private = slot;
 
-	bss_hotplug_slot->name = kmalloc(SN_SLOT_NAME_SIZE, GFP_KERNEL);
-	if (!bss_hotplug_slot->name) {
-		kfree(bss_hotplug_slot->private);
-		return -ENOMEM;
-	}
+	bss_hotplug_slot->name = slot->physical_path;
 
 	slot->device_num = device;
 	slot->pci_bus = pci_bus;
-	sprintf(bss_hotplug_slot->name, "%04x:%02x:%02x",
-		pci_domain_nr(pci_bus),
-		((u16)pcibus_info->pbi_buscommon.bs_persist_busnum),
-		device + 1);
 
 	sn_generate_path(pci_bus, slot->physical_path);
 

@@ -203,8 +180,6 @@ static struct hotplug_slot * sn_hp_destroy(void)
 		bss_hotplug_slot = slot->hotplug_slot;
 		list_del(&((struct slot *)bss_hotplug_slot->private)->
 			 hp_list);
-		sysfs_remove_file(&bss_hotplug_slot->kobj,
-				  &sn_slot_path_attr.attr);
 		break;
 	}
 	return bss_hotplug_slot;

@@ -653,11 +628,6 @@ static int sn_hotplug_slot_register(struct pci_bus
 *pci_bus) rc = pci_hp_register(bss_hotplug_slot);
 		if (rc)
 			goto register_err;
-
-		rc = sysfs_create_file(&bss_hotplug_slot->kobj,
-				       &sn_slot_path_attr.attr);
-		if (rc)
-			goto register_err;
 	}
 	dev_dbg(&pci_bus->self->dev, "Registered bus with hotplug\n");
 	return rc;

Quoted 8 lines

drivers/pci/hotplug/sgi_hotplug.c +1 -5

-	bss_hotplug_slot->name = kmalloc(SN_SLOT_NAME_SIZE, GFP_KERNEL);
-	if (!bss_hotplug_slot->name) {
-		kfree(bss_hotplug_slot->private);
-		return -ENOMEM;
-	}
+	bss_hotplug_slot->name = slot->physical_path;

drivers/pci/hotplug/sgi_hotplug.c static int sn_hp_slot_private_alloc(struct hotplug_slot *bss_hotplug_slot, · line 187

Quoted 8 lines

drivers/pci/hotplug/sgi_hotplug.c +0 -4

 	slot->device_num = device;
 	slot->pci_bus = pci_bus;
-	sprintf(bss_hotplug_slot->name, "%04x:%02x:%02x",
-		pci_domain_nr(pci_bus),
-		((u16)pcibus_info->pbi_buscommon.bs_persist_busnum),
-		device + 1);

Quoted 8 lines

Quoted 8 lines

drivers/pci/hotplug/sgi_hotplug.c +1 -5

-	bss_hotplug_slot->name = kmalloc(SN_SLOT_NAME_SIZE, GFP_KERNEL);
-	if (!bss_hotplug_slot->name) {
-		kfree(bss_hotplug_slot->private);
-		return -ENOMEM;
-	}
+	bss_hotplug_slot->name = slot->physical_path;

Quoted 9 lines

drivers/pci/hotplug/sgi_hotplug.c static int sn_hp_slot_private_alloc(struct hotplug_slot *bss_hotplug_slot, · line 187

Quoted 8 lines

drivers/pci/hotplug/sgi_hotplug.c +0 -4

 	slot->device_num = device;
 	slot->pci_bus = pci_bus;
-	sprintf(bss_hotplug_slot->name, "%04x:%02x:%02x",
-		pci_domain_nr(pci_bus),
-		((u16)pcibus_info->pbi_buscommon.bs_persist_busnum),
-		device + 1);

Ugh ... it has been a while since I've looked at or owned this code ;) -- but I do seem to recall that on SGI boxes the slot's name was different from the physical path of the device (ie, what was stamped on the back of the PCI hotplug chassis was different from Linux' PCI name) -- OTOH, I haven't recently looked at what slot->physical_path has been initialized to, so they might now be one-and-the-same ....

- Make pci_slot the primary sysfs entity. hotplug_slot becomes a subsidiary structure.
o pci_create_slot() creates and registers a slot with the PCI core
o pci_slot_add_hotplug() gives it hotplug capability

- Change the prototype of pci_hp_register() to take the bus and slot number (on parent bus) as parameters.

- Remove all the ->get_address methods since this functionality is now handled by pci_slot directly.

Quoted 8 lines

- Make pci_slot the primary sysfs entity. hotplug_slot becomes a subsidiary structure.
o pci_create_slot() creates and registers a slot with the PCI core
o pci_slot_add_hotplug() gives it hotplug capability

- Change the prototype of pci_hp_register() to take the bus and slot number (on parent bus) as parameters.

Quoted 2 lines

- Remove all the ->get_address methods since this functionality is now handled by pci_slot directly.

This describes what you did, but not why you are doing this, making it a pretty bad changelog comment.

Can you refresh my memory as to the "why" for all of this

and how you are handling machines that do not export this information at all?

oh, and don't put "extern" in a .c file,

and call kobject_uevent in the same function that you added the kobject in, unless there is a very good reason to do so, otherwise you just missed all of those events...

Hi Greg,

* Greg KH <[email protected]>:

Quoted 9 lines · Alex Chiang

Quoted 8 lines

- Make pci_slot the primary sysfs entity. hotplug_slot becomes a subsidiary structure.
o pci_create_slot() creates and registers a slot with the PCI core
o pci_slot_add_hotplug() gives it hotplug capability

- Change the prototype of pci_hp_register() to take the bus and slot number (on parent bus) as parameters.

Quoted 7 lines

Quoted 2 lines

- Remove all the ->get_address methods since this functionality is now handled by pci_slot directly.

This describes what you did, but not why you are doing this, making it a pretty bad changelog comment.

Can you refresh my memory as to the "why" for all of this

How about this:

Currently, /sys/bus/pci/slots/ only exposes hotplug attributes when a hotplug driver is loaded, but PCI slots have attributes such as address, speed, width, etc. that are not related to hotplug at all.

Introduce pci_slot as the primary data structure and kobject model. Hotplug attributes described in hotplug_slot become a secondary structure associated with the pci_slot.

This patch only creates the infrastructure that allows the separation of PCI slot attributes and hotplug attributes. In this patch, the PCI hotplug core remains the only user of this infrastructure, and thus, /sys/bus/pci/slots/ will still only become populated when a hotplug driver is loaded.

A later patch in this series will add a second user of this new infrastructure and demonstrate splitting the task of exposing pci_slot attributes from hotplug_slot attributes.

- Make pci_slot the primary sysfs entity. hotplug_slot becomes a subsidiary structure.
o pci_create_slot() creates and registers a slot with the PCI core
o pci_slot_add_hotplug() gives it hotplug capability

- Change the prototype of pci_hp_register() to take the bus and slot number (on parent bus) as parameters.

- Remove all the ->get_address methods since this functionality is now handled by pci_slot directly.

Quoted 2 lines

and how you are handling machines that do not export this information at all?

With this patch, there is no change from existing behavior that users see; only infrastructure is changed. The existing hotplug drivers will continue to expose whatever they were exposing before (when they are loaded).

Quoted 3 lines

and call kobject_uevent in the same function that you added the kobject in, unless there is a very good reason to do so, otherwise you just missed all of those events...

I *think* I might actually have a "good" reason, but welcome your feedback.

In this patch, pci_create_slot() is responsible for kobject_init_and_add, and it adds the 'address' attribute in sysfs.

The caller of pci_create_slot() is pci_hp_register, and it is calling kobject_uevent after pci_create_slot, because it still has to expose the hotplug attributes in sysfs, which can only happen *after* the pci_slot is created.

I don't think I want to emit the uevent until those hotplug attributes are exposed, right?

This kinda seems like a stupid design, but the next patch in the series adds another callsite for pci_create_slot. The next patch is detecting physical slots described by ACPI, but doesn't know (or care) about their hotplug capabilities.

I don't think it makes sense to be emitting uevents simply upon detecting a physical slot.

[over time, I hope to add more functionality to pci_slot, such as displaying speed, width, etc., but right now, we only get the address]

One alternative I can think of -- which would further complicate this model that I'm introducing -- would be to make hotplug_slot a kobject too, and then let pci_slot emit a uevent upon physical slot detection, and then let pci_hp_register emit another uevent when the hotplug_slot is created / added to sysfs. But I must admit, I don't really like that alternative.

Detect all physical PCI slots as described by ACPI, and create entries in /sys/bus/pci/slots/.

Not all physical slots are hotpluggable, and the acpiphp module does not detect them. Now we know the physical PCI geography of our system, without caring about hotplug.

What is the guarantee that the names of these slots are correct and do not happen to be the same as the hotpluggable ones?

Why show this information on machines that can not do anything with these slots at all? Will that not just confuse people?

Quoted 2 lines · Greg KH

What is the guarantee that the names of these slots are correct and do not happen to be the same as the hotpluggable ones?

That would be a bug -- and yes, bugs happen, and we have to deal with them.

Quoted 2 lines

Why show this information on machines that can not do anything with these slots at all? Will that not just confuse people?

Only for people who think that /sys/bus/pci/slots/ is for hotpluggable slots only. There is plenty of useful information available for slots that aren't hotpluggable (eg bus address, speed, width, error status).

Quoted 4 lines · Matthew Wilcox

Quoted 2 lines · Greg KH

What is the guarantee that the names of these slots are correct and do not happen to be the same as the hotpluggable ones?

Quoted 2 lines

That would be a bug -- and yes, bugs happen, and we have to deal with them.

My main concern is that BIOS vendors will not fix these bugs, as no other OS cares/does this kind of thing today. The ammount of bad information out there might be quite large, and I think this was confirmed by some initial testing of IBM systems, right?

Quoted 5 lines · Greg KH

Quoted 4 lines · Matthew Wilcox

Quoted 2 lines · Greg KH

What is the guarantee that the names of these slots are correct and do not happen to be the same as the hotpluggable ones?

Quoted 7 lines

Quoted 2 lines

That would be a bug -- and yes, bugs happen, and we have to deal with them.

My main concern is that BIOS vendors will not fix these bugs, as no other OS cares/does this kind of thing today. The ammount of bad information out there might be quite large, and I think this was confirmed by some initial testing of IBM systems, right?

Yeah, but there's a flip side to this too: if no one uses the data, no one will complain when it's wrong. If Linux starts making it easy to see this stuff, there's a chance system vendors will start taking an extra 5 min. before shipment to make sure that the BIOS info is up to date...

OTOH, I'm not sure which is worse, bad data or no data.

bad data is worse.

And then there's the machines with duplicate slot names, how does this code handle PCI slots with that? I think some of the IBM machines had non-hotplug slots named the same as the hotplug slots, right?

This stuff needs a _lot_ of testing on a lot of different machines, and a sane way to fall-back if there are errors to ensure that working machines don't break.

Quoted 15 lines · Jesse Barnes

Quoted 6 lines · Greg KH

Quoted 5 lines · Matthew Wilcox

Quoted 3 lines · Greg KH

What is the guarantee that the names of these slots are correct and do not happen to be the same as the hotpluggable ones?

Quoted 7 lines

Quoted 2 lines

That would be a bug -- and yes, bugs happen, and we have to deal with them.

My main concern is that BIOS vendors will not fix these bugs, as no other OS cares/does this kind of thing today. The ammount of bad information out there might be quite large, and I think this was confirmed by some initial testing of IBM systems, right?

Quoted 7 lines

Yeah, but there's a flip side to this too: if no one uses the data, no one will complain when it's wrong. If Linux starts making it easy to see this stuff, there's a chance system vendors will start taking an extra 5 min. before shipment to make sure that the BIOS info is up to date...

OTOH, I'm not sure which is worse, bad data or no data.

bad data is worse.

And then there's the machines with duplicate slot names, how does this code handle PCI slots with that? I think some of the IBM machines had non-hotplug slots named the same as the hotplug slots, right?

This stuff needs a _lot_ of testing on a lot of different machines, and a sane way to fall-back if there are errors to ensure that working machines don't break.

And then there's the issue with userspace programs only expecting hotplugable slots in the slots/ directory...

thanks,

greg k-h

Quoted 19 lines · Greg KH

Quoted 5 lines · Matthew Wilcox

Quoted 4 lines · Greg KH

Quoted 2 lines

What is the guarantee that the names of these slots are correct and do not happen to be the same as the hotpluggable ones?

Quoted 7 lines

Quoted 2 lines

That would be a bug -- and yes, bugs happen, and we have to deal with them.

My main concern is that BIOS vendors will not fix these bugs, as no other OS cares/does this kind of thing today. The ammount of bad information out there might be quite large, and I think this was confirmed by some initial testing of IBM systems, right?

Yeah, but there's a flip side to this too: if no one uses the data, no one will complain when it's wrong. If Linux starts making it easy to see this stuff, there's a chance system vendors will start taking an extra 5 min. before shipment to make sure that the BIOS info is up to date...

Quoted 1 line

OTOH, I'm not sure which is worse, bad data or no data.

bad data is worse.

And then there's the machines with duplicate slot names, how does this code handle PCI slots with that? I think some of the IBM machines had non-hotplug slots named the same as the hotplug slots, right?

This stuff needs a _lot_ of testing on a lot of different machines, and a sane way to fall-back if there are errors to ensure that working machines don't break.

And then there's the issue with userspace programs only expecting hotplugable slots in the slots/ directory...

It wasn't the IBM machine that was breaking; it was Fujitsu. They were returning an error code (the numerical value 1023) when I called the _SUN method on a slot object that existed in the ACPI namespace but was not present (as reported by the _STA method).

By the time I got that error report, I'd already dropped the duplicate name detection code, and was letting the kobject infrastructure warn about duplicate names because for my test cases, refcounting was a better solution.

[Kenji-san from Fujitsu seemed to be ok with the progress I'd made at the time, he can speak up if he's changed his mind ;)]

* Greg KH <[email protected]>:

Quoted 12 lines · Jesse Barnes

Quoted 11 lines · Greg KH

Quoted 10 lines · Matthew Wilcox

Quoted 5 lines · Greg KH

Quoted 2 lines

What is the guarantee that the names of these slots are correct and do not happen to be the same as the hotpluggable ones?

That would be a bug -- and yes, bugs happen, and we have to deal with them.

My main concern is that BIOS vendors will not fix these bugs, as no other OS cares/does this kind of thing today. The ammount of bad information out there might be quite large, and I think this was confirmed by some initial testing of IBM systems, right?

Quoted 8 lines

Quoted 3 lines

Yeah, but there's a flip side to this too: if no one uses the data, no one will complain when it's wrong. If Linux starts making it easy to see this stuff, there's a chance system vendors will start taking an extra 5 min. before shipment to make sure that the BIOS info is up to date...

OTOH, I'm not sure which is worse, bad data or no data.

bad data is worse.

And then there's the machines with duplicate slot names, how does this code handle PCI slots with that? I think some of the IBM machines had non-hotplug slots named the same as the hotplug slots, right?

At one point, I had some code in there to stick the names of the slots into a linked list and walk through it to try and detect duplicate slot names, but after a few iterations, the cases I was dealing with, it turned out to be easier to refcount them.

[my machines did not have colliding names between hp and non-hp slots, it was more like seeing the same SxFy object appear multiple times in the namespace and trying to create them multiple times.]

It wasn't the IBM machine that was breaking; it was Fujitsu. They were returning an error code (the numerical value 1023) when I called the _SUN method on a slot object that existed in the ACPI namespace but was not present (as reported by the _STA method).

By the time I got that error report, I'd already dropped the duplicate name detection code, and was letting the kobject infrastructure warn about duplicate names because for my test cases, refcounting was a better solution.

[Kenji-san from Fujitsu seemed to be ok with the progress I'd made at the time, he can speak up if he's changed his mind ;)]

But even that is not the error case you're describing, where there is clear name collision of two physical slots in the machine, one being hotplug, the other non-hotplug.

Maybe I would have to add some duplicate name detection code back in there but...

Quoted 6 lines

This stuff needs a _lot_ of testing on a lot of different machines, and a sane way to fall-back if there are errors to ensure that working machines don't break.

And then there's the issue with userspace programs only expecting hotplugable slots in the slots/ directory...

Yes -- totally agreed. And I'd like to see actual examples of name collisions or userspace breakage to get a better idea of how to handle real world problems rather than writing some crummy code based on what my limited imagination can think of.

So how to get this test coverage? -mm? linux-next?

Thanks.

/ac

Hi Alex-san,

Sorry for the delay in responding.

Quoted 10 lines

It wasn't the IBM machine that was breaking; it was Fujitsu. They were returning an error code (the numerical value 1023) when I called the _SUN method on a slot object that existed in the ACPI namespace but was not present (as reported by the _STA method). By the time I got that error report, I'd already dropped the duplicate name detection code, and was letting the kobject infrastructure warn about duplicate names because for my test cases, refcounting was a better solution.
[Kenji-san from Fujitsu seemed to be ok with the progress I'd made at the time, he can speak up if he's changed his mind ;)]

Unfortunatelly, I have not tried the new version of slot detection driver because of the lack of test environment. Maybe we need more several days to wait for test environment.
BTW, does the new one fixes the issue I reported before? I could not find it in the changelog. IIRC, this issue was difficult to solve because the root cause of this issue is from the difference of interpretation of ACPI spec between HP and Fujitsu (I still don't think it's a good idea to evaluate _SUN for the device object whose _STA is 0).

So given that it's two versus one, I modified my drivers/acpi/pci_slot module to consider Fujitsu machines to be a quirk. :)

Can you please test patches 1 and 2 that I sent out as v8 of my series, but replace patch 3 with this patch?

So given that it's two versus one, I modified my drivers/acpi/pci_slot module to consider Fujitsu machines to be a quirk. :)

Can you please test patches 1 and 2 that I sent out as v8 of my series, but replace patch 3 with this patch?

Quoted 10 lines

Quoted 10 lines

It wasn't the IBM machine that was breaking; it was Fujitsu. They were returning an error code (the numerical value 1023) when I called the _SUN method on a slot object that existed in the ACPI namespace but was not present (as reported by the _STA method). By the time I got that error report, I'd already dropped the duplicate name detection code, and was letting the kobject infrastructure warn about duplicate names because for my test cases, refcounting was a better solution.
[Kenji-san from Fujitsu seemed to be ok with the progress I'd made at the time, he can speak up if he's changed his mind ;)]

Quoted 8 lines

Unfortunatelly, I have not tried the new version of slot detection
driver because of the lack of test environment. Maybe we need more
several days to wait for test environment. BTW, does the new one fixes the issue I reported before? I could not find it in the changelog. IIRC, this issue was difficult to solve
because the root cause of this issue is from the difference of
interpretation of ACPI spec between HP and Fujitsu (I still don't
think it's a good idea to evaluate _SUN for the device object whose
_STA is 0).

It looks like we disagree on how to interpret the spec (IBM machines interpret the spec the same way HP machines do).

So given that it's two versus one, I modified my drivers/acpi/pci_slot module to consider Fujitsu machines to be a quirk. :)

Can you please test patches 1 and 2 that I sent out as v8 of my series, but replace patch 3 with this patch?

Please note -- you will probably need to modify this block:

	{ 
	 .callback = do_sta_before_sun,
	 .ident = "Fujitsu Limited Primequest",
	 .matches = {
	 	DMI_MATCH(DMI_BIOS_VENDOR, "Fujitsu"),
		DMI_MATCH(DMI_BIOS_VERSION, "2.35"),
		},
	},

To get the correct values for DMI_BIOS_VENDOR and
DMI_BIOS_VERSION, because I was just guessing.

1) I checked ACPI spec again and again, but I could not find any reason to add Fujitsu servers to quirks list. So I'd like you to add HP servers to the quirks list. I'll send the following patches followed by this e-mail.

2) The ACPI PCI slot detection driver would change the slot names of some hotplug drivers (at least I checked shpchp and pciehp). And the name of slots are depending on the order of driver loading. For example, on my system which has several SHPCHP slots and PCIEHP slots, the name of PCIEHP slots are changed as follows. Please note that PCIEHP based slots are 0034_0027 and 0032_0026, and others are SHPCHP based slots.

- Without ACPI PCI slot detection driver

     # ls /sys/bus/pci/slots/
     0009_0016  0014_0018  0019_0020  0021_0022  0034_0027
     0011_0017  0016_0019  0021_0021  0032_0026

- With ACPI PCI slot detection driver

     # ls /sys/bus/pci/slots/
     0009_0016  0014_0018  0019_0020  0021_0022  27
     0011_0017  0016_0019  0021_0021  26

- Unify slot names among all hotplug drivers

Quoted 4 lines · Kenji Kaneshige

1) I checked ACPI spec again and again, but I could not find any reason to add Fujitsu servers to quirks list. So I'd like you to add HP servers to the quirks list. I'll send the following patches followed by this e-mail.

Alex pointed out that IBM interprets the spec the same way that HP does. Are there any other machines that follow the spec the same way that Fujitsu does?

Quoted 8 lines · Matthew Wilcox

Quoted 4 lines · Kenji Kaneshige

1) I checked ACPI spec again and again, but I could not find any reason to add Fujitsu servers to quirks list. So I'd like you to add HP servers to the quirks list. I'll send the following patches followed by this e-mail.

Alex pointed out that IBM interprets the spec the same way that HP does. Are there any other machines that follow the spec the same way that Fujitsu does?

So Kenji is arguing that _STA should be called before _SUN (iow that the _SUN value is bogus unless _STA indicates that the slot is powered)?

That seems
harder for the user; often times you want to find a slot even if it's powered down & empty (say to add a new card)... It looks like the current code won't provide a slot name on Fujitsu machines in this case, or am I missing something?

Quoted 4 lines · Kenji Kaneshige

1) I checked ACPI spec again and again, but I could not find any reason to add Fujitsu servers to quirks list. So I'd like you to add HP servers to the quirks list. I'll send the following patches followed by this e-mail.

Alex pointed out that IBM interprets the spec the same way that HP does. Are there any other machines that follow the spec the same way that Fujitsu does?

Quoted 13 lines

2) The ACPI PCI slot detection driver would change the slot names of some hotplug drivers (at least I checked shpchp and pciehp). And the name of slots are depending on the order of driver loading. For example, on my system which has several SHPCHP slots and PCIEHP slots, the name of PCIEHP slots are changed as follows. Please note that PCIEHP based slots are 0034_0027 and 0032_0026, and others are SHPCHP based slots.

- Without ACPI PCI slot detection driver

     # ls /sys/bus/pci/slots/
     0009_0016  0014_0018  0019_0020  0021_0022  0034_0027
     0011_0017  0016_0019  0021_0021  0032_0026

Quoted 4 lines

- With ACPI PCI slot detection driver

     # ls /sys/bus/pci/slots/
     0009_0016  0014_0018  0019_0020  0021_0022  27
     0011_0017  0016_0019  0021_0021  26

I hadn't realised that patch got in to put the bus name in as a uniquifier. I thought we'd rejected it because the problem only occurred on one box with bad firmware.

Quoted 1 line

- Unify slot names among all hotplug drivers

That is the plan. I'm not sure why the shpc slots aren't renamed in this revision of the patch -- maybe Alex dropped that part?

Hi Alex-san,

Quoted 23 lines · Alex Chiang

Quoted 10 lines

Quoted 10 lines

It wasn't the IBM machine that was breaking; it was Fujitsu. They were returning an error code (the numerical value 1023) when I called the _SUN method on a slot object that existed in the ACPI namespace but was not present (as reported by the _STA method). By the time I got that error report, I'd already dropped the duplicate name detection code, and was letting the kobject infrastructure warn about duplicate names because for my test cases, refcounting was a better solution.
[Kenji-san from Fujitsu seemed to be ok with the progress I'd made at the time, he can speak up if he's changed his mind ;)]

Quoted 9 lines

Unfortunatelly, I have not tried the new version of slot detection driver because of the lack of test environment. Maybe we need more several days to wait for test environment.
BTW, does the new one fixes the issue I reported before? I could not find it in the changelog. IIRC, this issue was difficult to solve because the root cause of this issue is from the difference of interpretation of ACPI spec between HP and Fujitsu (I still don't think it's a good idea to evaluate _SUN for the device object whose _STA is 0).

It looks like we disagree on how to interpret the spec (IBM machines interpret the spec the same way HP machines do).

Quoted 23 lines · On Tue, 11 Mar 2008 22:10:40 +0900 Kenji Kaneshige…

Hi Alex-san,

Quoted 20 lines · Alex Chiang

Quoted 19 lines

Quoted 10 lines

It wasn't the IBM machine that was breaking; it was Fujitsu. They were returning an error code (the numerical value 1023) when I called the _SUN method on a slot object that existed in the ACPI namespace but was not present (as reported by the _STA method). By the time I got that error report, I'd already dropped the duplicate name detection code, and was letting the kobject infrastructure warn about duplicate names because for my test cases, refcounting was a better solution.
[Kenji-san from Fujitsu seemed to be ok with the progress I'd made at the time, he can speak up if he's changed his mind ;)]

Unfortunatelly, I have not tried the new version of slot detection driver because of the lack of test environment. Maybe we need more several days to wait for test environment.
BTW, does the new one fixes the issue I reported before? I could not find it in the changelog. IIRC, this issue was difficult to solve because the root cause of this issue is from the difference of interpretation of ACPI spec between HP and Fujitsu (I still don't think it's a good idea to evaluate _SUN for the device object whose _STA is 0).

Quoted 3 lines

Quoted 3 lines

It looks like we disagree on how to interpret the spec (IBM machines interpret the spec the same way HP machines do).

OK, let me see if I can understand what the issue is here. Please correct me if I'm wrong. The debate is about whether or not it is legitimate to call _SUN if _STA indicates that the device is not present and functional.

I've checked ACPI 3.0b, and from what I've read, you may not evaluate _SUN until _INI is called. And _INI should not be called unless _STA indicates that a device is present and functional.

On my machine, it is legal to evaluate S1F0._SUN independent of S1F0._STA because L001._INI has already been evaluated.

It would be helpful to know what Fujitsu's namespace looks like. If Fujitsu slot objects contain _STA and _INI, then I agree with Kenji-san -- I definitely need to check _STA before evaluating _SUN.

But in any case, I think both HP and Fujitsu firmware are doing legal things -- neither firmware is breaking the spec.

If one list is shorter than the other, then that should be the list to put in the kernel, and the default behavior should be "majority rule".

Hi Alex-san,

Quoted 6 lines

On my machine, it is legal to evaluate S1F0._SUN independent of S1F0._STA because L001._INI has already been evaluated. It would be helpful to know what Fujitsu's namespace looks like. If Fujitsu slot objects contain _STA and _INI, then I agree with Kenji-san -- I definitely need to check _STA before evaluating _SUN.

Thank you for explanation. Maybe I understood the summary of implementation of HP firmware. But how to use or where to put _INI method in the ACPI namespace never becomes reasonable reason why your driver may ignore _STA before evaluating _SUN.

Quoted 2 lines

But in any case, I think both HP and Fujitsu firmware are doing legal things -- neither firmware is breaking the spec.

My understanding of your explanation so far is:

- evaluating _SUN without checking _STA doesn't cause problem, from the view point of HP's implementation.
- some IBM machine is doing same as HP

I never think those are reasonable reasons why ignoring _STA before evaluating _SUN is legal. Am I missing something?

Quoted 3 lines

If one list is shorter than the other, then that should be the list to put in the kernel, and the default behavior should be "majority rule".

I don't want to consider "majority rule" before I understand why ignoring _STA is legal.

Hi Kenji-san,

* Kenji Kaneshige <[email protected]>:

Quoted 23 lines

Hi Alex-san,

Quoted 6 lines

On my machine, it is legal to evaluate S1F0._SUN independent of S1F0._STA because L001._INI has already been evaluated. It would be helpful to know what Fujitsu's namespace looks like. If Fujitsu slot objects contain _STA and _INI, then I agree with Kenji-san -- I definitely need to check _STA before evaluating _SUN.

Thank you for explanation. Maybe I understood the summary of implementation of HP firmware. But how to use or where to put _INI method in the ACPI namespace never becomes reasonable reason why your driver may ignore _STA before evaluating _SUN.

Quoted 2 lines

But in any case, I think both HP and Fujitsu firmware are doing legal things -- neither firmware is breaking the spec.

My understanding of your explanation so far is:

- evaluating _SUN without checking _STA doesn't cause problem, from the view point of HP's implementation.
- some IBM machine is doing same as HP

Quoted 2 lines

I never think those are reasonable reasons why ignoring _STA before evaluating _SUN is legal. Am I missing something?

I think the piece that I did not explain clearly is that the spec does not require checking _STA immediately before _SUN. The spec says:

- you must check _STA before calling _INI

- _INI must be called to initialize _SUN

- _INI is called once, when the table is loaded

On HP's implementation, we do obey those rules. We call _INI on the PCI bridge during boot, which then initializes the children SxFy objects. From that point on, it is legal for us to call _SUN.

The other issue is that the spec does not specify the *semantics* of _STA. P/IBM firmware engineers think _STA should indicate card presence,

but Fujitsu firmware engineers think _STA should
indicate slot presence.

I don't think either firmware team is incorrect -- it is simply the case that the specification was not precise enough, to the point where separate teams following the same spec came up with implementations with different behaviors and different semantics.

I believe that we both have compliant, legal firmware.

Quoted 6 lines

Quoted 3 lines

If one list is shorter than the other, then that should be the list to put in the kernel, and the default behavior should be "majority rule".

I don't want to consider "majority rule" before I understand why ignoring _STA is legal.

On HP and IBM machines, it *is* legal because we *did* call _STA, then _INI, then _SUN. That is one interpretation of the spec.

On Fujitsu machines, the semantics of _STA are different, and it is not legal to ignore _STA. That is another interpretation of the spec.

Hi Alex/Kenji-san,

Time for my 2 cents.

Quoted 24 lines · Alex Chiang

Hi Kenji-san,

* Kenji Kaneshige <[email protected]>:

Quoted 21 lines

Hi Alex-san,

Quoted 6 lines

On my machine, it is legal to evaluate S1F0._SUN independent of S1F0._STA because L001._INI has already been evaluated. It would be helpful to know what Fujitsu's namespace looks like. If Fujitsu slot objects contain _STA and _INI, then I agree with Kenji-san -- I definitely need to check _STA before evaluating _SUN.

Thank you for explanation. Maybe I understood the summary of implementation of HP firmware. But how to use or where to put _INI method in the ACPI namespace never becomes reasonable reason why your driver may ignore _STA before evaluating _SUN.

Quoted 2 lines

But in any case, I think both HP and Fujitsu firmware are doing legal things -- neither firmware is breaking the spec.

My understanding of your explanation so far is:

- evaluating _SUN without checking _STA doesn't cause problem, from the view point of HP's implementation.
- some IBM machine is doing same as HP

Quoted 7 lines

Quoted 2 lines

I never think those are reasonable reasons why ignoring _STA before evaluating _SUN is legal. Am I missing something?

I think the piece that I did not explain clearly is that the spec does not require checking _STA immediately before _SUN. The spec says:

- you must check _STA before calling _INI

Agreed.

Quoted 1 line

- _INI must be called to initialize _SUN

Based on my review of ACPI spec 3.0b I would add:
1. For a specific device that has only _STA and _SUN methods, _SUN can be run and it's results can be trusted irrespective of the _STA return value.
2. For a specific device that has _STA, _INI, and _SUN methods, _SUN can be run and it's results can be trusted even if _INI is not run because the device is absent. If the device is present and _INI is run then _SUN cannot be run until after _INI is run.

See ACPI spec 3.0b "6.1.8 _SUN (Slot User Number)" which says nothing about a required presence of the device.

Quoted 1 line

- _INI is called once, when the table is loaded

Agreed.

Quoted 8 lines

On HP's implementation, we do obey those rules. We call _INI on the PCI bridge during boot, which then initializes the children SxFy objects. From that point on, it is legal for us to call _SUN.

The other issue is that the spec does not specify the *semantics* of _STA. P/IBM firmware engineers think _STA should indicate card presence,

I checked this on the IBM x3850 and it appears to be true. I instrumented register_slot() in drivers/pci/hotplug/acpiphp_glue.c to print _STA and _SUN returns and got the following results with
slot 1 (PCI-X) populated, slot 2 (PCI-X) vacant, slot 3 (PCIe) vacant,
slot 4 (PCIe) populated, slot 5 (PCIe) vacant, and slot 6 (PCIe)
populated. When a card is present _STA returns non-zero status
for all functions, otherwise it returns zero. None of the SxFy
devices have an _INI method.

acpiphp: ACPI Hot Plug PCI Controller Driver version: 0.5
acpiphp_glue: found PCI host-bus bridge with hot-pluggable slots
acpiphp_glue: register_slot: \_SB_.VP02.S1F0 sta=0 sun=1
acpiphp_glue: found ACPI PCI Hotplug slot 1 at PCI 0000:02:01
acpiphp: Slot [1] registered
acpiphp_glue: register_slot: \_SB_.VP02.S1F1 sta=0 sun=1
acpiphp_glue: register_slot: \_SB_.VP02.S1F2 sta=0 sun=1
acpiphp_glue: register_slot: \_SB_.VP02.S1F3 sta=0 sun=1
acpiphp_glue: register_slot: \_SB_.VP02.S1F4 sta=0 sun=1
acpiphp_glue: register_slot: \_SB_.VP02.S1F5 sta=0 sun=1
acpiphp_glue: register_slot: \_SB_.VP02.S1F6 sta=0 sun=1
acpiphp_glue: register_slot: \_SB_.VP02.S1F7 sta=0 sun=1
acpiphp_glue: found PCI host-bus bridge with hot-pluggable slots
acpiphp_glue: register_slot: \_SB_.VP03.S2F0 sta=f sun=2
acpiphp_glue: found ACPI PCI Hotplug slot 2 at PCI 0000:06:01
acpiphp: Slot [2] registered
acpiphp_glue: register_slot: \_SB_.VP03.S2F1 sta=f sun=2
acpiphp_glue: register_slot: \_SB_.VP03.S2F2 sta=f sun=2
acpiphp_glue: register_slot: \_SB_.VP03.S2F3 sta=f sun=2
acpiphp_glue: register_slot: \_SB_.VP03.S2F4 sta=f sun=2
acpiphp_glue: register_slot: \_SB_.VP03.S2F5 sta=f sun=2
acpiphp_glue: register_slot: \_SB_.VP03.S2F6 sta=f sun=2
acpiphp_glue: register_slot: \_SB_.VP03.S2F7 sta=f sun=2
acpiphp_glue: found PCI-to-PCI bridge at PCI 0000:0a:00.0
acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F0 sta=0 sun=3
acpiphp_glue: found ACPI PCI Hotplug slot 3 at PCI 0000:0b:00
acpiphp: Slot [3] registered
acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F1 sta=0 sun=3
acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F2 sta=0 sun=3
acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F3 sta=0 sun=3
acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F4 sta=0 sun=3
acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F5 sta=0 sun=3
acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F6 sta=0 sun=3
acpiphp_glue: register_slot: \_SB_.VP04.CALG.E3F7 sta=0 sun=3
acpiphp_glue: found PCI-to-PCI bridge at PCI 0000:0f:00.0
acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F0 sta=f sun=4
acpiphp_glue: found ACPI PCI Hotplug slot 4 at PCI 0000:10:00
acpiphp: Slot [4] registered
acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F1 sta=f sun=4
acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F2 sta=f sun=4
acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F3 sta=f sun=4
acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F4 sta=f sun=4
acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F5 sta=f sun=4
acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F6 sta=f sun=4
acpiphp_glue: register_slot: \_SB_.VP05.CALG.E4F7 sta=f sun=4
acpiphp_glue: found PCI-to-PCI bridge at PCI 0000:14:00.0
acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F0 sta=0 sun=5
acpiphp_glue: found ACPI PCI Hotplug slot 5 at PCI 0000:15:00
acpiphp: Slot [5] registered
acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F1 sta=0 sun=5
acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F2 sta=0 sun=5
acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F3 sta=0 sun=5
acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F4 sta=0 sun=5
acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F5 sta=0 sun=5
acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F6 sta=0 sun=5
acpiphp_glue: register_slot: \_SB_.VP06.CALG.E5F7 sta=0 sun=5
acpiphp_glue: found PCI-to-PCI bridge at PCI 0000:19:00.0
acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F0 sta=f sun=6
acpiphp_glue: found ACPI PCI Hotplug slot 6 at PCI 0000:1a:00
acpiphp: Slot [6] registered
acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F1 sta=f sun=6
acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F2 sta=f sun=6
acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F3 sta=f sun=6
acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F4 sta=f sun=6
acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F5 sta=f sun=6
acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F6 sta=f sun=6
acpiphp_glue: register_slot: \_SB_.VP07.CALG.E6F7 sta=f sun=6
acpiphp_glue: Bus 0000:1a has 1 slot
acpiphp_glue: Bus 0000:15 has 1 slot
acpiphp_glue: Bus 0000:10 has 1 slot
acpiphp_glue: Bus 0000:0b has 1 slot
acpiphp_glue: Bus 0000:06 has 1 slot
acpiphp_glue: Bus 0000:02 has 1 slot
acpiphp_glue: Total 6 slots

Quoted 13 lines

but Fujitsu firmware engineers think _STA should
indicate slot presence.

I don't think either firmware team is incorrect -- it is simply the case that the specification was not precise enough, to the point where separate teams following the same spec came up with implementations with different behaviors and different semantics.

I believe that we both have compliant, legal firmware.

Quoted 3 lines

Quoted 3 lines

If one list is shorter than the other, then that should be the list to put in the kernel, and the default behavior should be "majority rule".

Quoted 8 lines

Quoted 2 lines

I don't want to consider "majority rule" before I understand why ignoring _STA is legal.

On HP and IBM machines, it *is* legal because we *did* call _STA, then _INI, then _SUN. That is one interpretation of the spec.

On Fujitsu machines, the semantics of _STA are different, and it is not legal to ignore _STA. That is another interpretation of the spec.

I am not convinced that this a correct interpretation. I believe that the ACPI spec indicates that it is legal to call _SUN and trust it's results no matter what _STA returns. I believe the only constraint on running _SUN is that it be run after _INI is run if _INI exists for the same device and _STA for that device indicates that it should be run.

Gary

Ok, I understood what you mean and I agreed both implementations are legal. Also I explained it to our firmware team, and they understood your situation.

I don't know which implementation is more popular. But according to the current score (2 : 1), you win the "majority rule".

Alex-san, could you send me the updated patches? I'll Ack the patch after adding Fujitsu firmware versions to the DMI list and testing it.

My interpretation of this is that if Alex's driver scans the Fujitsu machine and calls _STA on one of the slots and gets Bit 0 (present) and Bit 3 (functioning) set, then it is ok to enumerate it's children and evaluate _ADR and _SUN for the children. If they are returning 1023, that would mean you should not evaluate the children of that object since the functional bit is not set. Is this correct?

If that is the case, I would say that Alex's driver needs to obey the spec - or else if the this is allowed in an earlier version of the spec then he needs to check for the version of the spec that was implemented and make some different rules for that.

Quoted 24 lines · On Tue, 11 Mar 2008 22:10:40 +0900 Kenji Kaneshige…

Hi Alex-san,

Quoted 21 lines · Alex Chiang

Quoted 10 lines

Quoted 10 lines

It wasn't the IBM machine that was breaking; it was Fujitsu. They were returning an error code (the numerical value 1023) when I called the _SUN method on a slot object that existed in the ACPI namespace but was not present (as reported by the _STA method). By the time I got that error report, I'd already dropped the duplicate name detection code, and was letting the kobject infrastructure warn about duplicate names because for my test cases, refcounting was a better solution.
[Kenji-san from Fujitsu seemed to be ok with the progress I'd made at the time, he can speak up if he's changed his mind ;)]

Quoted 8 lines

Unfortunatelly, I have not tried the new version of slot detection
driver because of the lack of test environment. Maybe we need more
several days to wait for test environment. BTW, does the new one fixes the issue I reported before? I could not find it in the changelog. IIRC, this issue was difficult to
solve because the root cause of this issue is from the difference
of interpretation of ACPI spec between HP and Fujitsu (I still
don't think it's a good idea to evaluate _SUN for the device
object whose _STA is 0).

It looks like we disagree on how to interpret the spec (IBM machines interpret the spec the same way HP machines do).

OK, let me see if I can understand what the issue is here. Please correct me if I'm wrong. The debate is about whether or not it is legitimate to call _SUN if _STA indicates that the device is not present and functional. I've checked ACPI 3.0b, and from what I've read, you may not evaluate _SUN until _INI is called. And _INI should not be called unless _STA indicates that a device is present and functional.

"OSPM evaluates the _STA object before it evaluates a device _INI method. The return values of the Present and Functioning bits determines whether _INI should be evaluated and whether children of the device should be enumerated and initialized. See section 6.5.1, “_INI (Init)”."

My interpretation of this is that if Alex's driver scans the Fujitsu machine and calls _STA on one of the slots and gets Bit 0 (present) and Bit 3 (functioning) set, then it is ok to enumerate it's children and evaluate _ADR and _SUN for the children. If they are returning 1023, that would mean you should not evaluate the children of that object since the functional bit is not set. Is this correct?

If that is the case, I would say that Alex's driver needs to obey the spec - or else if the this is allowed in an earlier version of the spec then he needs to check for the version of the spec that was implemented and make some different rules for that.

Thanks for the clarification,
Kristen

Hi Alex-san,

I tried your patches, and I have two comments. I want 1) to be fixed before merge to Greg's tree (or linux-next?), at least.

1) I checked ACPI spec again and again, but I could not find any reason to add Fujitsu servers to quirks list. So I'd like you to add HP servers to the quirks list. I'll send the following patches followed by this e-mail.

2) The ACPI PCI slot detection driver would change the slot names of some hotplug drivers (at least I checked shpchp and pciehp). And the name of slots are depending on the order of driver loading. For example, on my system which has several SHPCHP slots and PCIEHP slots, the name of PCIEHP slots are changed as follows. Please note that PCIEHP based slots are 0034_0027 and 0032_0026, and others are SHPCHP based slots.

- Without ACPI PCI slot detection driver
# ls /sys/bus/pci/slots/
0009_0016 0014_0018 0019_0020 0021_0022 0034_0027 0011_0017 0016_0019 0021_0021 0032_0026

- With ACPI PCI slot detection driver
# ls /sys/bus/pci/slots/
0009_0016 0014_0018 0019_0020 0021_0022 27 0011_0017 0016_0019 0021_0021 26

I had thought it is not a big problem before because people who don't like new names would not load the PCI slot driver. But since it is loaded automatically at boot time, I'm wondering that it would be a problem. For example, some platform, not fujitsu, depends on the old slot name to work, IIRC (Maybe Kristen knows the background about it). And I don't think the fact that slot names are changed depending on the order of driver loading is acceptable by system management people/software, though I don't have such software.

Though I don't have any specific idea about this, folliwings might be candidates.

- Override slot names with hotplug driver's slot names

- Unify slot names among all hotplug drivers

- Stop automatic loading of ACPI PCI slot driver

Quoted 4 lines · On Tue, 11 Mar 2008 22:10:40 +0900 Kenji Kaneshige…

Hi Alex-san,

I tried your patches, and I have two comments. I want 1) to be fixed before merge to Greg's tree (or linux-next?), at least.

yes, we will not merge these patches without your ack.

Quoted 4 lines

1) I checked ACPI spec again and again, but I could not find any reason to add Fujitsu servers to quirks list. So I'd like you to add HP servers to the quirks list. I'll send the following patches followed by this e-mail.

I have followed up with a question on this on another thread, so I'll skip to #2...
<snip>

Quoted 28 lines

2) The ACPI PCI slot detection driver would change the slot names of some hotplug drivers (at least I checked shpchp and pciehp). And the name of slots are depending on the order of driver loading. For example, on my system which has several SHPCHP slots and PCIEHP slots, the name of PCIEHP slots are changed as follows. Please note that PCIEHP based slots are 0034_0027 and 0032_0026, and others are SHPCHP based slots.

- Without ACPI PCI slot detection driver
# ls /sys/bus/pci/slots/
0009_0016 0014_0018 0019_0020 0021_0022 0034_0027 0011_0017 0016_0019 0021_0021 0032_0026

- With ACPI PCI slot detection driver
# ls /sys/bus/pci/slots/
0009_0016 0014_0018 0019_0020 0021_0022 27 0011_0017 0016_0019 0021_0021 26

I had thought it is not a big problem before because people who don't like new names would not load the PCI slot driver. But since it is loaded automatically at boot time, I'm wondering that it would be a problem. For example, some platform, not fujitsu, depends on the old slot name to work, IIRC (Maybe Kristen knows the background about it). And I don't think the fact that slot names are changed depending on the order of driver loading is acceptable by system management people/software, though I don't have such software.

Quoted 4 lines

Though I don't have any specific idea about this, folliwings might be candidates.

- Override slot names with hotplug driver's slot names

I think this should be done - if the pci slot driver detects that a hotplug driver is controlling a slot, it should allow that driver to set the name of the slot.

Quoted 1 line

- Unify slot names among all hotplug drivers

I'm not sure if we can do this, since slot name might depend on what spec people are implementing.

Quoted 1 line

- Stop automatic loading of ACPI PCI slot driver

I think we should definitely implement this one ^^^.

Quoted 2 lines · Greg KH

Quoted 1 line

What is the guarantee that the names of these slots are correct

Quoted 1 line

Quoted 1 line

and do not happen to be the same as the hotpluggable ones?

Quoted 2 lines

That would be a bug -- and yes, bugs happen, and we have to deal with them.

My main concern is that BIOS vendors will not fix these bugs, as no other OS cares/does this kind of thing today. The ammount of bad information out there might be quite large, and I think this was confirmed by some initial testing of IBM systems, right?

Quoted 4 lines

Quoted 3 lines

Why show this information on machines that can not do anything with these slots at all? Will that not just confuse people?

Quoted 4 lines

Only for people who think that /sys/bus/pci/slots/ is for hotpluggable slots only. There is plenty of useful information available for slots that aren't hotpluggable (eg bus address, speed, width, error status).

Can the userspace tools that are using the existing directories thinking that only hotplug slots are there, handle "non-hotplug" slots showing up in this location?

* Greg KH <[email protected]>:

Quoted 3 lines · Matthew Wilcox

Quoted 2 lines · Greg KH

Quoted 1 line

What is the guarantee that the names of these slots are correct

No guarantee there. We report whatever firmware tells us.

Quoted 1 line

Quoted 1 line

Quoted 1 line

and do not happen to be the same as the hotpluggable ones?

Stronger guarantee here, since both pci_slot and <foo>hp driver will be getting the name of the slot from the same place.

Quoted 7 lines

Quoted 2 lines

That would be a bug -- and yes, bugs happen, and we have to deal with them.

My main concern is that BIOS vendors will not fix these bugs, as no other OS cares/does this kind of thing today. The ammount of bad information out there might be quite large, and I think this was confirmed by some initial testing of IBM systems, right?

We saw problems on Fujitsu machines, where they return an error code when the _SUN method is called on a slot that exists in the namespace but isn't actually present.

After discussing with Kenji-san about specs, we came to the agreement that he was ok with this behavior because he had the option to not load pci_slot on his machines.

I agree that there might be lots of buggy firmwares out there, but we won't know for certain until we get some exposure. And I think the upside is worth it.

Kristen suggested the linux-next tree. That sounds viable to me...

Quoted 9 lines

Quoted 8 lines

Quoted 3 lines

Why show this information on machines that can not do anything with these slots at all? Will that not just confuse people?

Only for people who think that /sys/bus/pci/slots/ is for hotpluggable slots only. There is plenty of useful information available for slots that aren't hotpluggable (eg bus address, speed, width, error status).

Quoted 3 lines

Can the userspace tools that are using the existing directories thinking that only hotplug slots are there, handle "non-hotplug" slots showing up in this location?

Of course we shouldn't break userspace, no one wants that. But nothing about that name (/sys/bus/pci/slots/) implies "hotplug only", and we have no idea how big the problem might be.

Again, I'm thinking more exposure in linux-next might be a reasonable way for us to figure out how bad (or good) the situation might really be out there.

Quoted 4 lines

Again, I'm thinking more exposure in linux-next might be a reasonable way for us to figure out how bad (or good) the situation might really be out there.

Ok, care to resend them with the requested updates?

Hi Gary, John, Kenji-san, et. al,

Well, first Gary was on holiday for a month, and then I was on holiday for a month, but I'm back now, and have refreshed this patch series against 2.6.25.

The major thing that happened was all the kobject changes (learned my lesson about taking long holidays when holding onto a largish chunk of code that hasn't been accepted yet ;), and so the only real change is in patch 3/4.

The kobject changes were nice, btw. In the prior versions of this series, I could never figure out why my kobjects weren't getting released when their refcounts went to 1, and had some hacky code in there to manually release them. (I'm sure I was doing something wrong, but I couldn't figure out what.) I was able to remove that hack in this series because the kobjects are working the way they're supposed to.

I did turn on kobject debugging, and all seems well except for one little thing. I based my module (pci_slot) on acpiphp, and the kobject system complains:

kobject: 'acpiphp' (a00000020476aed0): does not have a release() function, it is broken and must be fixed.

kobject: 'pci_slot' (a000000204791e50): does not have a release() function, it is broken and must be fixed.

Not quite sure what to do about these yet, but since no one has fixed acpiphp yet, I'm thinking that I can't be *too* wrong. :)

Quoted 11 lines

I did turn on kobject debugging, and all seems well except for one little thing. I based my module (pci_slot) on acpiphp, and the kobject system complains:

kobject: 'acpiphp' (a00000020476aed0): does not have a release() function, it is broken and must be fixed.

kobject: 'pci_slot' (a000000204791e50): does not have a release() function, it is broken and must be fixed.

Quoted 2 lines

Not quite sure what to do about these yet, but since no one has fixed acpiphp yet, I'm thinking that I can't be *too* wrong. :)

Um, the obvious solution of providing a release function for these kobjects is somehow not correct?

Please do that, otherwise the code is wrong (and yes, acpiphp might be wrong as well, I haven't seen that report yet.)

* Greg KH <[email protected]>:

Quoted 18 lines · Alex Chiang

Quoted 5 lines

I did turn on kobject debugging, and all seems well except for one little thing. I based my module (pci_slot) on acpiphp, and the kobject system complains:

Quoted 8 lines

kobject: 'acpiphp' (a00000020476aed0): does not have a release() function, it is broken and must be fixed.

kobject: 'pci_slot' (a000000204791e50): does not have a release() function, it is broken and must be fixed.

Not quite sure what to do about these yet, but since no one has fixed acpiphp yet, I'm thinking that I can't be *too* wrong. :)

Um, the obvious solution of providing a release function for these kobjects is somehow not correct?

Quoted 2 lines

Please do that, otherwise the code is wrong (and yes, acpiphp might be wrong as well, I haven't seen that report yet.)

Hrm, maybe this is a false alarm? Those messages appear when doing an rmmod <foo>. Turns out you get those messages for lots of modules, for instance, uhci_ucd:

kobject: 'uhci_hcd' (a00000020427c8d0): kobject_cleanup
kobject: 'uhci_hcd' (a00000020427c8d0): does not have a release() function, it is broken and must be fixed.
kobject: 'uhci_hcd' (a00000020427c8d0): auto cleanup 'remove' event
kobject: 'uhci_hcd' (a00000020427c8d0): kobject_uevent_env
kobject: 'uhci_hcd' (a00000020427c8d0): fill_kobj_path: path = '/module/uhci_hcd'
kobject: 'uhci_hcd' (a00000020427c8d0): auto cleanup kobject_del
kobject: 'uhci_hcd': free name

Are you saying that modules that call module_init/module_exit are supposed to supply a release() function?