Re: [PATCH 0/4, v3] Physical PCI slot objects

[Alex Chiang <achiang@hp.com>]

Hi Kenji-san,

* Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>:
> > Hi Gary, Kenji-san, et. al,
> > 
> > * Gary Hade <garyhade@us.ibm.com>:
> >> Alex, What I was trying to suggest is a boot-time kernel
> >> option, not a kernel configuration option.  The basic idea is
> >> to give the user (with a single binary kernel) the ability to
> >> include your ACPI-PCI slot driver feature changes only when
> >> they are really needed.  In addition to reducing the number of
> >> system/PCI hotplug driver combinations where your changes would
> >> need to be validated, I believe would also help alleviate other
> >> worries (e.g. Andi Kleen's memory consumption concern).  I
> >> believe this goal could also be achieved with the kernel config
> >> option by making the pci_slot module runtime loadable with the
> >> PCI hotplug drivers only visiting your new code when the
> >> pci_slot driver is loaded, although I think this would be more
> >> difficult to implement.
> > 
> > I have modified my patch series so that the final patch that
> > introduces my ACPI-PCI slot driver is a full-fledged module, that
> > has a tristate Kconfig option.
> > 
> 
> Thank you for your good job.

Thanks for testing. :)

> I tested shpchp and pciehp both with and without pci_slot
> module. There seems no regression from shpchp and pciehp's
> point of view.  (I had a little concern about the hotplug
> slots' name that vary depending on whether pci_slot
> functionality is enabled or disabled. But, now that we can
> build pci_slot driver as a kernel module, I don't think it is a
> big problem).

Hm, you are right. On my machine, if I load pciehp first and
acpiphp second (even without loading pci_slot), I will see the
following:

[root@canola slots]# ls
0016_0006  0197_0005  10  3  4  7  8  9

[root@canola slots]# lsmod | grep pci_slot
[root@canola slots]# lsmod | grep hp
acpiphp               115984  0 
pciehp                140616  0 
pci_hotplug           123972  2 acpiphp,pciehp

On the other hand, if I do load pci_slot first, and then pciehp,
you are right, I will see something like this:

[root@canola slots]# ls
1  10  2  3  4  5  6  7  8  9

[root@canola slots]# lsmod | grep pci_slot
pci_slot               74436  0 
[root@canola slots]# lsmod | grep hp
pciehp                140616  0 
pci_hotplug           123972  1 pciehp

But I do agree, people don't need to load pci_slot at all if they
don't want it, and they won't be bothered.

> Only the problems is that I got Call Traces with the following
> error messages when pci_slot driver was loaded, and one strange
> slot named '1023' was registered (other slots are fine). This
> is the same problem I reported before.
> 
>     sysfs: duplicate filename '1023' can not be created
>     WARNING: at fs/sysfs/dir.c:424 sysfs_add_one()
> 
>     kobject_add failed for 1023 with -EEXIST, don't try to
>     register things with the same name in the same directory.
> 
> On my system, hotplug slots themselves can be added, removed
> and replaced with the ohter type of I/O box. The ACPI firmware
> tells OS the presence of those slots using _STA method (That
> is, it doesn't use 'LoadTable()' AML operator). On the other
> hand, current pci_slot driver doesn't check _STA.  As a result,
> pci_slot driver tryied to register the invalid (non-existing)
> slots. The ACPI firmware of my system returns '1023' if the
> invalid slot's _SUN is evaluated. This is the cause of Call
> Traces mentioned above. To fix this problem, pci_slot driver
> need to check _STA when scanning ACPI Namespace.

Now this is very curious. The relevant line in pci_slot is:

check_slot()
	status = acpi_evaluate_integer(handle, "_SUN", NULL, sun);
	if (ACPI_FAILURE(status))
		return -1;

Why does your firmware return the error information inside sun,
instead of returning an error in status? That doesn't seem right
to me...

> I'm sorry for reporting this so late. I'm attaching the patch
> to fix the problem. This is against 2.6.24-rc3 with your
> patches applied. Could you try it?

Applying this patch causes me to only detect populated slots in
my system, which isn't what I want -- otherwise, I could have
just enumerated the PCI bus and found the devices that way. :)

Maybe on your machine, checking existence of _STA might do the
right thing, but I don't think we should actually be looking at
any of the actual bits returned. 

If we check ACPI_STA_DEVICE_PRESENT, then we will not detect
empty slots on my system. Can you try this patch to see if at
least the first call to acpi_evaluate_integer helps? If that
doesn't help, maybe the second block will help you, but it breaks
my machine...

Thanks.

/ac


diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
index 724f4f0..63a4dc8 100644
--- a/drivers/acpi/pci_slot.c
+++ b/drivers/acpi/pci_slot.c
@@ -55,9 +65,21 @@ static struct acpi_pci_driver acpi_pci_slot_driver = {
 static int
 check_slot(acpi_handle handle, int *device, unsigned long *sun)
 {
-       unsigned long adr;
+       unsigned long adr, sta;
        acpi_status status;
 
+       /* Doesn't seem to hurt anything on hp systems */
+       status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
+       if (ACPI_FAILURE(status))
+               return -1;
+
+       /* This code causes us to fail to detect empty slots, so
+        * commented out for now.
+        *
+       if (!(sta & ACPI_STA_DEVICE_PRESENT))
+               return -1;
+        */
+
        status = acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
        if (ACPI_FAILURE(status))
                return -1;