[+cc Myron]
On Wed, Jan 9, 2013 at 1:19 PM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> On Thursday, January 10, 2013 12:58:25 AM Jiang Liu wrote:
>> Hi Rafael,
>> Thanks for your great efforts to review the patch.
>>
>> On 01/09/2013 08:01 AM, Rafael J. Wysocki wrote:
>> > Hi,
>> >
>> > On Wednesday, January 09, 2013 12:52:22 AM Jiang Liu wrote:
>> snip
>> >>
>> >> +static void acpi_pci_slot_notify_add(struct pci_dev *dev)
>> >> +{
>> >> + acpi_handle handle;
>> >> + struct callback_args context;
>> >> +
>> >> + if (!dev->subordinate)
>> >> + return;
>> >> +
>> >> + mutex_lock(&slot_list_lock);
>> >> + handle = DEVICE_ACPI_HANDLE(&dev->dev);
>> >> + context.root_handle = acpi_find_root_bridge_handle(dev);
>> >
>> > There's a patch under discussion that removes this function.
>> >
>> > Isn't there any other way to do this?
>> I will try to find a way to get rid of calling acpi_find_root_bridge_handle,
>> and it seems doable.
>>
>> >
>> >> + if (handle && context.root_handle) {
>> >> + context.pci_bus = dev->subordinate;
>> >> + context.user_function = register_slot;
>> >> + acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
>> >
>> > You can just pass 1 here I think. Does the compiler complain?
>> Thanks for reminder, the (u32) is unnecessary.
>>
>> >
>> >> + register_slot, NULL, &context, NULL);
>> >> + }
>> >> + mutex_unlock(&slot_list_lock);
>> >> +}
>> >> +
>> >> +static void acpi_pci_slot_notify_del(struct pci_dev *dev)
>> >> +{
>> >> + struct acpi_pci_slot *slot, *tmp;
>> >> + struct pci_bus *bus = dev->subordinate;
>> >> +
>> >> + if (!bus)
>> >> + return;
>> >> +
>> >> + mutex_lock(&slot_list_lock);
>> >> + list_for_each_entry_safe(slot, tmp, &slot_list, list)
>> >> + if (slot->pci_slot && slot->pci_slot->bus == bus) {
>> >> + list_del(&slot->list);
>> >> + pci_destroy_slot(slot->pci_slot);
>> >> + put_device(&bus->dev);
>> >> + kfree(slot);
>> >> + }
>> >> + mutex_unlock(&slot_list_lock);
>> >> +}
>> >> +
>> >> +static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
>> >> + unsigned long event, void *data)
>> >> +{
>> >> + struct device *dev = data;
>> >> +
>> >> + switch (event) {
>> >> + case BUS_NOTIFY_ADD_DEVICE:
>> >> + acpi_pci_slot_notify_add(to_pci_dev(dev));
>> >> + break;
>> >
>> > Do I think correctly that this is going to be called for every PCI device
>> > added to the system, even if it's not a bridge?
>> You are right. Function acpi_pci_slot_notify_add() and acpi_pci_slot_notify_del()
>> will check whether it's a bridge. If preferred, I will move the check up into
>> acpi_pci_slot_notify_fn().
>>
>> >
>> >> + case BUS_NOTIFY_DEL_DEVICE:
>> >> + acpi_pci_slot_notify_del(to_pci_dev(dev));
>> >> + break;
>> >> + default:
>> >> + return NOTIFY_DONE;
>> >> + }
>> >> +
>> >> + return NOTIFY_OK;
>> >> +}
>> >> +
>> >> +static struct notifier_block acpi_pci_slot_notifier = {
>> >> + .notifier_call = &acpi_pci_slot_notify_fn,
>> >> +};
>> >> +
>> >> static int __init
>> >> acpi_pci_slot_init(void)
>> >> {
>> >> dmi_check_system(acpi_pci_slot_dmi_table);
>> >> acpi_pci_register_driver(&acpi_pci_slot_driver);
>> >> + bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
>> >
>> > I wonder if/why this has to be so convoluted?
>> >
>> > We have found a PCI bridge in the ACPI namespace, so we've created a struct
>> > acpi_device for it and we've walked the namespace below it already.
>> >
>> > Now we're creating a struct pci_dev for it and while registering it we're
>> > going to walk the namespace below the bridge again to find and register its
>> > slots and that is done indirectly from a bus type notifier.
>> >
>> > Why can't we enumerate the slots directly upfront?
>> Do you mean to create the PCI slot devices when creating the ACPI devices?
>> I think there are two factors prevent us from doing that.
>> The first is that the ACPI pci_slot driver could be built as a module, so
>> we can't call into it from the ACPI core.
>
> I didn't say about calling the pci_slot driver from the ACPI core, but about
> enumerating slots in a way suitable for consumption by the pci_slot driver
> when it's ready.
>
> That said I really don't see a value in having a modular pci_slot driver. It
> is part of the hotplug infrastructure and should always be presend for this
> reason, so we don't need to worry about the "pci_slot driver not present" case.
I agree that there's no value in supporting CONFIG_ACPI_PCI_SLOT=m. I
think Myron has some patches that remove that case.
I'm not sure what the best way to merge them is. We have a bunch of
stuff this cycle that touches both ACPI and PCI.
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
