| Message ID | 1357944049-29620-9-git-send-email-yinghai@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Friday, January 11, 2013 02:40:35 PM Yinghai Lu wrote: > It causes confusion. > > We may only need acpi hp for pci host bridge. What does this mean? > Split host bridge hot-add support to pci_root_hp, and keep acpiphp simple. s/Split/Move/ I suppose? In any case that's not telling the whole story, because the patch doesn't just move code from one file to another. > -v2: put back pci_root_hp change in one patch > -v3: add pcibios_resource_survey_bus() calling > -v4: remove not needed code with remove_bridge > -v5: put back support for acpiphp support for slots just on root bus. > -v6: change some functions to *_p2p_* to make it more clean. > -v7: split hot_added change out. > -v8: Move to pci_root.c instead of adding another file requested by Bjorn. > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > --- > drivers/acpi/pci_root.c | 220 ++++++++++++++++++++++++++++++++++++ > drivers/pci/hotplug/acpiphp_glue.c | 59 +++------- > 2 files changed, 235 insertions(+), 44 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index 471b2dc..5c1f462c 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -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? 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? > + > +#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? > + > + 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? > + > +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? > + /* 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? > + 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? > + add_acpi_root_bridge(handle); > + } > + > + 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? And even if not, why don't we call acpi_pci_root_hp_init() from acpi_pci_root_init()? All of the changes in acpiphp_glue.c look reasonable to me. Thanks, Rafael
On Sat, Jan 12, 2013 at 3:18 PM, Rafael J. Wysocki <rjw@sisk.pl> 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-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 14, 2013 at 10:44 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Sat, Jan 12, 2013 at 3:18 PM, Rafael J. Wysocki <rjw@sisk.pl> 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. please check if you are ok with merging attached 5 patches in to one patches instead. Thanks Yinghai
On Tuesday, January 15, 2013 07:54:22 AM Yinghai Lu wrote: > On Mon, Jan 14, 2013 at 10:44 PM, Yinghai Lu <yinghai@kernel.org> wrote: > > On Sat, Jan 12, 2013 at 3:18 PM, Rafael J. Wysocki <rjw@sisk.pl> 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. > > please check if you are ok with merging attached 5 patches in to one > patches instead. Do I understand correctly that you're asking whether to merge the attached patches together, so as to obtain one combined patch? If so, yes, I'd like that to be done. Thanks, Rafael
On Tue, Jan 15, 2013 at 2:00 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > Do I understand correctly that you're asking whether to merge the attached > patches together, so as to obtain one combined patch? > > If so, yes, I'd like that to be done. good, will merge them for easy review. Thanks Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 471b2dc..5c1f462c 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -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 + */ + +static LIST_HEAD(acpi_root_bridge_list); +struct acpi_root_bridge { + struct list_head list; + acpi_handle handle; + u32 flags; +}; + +/* bridge flags */ +#define ROOT_BRIDGE_HAS_EJ0 (0x00000002) +#define ROOT_BRIDGE_HAS_PS3 (0x00000080) + +#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; + + /* 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; + + 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); +} + +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... + */ + /* 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); + 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); + add_acpi_root_bridge(handle); + } + + 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; +} + +subsys_initcall(acpi_pci_root_hp_init); diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c index 1e5c5df..02c41ab 100644 --- a/drivers/pci/hotplug/acpiphp_glue.c +++ b/drivers/pci/hotplug/acpiphp_glue.c @@ -543,10 +543,13 @@ static void cleanup_bridge(struct acpiphp_bridge *bridge) acpi_status status; acpi_handle handle = bridge->handle; - status = acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY, + if (bridge->type != BRIDGE_TYPE_HOST) { + status = acpi_remove_notify_handler(handle, + ACPI_SYSTEM_NOTIFY, handle_hotplug_event_bridge); - if (ACPI_FAILURE(status)) - err("failed to remove notify handler\n"); + if (ACPI_FAILURE(status)) + err("failed to remove notify handler\n"); + } if ((bridge->type != BRIDGE_TYPE_HOST) && ((bridge->flags & BRIDGE_HAS_EJ0) && bridge->func)) { @@ -630,9 +633,6 @@ static void remove_bridge(struct acpi_pci_root *root) bridge = acpiphp_handle_to_bridge(handle); if (bridge) cleanup_bridge(bridge); - else - acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY, - handle_hotplug_event_bridge); } static int power_on_slot(struct acpiphp_slot *slot) @@ -1123,18 +1123,12 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus) } /* Program resources in newly inserted bridge */ -static int acpiphp_configure_bridge (acpi_handle handle) +static int acpiphp_configure_p2p_bridge(acpi_handle handle) { - struct pci_bus *bus; + struct pci_dev *pdev = acpi_get_pci_dev(handle); + struct pci_bus *bus = pdev->subordinate; - if (acpi_is_root_bridge(handle)) { - struct acpi_pci_root *root = acpi_pci_find_root(handle); - bus = root->bus; - } else { - struct pci_dev *pdev = acpi_get_pci_dev(handle); - bus = pdev->subordinate; - pci_dev_put(pdev); - } + pci_dev_put(pdev); pci_bus_size_bridges(bus); pci_bus_assign_resources(bus); @@ -1144,7 +1138,7 @@ static int acpiphp_configure_bridge (acpi_handle handle) return 0; } -static void handle_bridge_insertion(acpi_handle handle, u32 type) +static void handle_p2p_bridge_insertion(acpi_handle handle, u32 type) { struct acpi_device *device; @@ -1162,8 +1156,8 @@ static void handle_bridge_insertion(acpi_handle handle, u32 type) err("ACPI device object missing\n"); return; } - if (!acpiphp_configure_bridge(handle)) - add_bridge(handle); + if (!acpiphp_configure_p2p_bridge(handle)) + add_p2p_bridge(handle); else err("cannot configure and start bridge\n"); @@ -1249,7 +1243,7 @@ static void _handle_hotplug_event_bridge(struct work_struct *work) if (acpi_bus_get_device(handle, &device)) { /* This bridge must have just been physically inserted */ - handle_bridge_insertion(handle, type); + handle_p2p_bridge_insertion(handle, type); goto out; } @@ -1426,21 +1420,6 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type, _handle_hotplug_event_func); } -static acpi_status -find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv) -{ - int *count = (int *)context; - - if (!acpi_is_root_bridge(handle)) - return AE_OK; - - (*count)++; - acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY, - handle_hotplug_event_bridge, NULL); - - return AE_OK ; -} - static struct acpi_pci_driver acpi_pci_hp_driver = { .add = add_bridge, .remove = remove_bridge, @@ -1451,15 +1430,7 @@ static struct acpi_pci_driver acpi_pci_hp_driver = { */ int __init acpiphp_glue_init(void) { - int num = 0; - - acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, - ACPI_UINT32_MAX, find_root_bridges, NULL, &num, NULL); - - if (num <= 0) - return -1; - else - acpi_pci_register_driver(&acpi_pci_hp_driver); + acpi_pci_register_driver(&acpi_pci_hp_driver); return 0; }
It causes confusion. We may only need acpi hp for pci host bridge. Split host bridge hot-add support to pci_root_hp, and keep acpiphp simple. -v2: put back pci_root_hp change in one patch -v3: add pcibios_resource_survey_bus() calling -v4: remove not needed code with remove_bridge -v5: put back support for acpiphp support for slots just on root bus. -v6: change some functions to *_p2p_* to make it more clean. -v7: split hot_added change out. -v8: Move to pci_root.c instead of adding another file requested by Bjorn. Signed-off-by: Yinghai Lu <yinghai@kernel.org> --- drivers/acpi/pci_root.c | 220 ++++++++++++++++++++++++++++++++++++ drivers/pci/hotplug/acpiphp_glue.c | 59 +++------- 2 files changed, 235 insertions(+), 44 deletions(-)