Hi Kenji-san,
* Kenji Kaneshige <kaneshige.kenji@xxxxxxxxxxxxxx>:
> > Hi Gary, Kenji-san, et. al,
> >
> > * Gary Hade <garyhade@xxxxxxxxxx>:
> >> 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;
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
