Re: [PATCH v8 08/22] PCI, acpiphp: Separate out hot-add support of pci host bridge

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Jan 12, 2013 at 3:18 PM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
>> @@ -673,3 +673,223 @@ int __init acpi_pci_root_init(void)
>>
>>       return 0;
>>  }
>> +
>> +/*
>> + * Separated from drivers/pci/hotplug/acpiphp_glue.c
>> + *   only support root bridge
>> + */
>
> This comment will be useless after applying the patch.
>
>> +
>> +static LIST_HEAD(acpi_root_bridge_list);
>> +struct acpi_root_bridge {
>> +     struct list_head list;
>> +     acpi_handle handle;
>> +     u32 flags;
>> +};
>
> We have struct acpi_pci_root already.  Why do we need this in addition?

will address that in following patch.

>
> Also, we have acpi_pci_roots, so why do we need another list of root bridges?
>
>> +
>> +/* bridge flags */
>> +#define ROOT_BRIDGE_HAS_EJ0  (0x00000002)
>> +#define ROOT_BRIDGE_HAS_PS3  (0x00000080)
>
> What is that needed for?

will address that in following patch.

>
>> +
>> +#define ACPI_STA_FUNCTIONING (0x00000008)
>> +
>> +static struct acpi_root_bridge *acpi_root_handle_to_bridge(acpi_handle handle)
>> +{
>> +     struct acpi_root_bridge *bridge;
>> +
>> +     list_for_each_entry(bridge, &acpi_root_bridge_list, list)
>> +             if (bridge->handle == handle)
>> +                     return bridge;
>> +
>> +     return NULL;
>> +}
>> +
>> +/* allocate and initialize host bridge data structure */
>> +static void add_acpi_root_bridge(acpi_handle handle)
>> +{
>> +     struct acpi_root_bridge *bridge;
>> +     acpi_handle dummy_handle;
>> +     acpi_status status;
>> +
>
> Why do we need to evaluate all of the methods directly here?
>
> Don't we have a struct acpi_device for handle already?
>
>> +     /* if the bridge doesn't have _STA, we assume it is always there */
>> +     status = acpi_get_handle(handle, "_STA", &dummy_handle);
>> +     if (ACPI_SUCCESS(status)) {
>> +             unsigned long long tmp;
>> +
>> +             status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp);
>> +             if (ACPI_FAILURE(status)) {
>> +                     printk(KERN_DEBUG "%s: _STA evaluation failure\n",
>> +                                              __func__);
>> +                     return;
>> +             }
>> +             if ((tmp & ACPI_STA_FUNCTIONING) == 0)
>> +                     /* don't register this object */
>> +                     return;
>> +     }
>> +
>> +     bridge = kzalloc(sizeof(struct acpi_root_bridge), GFP_KERNEL);
>> +     if (!bridge)
>> +             return;
>> +
>> +     bridge->handle = handle;
>> +
>> +     if (ACPI_SUCCESS(acpi_get_handle(handle, "_EJ0", &dummy_handle)))
>> +             bridge->flags |= ROOT_BRIDGE_HAS_EJ0;
>> +     if (ACPI_SUCCESS(acpi_get_handle(handle, "_PS3", &dummy_handle)))
>> +             bridge->flags |= ROOT_BRIDGE_HAS_PS3;
>
> All of this attempts to duplicate the scanning code from scan.c in a very
> incomplete and questionable way.
>
> For example, what if the root bridge has _PR0?

will address that in following patch

>
>> +
>> +     list_add(&bridge->list, &acpi_root_bridge_list);
>> +}
>> +
>> +struct acpi_root_hp_work {
>> +     struct work_struct work;
>> +     acpi_handle handle;
>> +     u32 type;
>> +     void *context;
>> +};
>> +
>> +static void alloc_acpi_root_hp_work(acpi_handle handle, u32 type,
>> +                                     void *context,
>> +                                     void (*func)(struct work_struct *work))
>> +{
>> +     struct acpi_root_hp_work *hp_work;
>> +     int ret;
>> +
>> +     hp_work = kmalloc(sizeof(*hp_work), GFP_KERNEL);
>> +     if (!hp_work)
>> +             return;
>> +
>> +     hp_work->handle = handle;
>> +     hp_work->type = type;
>> +     hp_work->context = context;
>> +
>> +     INIT_WORK(&hp_work->work, func);
>> +     ret = queue_work(kacpi_hotplug_wq, &hp_work->work);
>> +     if (!ret)
>> +             kfree(hp_work);
>> +}
>
> The function above is called only once and used by __init stuff only.
> Why don't we move it to the caller and mark that caller as __init too?

will merge the function and shared with acpiphp

>
>> +
>> +static void handle_root_bridge_insertion(acpi_handle handle)
>> +{
>> +     struct acpi_device *device, *pdevice;
>> +     acpi_handle phandle;
>> +     int ret_val;
>> +
>> +     acpi_get_parent(handle, &phandle);
>> +     if (acpi_bus_get_device(phandle, &pdevice)) {
>> +             printk(KERN_DEBUG "no parent device, assuming NULL\n");
>> +             pdevice = NULL;
>> +     }
>> +     if (!acpi_bus_get_device(handle, &device)) {
>> +             /* check if  pci root_bus is removed */
>> +             struct acpi_pci_root *root = acpi_driver_data(device);
>> +             if (pci_find_bus(root->segment, root->secondary.start))
>> +                     return;
>> +
>> +             printk(KERN_DEBUG "bus exists... trim\n");
>> +             /* this shouldn't be in here, so remove
>> +              * the bus then re-add it...
>> +              */
>
> Why?  Shouldn't we just bail out here?

should be the same from acpiphp.

>
>> +             /* remove pci devices at first */
>> +             ret_val = acpi_bus_trim(device, 0);
>> +             printk(KERN_DEBUG "acpi_bus_trim stop return %x\n", ret_val);
>> +             /* remove acpi devices */
>> +             ret_val = acpi_bus_trim(device, 1);
>
> Oh, I see why you need the second argument of acpi_bus_trim() now.
>
> Do I think correctly that you want acpi_pci_root_remove() to be executed before
> all of the struct acpi_device objects are removed?  In which case why don't we
> call acpi_pci_root_remove() directly before doing the acpi_bus_trim(device, 1)
> instead of making the code next to impossible to understand?

yes.

>
>> +             printk(KERN_DEBUG "acpi_bus_trim remove return %x\n", ret_val);
>> +     }
>> +     if (acpi_bus_add(handle))
>> +             printk(KERN_ERR "cannot add bridge to acpi list\n");
>> +}
>> +
>> +static void _handle_hotplug_event_root(struct work_struct *work)
>> +{
>> +     struct acpi_root_bridge *bridge;
>> +     char objname[64];
>> +     struct acpi_buffer buffer = { .length = sizeof(objname),
>> +                                   .pointer = objname };
>> +     struct acpi_root_hp_work *hp_work;
>> +     acpi_handle handle;
>> +     u32 type;
>> +
>> +     hp_work = container_of(work, struct acpi_root_hp_work, work);
>> +     handle = hp_work->handle;
>> +     type = hp_work->type;
>> +
>> +     bridge = acpi_root_handle_to_bridge(handle);
>> +
>> +     acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
>> +
>> +     switch (type) {
>> +     case ACPI_NOTIFY_BUS_CHECK:
>> +             /* bus enumerate */
>> +             printk(KERN_DEBUG "%s: Bus check notify on %s\n", __func__,
>> +                              objname);
>> +             if (!bridge) {
>> +                     handle_root_bridge_insertion(handle);
>
> I don't think we should call add_acpi_root_bridge() for handle if the above
> fails.  So probably handle_root_bridge_insertion() should return error codes?

yes

>
>> +                     add_acpi_root_bridge(handle);

will remove root bridge in following patch.

>> +             }
>> +
>> +             break;
>> +
>> +     case ACPI_NOTIFY_DEVICE_CHECK:
>> +             /* device check */
>> +             printk(KERN_DEBUG "%s: Device check notify on %s\n", __func__,
>> +                              objname);
>> +             if (!bridge) {
>> +                     handle_root_bridge_insertion(handle);
>> +                     add_acpi_root_bridge(handle);
>> +             }
>> +             break;
>> +
>> +     default:
>> +             printk(KERN_WARNING "notify_handler: unknown event type 0x%x for %s\n",
>> +                              type, objname);
>> +             break;
>> +     }
>> +
>> +     kfree(hp_work); /* allocated in handle_hotplug_event_bridge */
>> +}
>> +
>> +static void handle_hotplug_event_root(acpi_handle handle, u32 type,
>> +                                     void *context)
>> +{
>> +     alloc_acpi_root_hp_work(handle, type, context,
>> +                             _handle_hotplug_event_root);
>> +}
>> +
>> +static acpi_status __init
>> +find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
>> +{
>> +     char objname[64];
>> +     struct acpi_buffer buffer = { .length = sizeof(objname),
>> +                                   .pointer = objname };
>> +     int *count = (int *)context;
>> +
>> +     if (!acpi_is_root_bridge(handle))
>> +             return AE_OK;
>> +
>> +     (*count)++;
>> +
>> +     acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
>> +
>> +     acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
>> +                             handle_hotplug_event_root, NULL);
>> +     printk(KERN_DEBUG "acpi root: %s notify handler installed\n", objname);
>> +
>> +     add_acpi_root_bridge(handle);
>> +
>> +     return AE_OK;
>> +}
>> +
>> +static int __init acpi_pci_root_hp_init(void)
>> +{
>> +     int num = 0;
>> +
>> +     acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
>> +             ACPI_UINT32_MAX, find_root_bridges, NULL, &num, NULL);
>> +
>> +     printk(KERN_DEBUG "Found %d acpi root devices\n", num);
>> +
>> +     return 0;
>> +}
>
> Why do we need to do that from an initcall?  Couldn't we simply hook up
> that code to acpi_pci_root_add() somewhere?

no, not inserted host bridge will not have acpi_pci_root_add called.

>
> And even if not, why don't we call acpi_pci_root_hp_init() from
> acpi_pci_root_init()?

will check if can move that to acpi_pci_root_init and address that in
another patch.

>
> All of the changes in acpiphp_glue.c look reasonable to me.

Good.
--
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


[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux