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

[Alex Chiang <achiang@hp.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.

It can be modprobe'd/rmmod'ed in any combination, and in any
order with other PCI hotplug modules. There is no ordering
dependency, even at module unload time, so you can safely rmmod
pci_slot, and safely continue using features provided by the PCI
hotplug drivers (acpiphp, pciehp, etc.). The opposite works too.

The one limitation is that two separate hotplug drivers cannot
both claim the same device (2nd module loaded will get -EBUSY
errors), but I do not believe that is a regression from current
behavior.

I have only tested with acpiphp and pciehp, as that's the only
hardware I have, but I believe my code will play nicely with the
other PCI hp drivers as well.

The patch series is fully bisectable, and the correct behavior
occurs no matter which patch you happen to have applied.

I'll be sending v5 of patches 3 and 4 shortly (patches 1 and 2
did not change). It is still based on 2.6.24-rc2, because I was
too scared to do another git rebase while using stgit. :-/

> Also, I notice that even with your current CONFIG_ACPI_PCI_SLOT
> implementation your numerous PCI hotplug driver changes (except
> for only two places in pci_hotplug_core.c where there is 
> `#ifndef CONFIG_ACPI_PCI_SLOT` and `#ifdef CONFIG_ACPI_PCI_SLOT`)
> are _always_ exposed.  So, even with CONFIG_ACPI_PCI_SLOT disabled
> there is IMO a need for testing of the affected PCI hotplug drivers
> on more than a small number of isolated systems.

You are, of course, correct.

In my opinion, though, I would say most of the changes to the PCI
hotplug drivers themselves are pretty straightforward, as in
removing the different ways of getting the PCI address.

The scary part of the changes (aside from the ACPI-PCI slot
driver) revolve around the new struct pci_slot, which is
relatively self-contained, and only expose themselves via the
pci_create_slot/pci_destroy_slot interfaces which only the PCI
hotplug corecares about.

Regardless, your point stands. How do you suggest I get more
testing time? Is this patchset appropriate for the -mm tree yet?
Or do you think it still needs more work?

> The good news is that I was able to test your v3 changes
> (w/2.6.24-rc3 source) on our x3850 today with 'acpiphp' and,
> except for the above mentioned inability to run-time
> include/exclude them, they seemed to work fine.  The previous
> boot-time ACPI error messages are gone and I was able to
> successfully hot-remove and hot-add both PCI-X and PCIe
> adapters.

Thanks for testing. Please let me know how v5 works for you too.

chers,

/ac