| Message ID | 1357663945-16054-5-git-send-email-jiang.liu@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wednesday, January 09, 2013 12:52:23 AM Jiang Liu wrote: > Currently the acpiphp driver fails to update hotplug slot information under > several conditions, such as: > 1) The bridge device is removed through /sys/bus/pci/devices/.../remove > 2) The bridge device is added/removed by PCI hotplug driver other than the > acpiphp driver itself. For example, if an IO expansion box with ACPI > hotplug slots available is hot-added by the pciehp driver, all ACPI > hotplug slots won't be discovered by the acpiphp driver. > > So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to > update ACPI hotplug slot information when PCI hotplug event happens. > > Signed-off-by: Jiang Liu <jiang.liu@huawei.com> > Signed-off-by: Yijing Wang <wangyijing@huawei.com> > --- > drivers/pci/hotplug/acpiphp_glue.c | 62 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 60 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > index 3d6d4fd..16fe692 100644 > --- a/drivers/pci/hotplug/acpiphp_glue.c > +++ b/drivers/pci/hotplug/acpiphp_glue.c > @@ -61,6 +61,7 @@ static void handle_hotplug_event_bridge (acpi_handle, u32, void *); > static void acpiphp_sanitize_bus(struct pci_bus *bus); > static void acpiphp_set_hpp_values(struct pci_bus *bus); > static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context); > +static struct acpiphp_bridge *acpiphp_handle_to_bridge(acpi_handle handle); > > /* callback routine to check for the existence of a pci dock device */ > static acpi_status > @@ -420,12 +421,14 @@ static void add_host_bridge(struct acpi_pci_root *root) > init_bridge_misc(bridge); > } > > - > /* allocate and initialize PCI-to-PCI bridge data structure */ > -static void add_p2p_bridge(acpi_handle *handle) > +static void add_p2p_bridge(acpi_handle handle) > { > struct acpiphp_bridge *bridge; > > + if (acpiphp_handle_to_bridge(handle)) > + return; > + > bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL); > if (bridge == NULL) { > err("out of memory\n"); > @@ -1428,6 +1431,58 @@ find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv) > return AE_OK ; > } > > +static void acpiphp_hp_notify_add(struct pci_dev *dev) > +{ > + acpi_handle handle = DEVICE_ACPI_HANDLE(&dev->dev); > + > + if (!dev->subordinate || !handle) > + return; > + > + /* check if this bridge has ejectable slots */ > + if (detect_ejectable_slots(handle) > 0) { > + dbg("found PCI-to-PCI bridge at PCI %s\n", pci_name(dev)); > + add_p2p_bridge(handle); > + } > +} > + > +static void acpiphp_hp_notify_del(struct pci_dev *dev) > +{ > + struct acpiphp_bridge *bridge, *tmp; > + struct pci_bus *bus = dev->subordinate; > + > + if (!bus) > + return; > + > + list_for_each_entry_safe(bridge, tmp, &bridge_list, list) > + if (bridge->pci_bus == bus) { > + cleanup_bridge(bridge); > + break; > + } > +} > + > +static int acpi_pci_hp_notify_fn(struct notifier_block *nb, > + unsigned long event, void *data) > +{ > + struct device *dev = data; > + > + switch (event) { > + case BUS_NOTIFY_ADD_DEVICE: > + acpiphp_hp_notify_add(to_pci_dev(dev)); > + break; > + case BUS_NOTIFY_DEL_DEVICE: > + acpiphp_hp_notify_del(to_pci_dev(dev)); > + break; > + default: > + return NOTIFY_DONE; > + } > + > + return NOTIFY_OK; > +} > + > +static struct notifier_block acpi_pci_hp_notifier = { > + .notifier_call = &acpi_pci_hp_notify_fn, > +}; > + > static struct acpi_pci_driver acpi_pci_hp_driver = { > .add = add_bridge, > .remove = remove_bridge, > @@ -1448,6 +1503,8 @@ int __init acpiphp_glue_init(void) > else > acpi_pci_register_driver(&acpi_pci_hp_driver); > > + bus_register_notifier(&pci_bus_type, &acpi_pci_hp_notifier); Your previous patch adds a PCI bus notifier for a similar thing. Why are you adding another one here? > + > return 0; > } > > @@ -1459,6 +1516,7 @@ int __init acpiphp_glue_init(void) > */ > void acpiphp_glue_exit(void) > { > + bus_unregister_notifier(&pci_bus_type, &acpi_pci_hp_notifier); > acpi_pci_unregister_driver(&acpi_pci_hp_driver); > } Thanks, Rafael
On 01/09/2013 08:04 AM, Rafael J. Wysocki wrote: > On Wednesday, January 09, 2013 12:52:23 AM Jiang Liu wrote: >> Currently the acpiphp driver fails to update hotplug slot information under >> several conditions, such as: >> 1) The bridge device is removed through /sys/bus/pci/devices/.../remove >> 2) The bridge device is added/removed by PCI hotplug driver other than the >> acpiphp driver itself. For example, if an IO expansion box with ACPI >> hotplug slots available is hot-added by the pciehp driver, all ACPI >> hotplug slots won't be discovered by the acpiphp driver. >> >> So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to >> update ACPI hotplug slot information when PCI hotplug event happens. >> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com> >> Signed-off-by: Yijing Wang <wangyijing@huawei.com> >> --- >> drivers/pci/hotplug/acpiphp_glue.c | 62 ++++++++++++++++++++++++++++++++++-- >> 1 file changed, 60 insertions(+), 2 deletions(-) >> snip >> +static struct notifier_block acpi_pci_hp_notifier = { >> + .notifier_call = &acpi_pci_hp_notify_fn, >> +}; >> + >> static struct acpi_pci_driver acpi_pci_hp_driver = { >> .add = add_bridge, >> .remove = remove_bridge, >> @@ -1448,6 +1503,8 @@ int __init acpiphp_glue_init(void) >> else >> acpi_pci_register_driver(&acpi_pci_hp_driver); >> >> + bus_register_notifier(&pci_bus_type, &acpi_pci_hp_notifier); > > Your previous patch adds a PCI bus notifier for a similar thing. Why are > you adding another one here? Hi Rafael, The acpiphp driver could be built as a module, and this bus notifier is used to setup/tear down ACPI based PCI hotplug controllers under a PCI bridge. The previous patch is to add/remove PCI slots. The PCI slot and PCI hotplug controller are related but different objects, so we use two notifiers to support them all. Thanks! Gerry > > >> + >> return 0; >> } >> >> @@ -1459,6 +1516,7 @@ int __init acpiphp_glue_init(void) >> */ >> void acpiphp_glue_exit(void) >> { >> + bus_unregister_notifier(&pci_bus_type, &acpi_pci_hp_notifier); >> acpi_pci_unregister_driver(&acpi_pci_hp_driver); >> } > > Thanks, > Rafael > > -- 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 Thursday, January 10, 2013 12:29:55 AM Jiang Liu wrote: > On 01/09/2013 08:04 AM, Rafael J. Wysocki wrote: > > On Wednesday, January 09, 2013 12:52:23 AM Jiang Liu wrote: > >> Currently the acpiphp driver fails to update hotplug slot information under > >> several conditions, such as: > >> 1) The bridge device is removed through /sys/bus/pci/devices/.../remove > >> 2) The bridge device is added/removed by PCI hotplug driver other than the > >> acpiphp driver itself. For example, if an IO expansion box with ACPI > >> hotplug slots available is hot-added by the pciehp driver, all ACPI > >> hotplug slots won't be discovered by the acpiphp driver. > >> > >> So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to > >> update ACPI hotplug slot information when PCI hotplug event happens. > >> > >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com> > >> Signed-off-by: Yijing Wang <wangyijing@huawei.com> > >> --- > >> drivers/pci/hotplug/acpiphp_glue.c | 62 ++++++++++++++++++++++++++++++++++-- > >> 1 file changed, 60 insertions(+), 2 deletions(-) > >> > snip > >> +static struct notifier_block acpi_pci_hp_notifier = { > >> + .notifier_call = &acpi_pci_hp_notify_fn, > >> +}; > >> + > >> static struct acpi_pci_driver acpi_pci_hp_driver = { > >> .add = add_bridge, > >> .remove = remove_bridge, > >> @@ -1448,6 +1503,8 @@ int __init acpiphp_glue_init(void) > >> else > >> acpi_pci_register_driver(&acpi_pci_hp_driver); > >> > >> + bus_register_notifier(&pci_bus_type, &acpi_pci_hp_notifier); > > > > Your previous patch adds a PCI bus notifier for a similar thing. Why are > > you adding another one here? > Hi Rafael, > The acpiphp driver could be built as a module, and this bus notifier > is used to setup/tear down ACPI based PCI hotplug controllers under a PCI bridge. > The previous patch is to add/remove PCI slots. The PCI slot and PCI hotplug > controller are related but different objects, so we use two notifiers to support > them all. I would be much happier if those notifiers were not needed. At least, I'm not sure why they need to be invoked by the driver core during device registration. In my opinion it would be much better if they were called only for the devices that might need them instead of all PCI devices. Thanks, Rafael
On 01/10/2013 04:23 AM, Rafael J. Wysocki wrote: > On Thursday, January 10, 2013 12:29:55 AM Jiang Liu wrote: >> On 01/09/2013 08:04 AM, Rafael J. Wysocki wrote: >>> On Wednesday, January 09, 2013 12:52:23 AM Jiang Liu wrote: >>>> Currently the acpiphp driver fails to update hotplug slot information under >>>> several conditions, such as: >>>> 1) The bridge device is removed through /sys/bus/pci/devices/.../remove >>>> 2) The bridge device is added/removed by PCI hotplug driver other than the >>>> acpiphp driver itself. For example, if an IO expansion box with ACPI >>>> hotplug slots available is hot-added by the pciehp driver, all ACPI >>>> hotplug slots won't be discovered by the acpiphp driver. >>>> >>>> So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to >>>> update ACPI hotplug slot information when PCI hotplug event happens. >>>> >>>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com> >>>> Signed-off-by: Yijing Wang <wangyijing@huawei.com> >>>> --- >>>> drivers/pci/hotplug/acpiphp_glue.c | 62 ++++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 60 insertions(+), 2 deletions(-) >>>> >> snip >>>> +static struct notifier_block acpi_pci_hp_notifier = { >>>> + .notifier_call = &acpi_pci_hp_notify_fn, >>>> +}; >>>> + >>>> static struct acpi_pci_driver acpi_pci_hp_driver = { >>>> .add = add_bridge, >>>> .remove = remove_bridge, >>>> @@ -1448,6 +1503,8 @@ int __init acpiphp_glue_init(void) >>>> else >>>> acpi_pci_register_driver(&acpi_pci_hp_driver); >>>> >>>> + bus_register_notifier(&pci_bus_type, &acpi_pci_hp_notifier); >>> >>> Your previous patch adds a PCI bus notifier for a similar thing. Why are >>> you adding another one here? >> Hi Rafael, >> The acpiphp driver could be built as a module, and this bus notifier >> is used to setup/tear down ACPI based PCI hotplug controllers under a PCI bridge. >> The previous patch is to add/remove PCI slots. The PCI slot and PCI hotplug >> controller are related but different objects, so we use two notifiers to support >> them all. > > I would be much happier if those notifiers were not needed. At least, I'm not > sure why they need to be invoked by the driver core during device registration. > > In my opinion it would be much better if they were called only for the devices > that might need them instead of all PCI devices. > Hi Rafael, The whole thing seems a little complex, and I will try my best to explain about it:) Essentially a PCI slot is associated with a PCI bus, and a PCI bus is associated with a host bridge or P2P bridge. When hot-adding/hot-removing a PCI bus, we need to create/destroy PCI slots associated with it. So the pci_slot and acpiphp drivers need to be notified when hot-adding/hot-removing a PCI bus. But there's no common mechanism to get notified when PCI buses are being hot-added/hot-removed. So my first implementation has introduced a dedicated notifier chain for PCI bus addition/removal. Later with ongoing changes to the PCI core, I found we could rely on the existing bus notification mechanism too, so that becomes the current implementation. I have a proposal to unify the process for boot time and hotplug as below: 1) Change pci_slot and acpiphp as built-in instead of modules. 2) Get rid of acpi_pci_driver for pci_slot and acpiphp driver. 3) Register some forms of callbacks to create/destroy hotplug controllers/slots when creating/destroying PCI buses. 3.a) Rely on the bus notification mechanism to invoke callbacks. This solution is simple, but with a little overhead 3.b) Introduce a dedicated notifier chain for PCI bus hot-adding/hot-removing. This solution could get rid of the overhead, but with more code changes. 3.c) Invoke callbacks from acpi_bus_type->setup/cleanup. This solution could only support ACPI based slots, such as pci_slot and acpiphp. Any recommendations? Regards! Gerry > Thanks, > Rafael > > -- 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 Sunday, January 13, 2013 11:13:28 PM Jiang Liu wrote: > On 01/10/2013 04:23 AM, Rafael J. Wysocki wrote: > > On Thursday, January 10, 2013 12:29:55 AM Jiang Liu wrote: > >> On 01/09/2013 08:04 AM, Rafael J. Wysocki wrote: > >>> On Wednesday, January 09, 2013 12:52:23 AM Jiang Liu wrote: > >>>> Currently the acpiphp driver fails to update hotplug slot information under > >>>> several conditions, such as: > >>>> 1) The bridge device is removed through /sys/bus/pci/devices/.../remove > >>>> 2) The bridge device is added/removed by PCI hotplug driver other than the > >>>> acpiphp driver itself. For example, if an IO expansion box with ACPI > >>>> hotplug slots available is hot-added by the pciehp driver, all ACPI > >>>> hotplug slots won't be discovered by the acpiphp driver. > >>>> > >>>> So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to > >>>> update ACPI hotplug slot information when PCI hotplug event happens. > >>>> > >>>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com> > >>>> Signed-off-by: Yijing Wang <wangyijing@huawei.com> > >>>> --- > >>>> drivers/pci/hotplug/acpiphp_glue.c | 62 ++++++++++++++++++++++++++++++++++-- > >>>> 1 file changed, 60 insertions(+), 2 deletions(-) > >>>> > >> snip > >>>> +static struct notifier_block acpi_pci_hp_notifier = { > >>>> + .notifier_call = &acpi_pci_hp_notify_fn, > >>>> +}; > >>>> + > >>>> static struct acpi_pci_driver acpi_pci_hp_driver = { > >>>> .add = add_bridge, > >>>> .remove = remove_bridge, > >>>> @@ -1448,6 +1503,8 @@ int __init acpiphp_glue_init(void) > >>>> else > >>>> acpi_pci_register_driver(&acpi_pci_hp_driver); > >>>> > >>>> + bus_register_notifier(&pci_bus_type, &acpi_pci_hp_notifier); > >>> > >>> Your previous patch adds a PCI bus notifier for a similar thing. Why are > >>> you adding another one here? > >> Hi Rafael, > >> The acpiphp driver could be built as a module, and this bus notifier > >> is used to setup/tear down ACPI based PCI hotplug controllers under a PCI bridge. > >> The previous patch is to add/remove PCI slots. The PCI slot and PCI hotplug > >> controller are related but different objects, so we use two notifiers to support > >> them all. > > > > I would be much happier if those notifiers were not needed. At least, I'm not > > sure why they need to be invoked by the driver core during device registration. > > > > In my opinion it would be much better if they were called only for the devices > > that might need them instead of all PCI devices. > > > Hi Rafael, > The whole thing seems a little complex, and I will try my best to explain > about it:) > Essentially a PCI slot is associated with a PCI bus, and a PCI bus is > associated with a host bridge or P2P bridge. When hot-adding/hot-removing a PCI bus, > we need to create/destroy PCI slots associated with it. So the pci_slot and acpiphp > drivers need to be notified when hot-adding/hot-removing a PCI bus. > But there's no common mechanism to get notified when PCI buses are being > hot-added/hot-removed. So my first implementation has introduced a dedicated notifier > chain for PCI bus addition/removal. Later with ongoing changes to the PCI core, > I found we could rely on the existing bus notification mechanism too, so that becomes > the current implementation. > I have a proposal to unify the process for boot time and hotplug as below: > 1) Change pci_slot and acpiphp as built-in instead of modules. Sure, go ahead with that. > 2) Get rid of acpi_pci_driver for pci_slot and acpiphp driver. Again, I have no objection here. Moreover, I think we might be better off without the acpi_pci_driver things at all. > 3) Register some forms of callbacks to create/destroy hotplug controllers/slots > when creating/destroying PCI buses. > 3.a) Rely on the bus notification mechanism to invoke callbacks. This solution is > simple, but with a little overhead > 3.b) Introduce a dedicated notifier chain for PCI bus hot-adding/hot-removing. > This solution could get rid of the overhead, but with more code changes. > 3.c) Invoke callbacks from acpi_bus_type->setup/cleanup. This solution could > only support ACPI based slots, such as pci_slot and acpiphp. As I said before, I'd prefer these notifications to be done specifically for PCI buses or bridges, so I'd vote for 3.b), depending on the implementation. Thanks, Rafael
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c index 3d6d4fd..16fe692 100644 --- a/drivers/pci/hotplug/acpiphp_glue.c +++ b/drivers/pci/hotplug/acpiphp_glue.c @@ -61,6 +61,7 @@ static void handle_hotplug_event_bridge (acpi_handle, u32, void *); static void acpiphp_sanitize_bus(struct pci_bus *bus); static void acpiphp_set_hpp_values(struct pci_bus *bus); static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context); +static struct acpiphp_bridge *acpiphp_handle_to_bridge(acpi_handle handle); /* callback routine to check for the existence of a pci dock device */ static acpi_status @@ -420,12 +421,14 @@ static void add_host_bridge(struct acpi_pci_root *root) init_bridge_misc(bridge); } - /* allocate and initialize PCI-to-PCI bridge data structure */ -static void add_p2p_bridge(acpi_handle *handle) +static void add_p2p_bridge(acpi_handle handle) { struct acpiphp_bridge *bridge; + if (acpiphp_handle_to_bridge(handle)) + return; + bridge = kzalloc(sizeof(struct acpiphp_bridge), GFP_KERNEL); if (bridge == NULL) { err("out of memory\n"); @@ -1428,6 +1431,58 @@ find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv) return AE_OK ; } +static void acpiphp_hp_notify_add(struct pci_dev *dev) +{ + acpi_handle handle = DEVICE_ACPI_HANDLE(&dev->dev); + + if (!dev->subordinate || !handle) + return; + + /* check if this bridge has ejectable slots */ + if (detect_ejectable_slots(handle) > 0) { + dbg("found PCI-to-PCI bridge at PCI %s\n", pci_name(dev)); + add_p2p_bridge(handle); + } +} + +static void acpiphp_hp_notify_del(struct pci_dev *dev) +{ + struct acpiphp_bridge *bridge, *tmp; + struct pci_bus *bus = dev->subordinate; + + if (!bus) + return; + + list_for_each_entry_safe(bridge, tmp, &bridge_list, list) + if (bridge->pci_bus == bus) { + cleanup_bridge(bridge); + break; + } +} + +static int acpi_pci_hp_notify_fn(struct notifier_block *nb, + unsigned long event, void *data) +{ + struct device *dev = data; + + switch (event) { + case BUS_NOTIFY_ADD_DEVICE: + acpiphp_hp_notify_add(to_pci_dev(dev)); + break; + case BUS_NOTIFY_DEL_DEVICE: + acpiphp_hp_notify_del(to_pci_dev(dev)); + break; + default: + return NOTIFY_DONE; + } + + return NOTIFY_OK; +} + +static struct notifier_block acpi_pci_hp_notifier = { + .notifier_call = &acpi_pci_hp_notify_fn, +}; + static struct acpi_pci_driver acpi_pci_hp_driver = { .add = add_bridge, .remove = remove_bridge, @@ -1448,6 +1503,8 @@ int __init acpiphp_glue_init(void) else acpi_pci_register_driver(&acpi_pci_hp_driver); + bus_register_notifier(&pci_bus_type, &acpi_pci_hp_notifier); + return 0; } @@ -1459,6 +1516,7 @@ int __init acpiphp_glue_init(void) */ void acpiphp_glue_exit(void) { + bus_unregister_notifier(&pci_bus_type, &acpi_pci_hp_notifier); acpi_pci_unregister_driver(&acpi_pci_hp_driver); }