lkml.org 
[lkml]   [2007]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 0/4, v3] Physical PCI slot objects
    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;
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/
    \
     
     \ /
      Last update: 2007-11-30 02:53    [W:4.299 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site