| Message ID | 1351058750-4275-3-git-send-email-tangchen@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wed, 2012-10-24 at 14:05 +0800, Tang Chen wrote: > This patch introduces a new function container_device_remove() to do > the container hot-remove job. It works like the following: > > 1. call acpi_bus_trim(device, 0) to stop the container device. > 2. generate the KOBJ_OFFLINE uevent. (Did I do this correct ?) > 3. call acpi_bus_hot_remove_device(), which will call acpi_bus_trim(device, 1) > to remove the container. > > This patch is based on Lu Yinghai's work. > git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-split-pci-root-hp-2 > > Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com> > --- > drivers/acpi/container.c | 58 ++++++++++++++++++++++++++++++++++++++------- > 1 files changed, 49 insertions(+), 9 deletions(-) > > diff --git a/drivers/acpi/container.c b/drivers/acpi/container.c > index d300e03..9676578 100644 > --- a/drivers/acpi/container.c > +++ b/drivers/acpi/container.c > @@ -166,6 +166,35 @@ static int container_device_add(struct acpi_device **device, acpi_handle handle) > return result; > } > > +static int container_device_remove(struct acpi_device *device) > +{ > + int ret; > + struct acpi_eject_event *ej_event; > + > + /* stop container device at first */ > + ret = acpi_bus_trim(device, 0); Hi Tang, Why do you need to call acpi_bus_trim(device,0) to stop the container device first? > + printk(KERN_WARNING "acpi_bus_trim stop return %x\n", ret); Do you need this message in the normal case? If so, I'd suggest to use pr_debug(). > + if (ret) > + return ret; > + > + /* event originated from ACPI eject notification */ > + device->flags.eject_pending = 1; You do not need to set the eject_pending flag when the handler calls acpi_bus_hot_remove_device(). It was set before because the handler did not initiate the hot-remove operation. > + > + /* send the uevent before remove the device */ > + kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE); > + > + ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL); > + if (!ej_event) > + return -ENOMEM; > + > + ej_event->device = device; > + ej_event->event = ACPI_NOTIFY_EJECT_REQUEST; > + > + acpi_bus_hot_remove_device(ej_event); > + > + return 0; > +} > + > static void __container_notify_cb(struct work_struct *work) > { > struct acpi_device *device = NULL; > @@ -181,6 +210,9 @@ static void __container_notify_cb(struct work_struct *work) > handle = hp_work->handle; > type = hp_work->type; > > + present = is_device_present(handle); > + status = acpi_bus_get_device(handle, &device); > + > switch (type) { > case ACPI_NOTIFY_BUS_CHECK: > /* Fall through */ > @@ -189,13 +221,14 @@ static void __container_notify_cb(struct work_struct *work) > (type == ACPI_NOTIFY_BUS_CHECK) ? > "ACPI_NOTIFY_BUS_CHECK" : "ACPI_NOTIFY_DEVICE_CHECK"); > > - present = is_device_present(handle); > - status = acpi_bus_get_device(handle, &device); > if (!present) { > if (ACPI_SUCCESS(status)) { > /* device exist and this is a remove request */ > - device->flags.eject_pending = 1; > - kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE); > + result = container_device_remove(device); > + if (result) { > + printk(KERN_WARNING "Failed to remove container\n"); > + break; > + } > return; > } > break; > @@ -215,12 +248,19 @@ static void __container_notify_cb(struct work_struct *work) > break; > > case ACPI_NOTIFY_EJECT_REQUEST: > - if (!acpi_bus_get_device(handle, &device) && device) { > - device->flags.eject_pending = 1; > - kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE); > - return; > + printk(KERN_WARNING "Container driver received %s event\n", > + "ACPI_NOTIFY_EJECT_REQUEST"); Same as other comment. Suggest to use pr_debug(). > + > + if (!present || ACPI_FAILURE(status) || !device) > + break; > + > + result = container_device_remove(device); > + if (result) { > + printk(KERN_WARNING "Failed to remove container\n"); Please use pr_warn(). Thanks, -Toshi > + break; > } > - break; > + > + return; > > default: > /* non-hotplug event; possibly handled by other handler */ -- 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 Wed, Oct 24, 2012 at 11:14 AM, Toshi Kani <toshi.kani@hp.com> wrote: > On Wed, 2012-10-24 at 14:05 +0800, Tang Chen wrote: >> + result = container_device_remove(device); >> + if (result) { >> + printk(KERN_WARNING "Failed to remove container\n"); > > Please use pr_warn(). I think you should use dev_warn() and similar when possible. In a year or two, the text "Failed to remove container" all by itself in a dmesg log is not going to mean anything to anybody except you, and it doesn't give any clue where to start looking for issues. I also have a larger question. I'm not sure if drivers/acpi/container.c does anything important in the first place. The code in it looks an awful lot like the code in drivers/acpi/processor_driver.c, drivers/acpi/acpi_memhotplug.c, and drivers/pci/hotplug/acpiphp_glue.c. To me, it looks like container.c (as well as the other places I mentioned) is an attempt to compensate for the lack of hotplug support in the ACPI core. If the ACPI core had generic hotplug support, e.g., if it handled BUS_CHECK, DEVICE_CHECK, and EJECT_REQUEST notifications and turned those into the appropriate calls to driver .add()/.start() and .remove() methods, would we need container.c at all? Bjorn -- 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 2012-10-25 2:02, Bjorn Helgaas wrote: > On Wed, Oct 24, 2012 at 11:14 AM, Toshi Kani <toshi.kani@hp.com> wrote: >> On Wed, 2012-10-24 at 14:05 +0800, Tang Chen wrote: > >>> + result = container_device_remove(device); >>> + if (result) { >>> + printk(KERN_WARNING "Failed to remove container\n"); >> >> Please use pr_warn(). > > I think you should use dev_warn() and similar when possible. In a > year or two, the text "Failed to remove container" all by itself in a > dmesg log is not going to mean anything to anybody except you, and it > doesn't give any clue where to start looking for issues. > > I also have a larger question. I'm not sure if > drivers/acpi/container.c does anything important in the first place. > The code in it looks an awful lot like the code in > drivers/acpi/processor_driver.c, drivers/acpi/acpi_memhotplug.c, and > drivers/pci/hotplug/acpiphp_glue.c. > > To me, it looks like container.c (as well as the other places I > mentioned) is an attempt to compensate for the lack of hotplug support > in the ACPI core. If the ACPI core had generic hotplug support, e.g., > if it handled BUS_CHECK, DEVICE_CHECK, and EJECT_REQUEST notifications > and turned those into the appropriate calls to driver .add()/.start() > and .remove() methods, would we need container.c at all? Hi Bjorn, It's true that container driver is just for hotplug. We are working on a hotplug framework which will consolidate all hotplug logic into one driver. --Gerry > > Bjorn > > . > -- 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
Hi Toshi, On 10/25/2012 01:14 AM, Toshi Kani wrote: > On Wed, 2012-10-24 at 14:05 +0800, Tang Chen wrote: >> +static int container_device_remove(struct acpi_device *device) >> +{ >> + int ret; >> + struct acpi_eject_event *ej_event; >> + >> + /* stop container device at first */ >> + ret = acpi_bus_trim(device, 0); > > Hi Tang, > > Why do you need to call acpi_bus_trim(device,0) to stop the container > device first? This issue was introduced by Lu Yinghai, I think he could give a better answer than me. :) Please refer to the following url: http://www.spinics.net/lists/linux-pci/msg17667.html However, this is not applied into the pci tree yet. > >> + printk(KERN_WARNING "acpi_bus_trim stop return %x\n", ret); > > Do you need this message in the normal case? If so, I'd suggest to use > pr_debug(). > >> + if (ret) >> + return ret; >> + >> + /* event originated from ACPI eject notification */ >> + device->flags.eject_pending = 1; > > You do not need to set the eject_pending flag when the handler calls > acpi_bus_hot_remove_device(). It was set before because the handler did > not initiate the hot-remove operation. I just set it to keep the logic the same as before. And thanks for telling me this. :) > ... >> + printk(KERN_WARNING "Container driver received %s event\n", >> + "ACPI_NOTIFY_EJECT_REQUEST"); > > Same as other comment. Suggest to use pr_debug(). OK. :) > >> + >> + if (!present || ACPI_FAILURE(status) || !device) >> + break; >> + >> + result = container_device_remove(device); >> + if (result) { >> + printk(KERN_WARNING "Failed to remove container\n"); > > Please use pr_warn(). > > Thanks, > -Toshi > >> + break; >> } >> - break; >> + >> + return; >> >> default: >> /* non-hotplug event; possibly handled by other handler */ > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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 2012-10-25 9:31, Tang Chen wrote: > Hi Toshi, > > On 10/25/2012 01:14 AM, Toshi Kani wrote: >> On Wed, 2012-10-24 at 14:05 +0800, Tang Chen wrote: >>> +static int container_device_remove(struct acpi_device *device) >>> +{ >>> + int ret; >>> + struct acpi_eject_event *ej_event; >>> + >>> + /* stop container device at first */ >>> + ret = acpi_bus_trim(device, 0); >> >> Hi Tang, >> >> Why do you need to call acpi_bus_trim(device,0) to stop the container >> device first? > > This issue was introduced by Lu Yinghai, I think he could give a better > answer than me. :) > Please refer to the following url: > > http://www.spinics.net/lists/linux-pci/msg17667.html > > However, this is not applied into the pci tree yet. We have worked out a patch set to clean up the logic for PCI/ACPI binding relationship. It updates PCI/ACPI binding relationship by registering bus notification onto pci_bus_type instead of hooking into the ACPI/glue.c. To accommodate that patch set, the ACPI device destroy process has been split into two steps: 1) acpi_bus_trim(device,0) to unbind ACPI drivers 2) acpi_bus_trim(device,1) to destroy ACPI devices > >> >>> + printk(KERN_WARNING "acpi_bus_trim stop return %x\n", ret); >> >> Do you need this message in the normal case? If so, I'd suggest to use >> pr_debug(). >> >>> + if (ret) >>> + return ret; >>> + >>> + /* event originated from ACPI eject notification */ >>> + device->flags.eject_pending = 1; >> >> You do not need to set the eject_pending flag when the handler calls >> acpi_bus_hot_remove_device(). It was set before because the handler did >> not initiate the hot-remove operation. > > I just set it to keep the logic the same as before. > And thanks for telling me this. :) > >> > ... >>> + printk(KERN_WARNING "Container driver received %s event\n", >>> + "ACPI_NOTIFY_EJECT_REQUEST"); >> >> Same as other comment. Suggest to use pr_debug(). > > OK. :) > >> >>> + >>> + if (!present || ACPI_FAILURE(status) || !device) >>> + break; >>> + >>> + result = container_device_remove(device); >>> + if (result) { >>> + printk(KERN_WARNING "Failed to remove container\n"); >> >> Please use pr_warn(). >> >> Thanks, >> -Toshi >> >>> + break; >>> } >>> - break; >>> + >>> + return; >>> >>> default: >>> /* non-hotplug event; possibly handled by other handler */ >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > > . > -- 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 Thu, 2012-10-25 at 09:47 +0800, Jiang Liu wrote: > On 2012-10-25 9:31, Tang Chen wrote: > > Hi Toshi, > > > > On 10/25/2012 01:14 AM, Toshi Kani wrote: > >> On Wed, 2012-10-24 at 14:05 +0800, Tang Chen wrote: > >>> +static int container_device_remove(struct acpi_device *device) > >>> +{ > >>> + int ret; > >>> + struct acpi_eject_event *ej_event; > >>> + > >>> + /* stop container device at first */ > >>> + ret = acpi_bus_trim(device, 0); > >> > >> Hi Tang, > >> > >> Why do you need to call acpi_bus_trim(device,0) to stop the container > >> device first? > > > > This issue was introduced by Lu Yinghai, I think he could give a better > > answer than me. :) > > Please refer to the following url: > > > > http://www.spinics.net/lists/linux-pci/msg17667.html > > > > However, this is not applied into the pci tree yet. > We have worked out a patch set to clean up the logic for PCI/ACPI binding > relationship. It updates PCI/ACPI binding relationship by registering bus > notification onto pci_bus_type instead of hooking into the ACPI/glue.c. Thanks for the info and pointer. Tang, I'd suggest you add such info to the comment so that others know that this step is needed for removing PCI bridges. It helps us to know where to look at... > To accommodate that patch set, the ACPI device destroy process has been > split into two steps: > 1) acpi_bus_trim(device,0) to unbind ACPI drivers Does this step also detach PCI drivers from PCI cards as well? Thanks, -Toshi > 2) acpi_bus_trim(device,1) to destroy ACPI devices > > > > >> > >>> + printk(KERN_WARNING "acpi_bus_trim stop return %x\n", ret); > >> > >> Do you need this message in the normal case? If so, I'd suggest to use > >> pr_debug(). > >> > >>> + if (ret) > >>> + return ret; > >>> + > >>> + /* event originated from ACPI eject notification */ > >>> + device->flags.eject_pending = 1; > >> > >> You do not need to set the eject_pending flag when the handler calls > >> acpi_bus_hot_remove_device(). It was set before because the handler did > >> not initiate the hot-remove operation. > > > > I just set it to keep the logic the same as before. > > And thanks for telling me this. :) > > > >> > > ... > >>> + printk(KERN_WARNING "Container driver received %s event\n", > >>> + "ACPI_NOTIFY_EJECT_REQUEST"); > >> > >> Same as other comment. Suggest to use pr_debug(). > > > > OK. :) > > > >> > >>> + > >>> + if (!present || ACPI_FAILURE(status) || !device) > >>> + break; > >>> + > >>> + result = container_device_remove(device); > >>> + if (result) { > >>> + printk(KERN_WARNING "Failed to remove container\n"); > >> > >> Please use pr_warn(). > >> > >> Thanks, > >> -Toshi > >> > >>> + break; > >>> } > >>> - break; > >>> + > >>> + return; > >>> > >>> default: > >>> /* non-hotplug event; possibly handled by other handler */ > >> > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >> > > > > > > . > > > > -- 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
Hi Toshi, On 10/26/2012 01:20 AM, Toshi Kani wrote: ... >>>> Why do you need to call acpi_bus_trim(device,0) to stop the container >>>> device first? >>> >>> This issue was introduced by Lu Yinghai, I think he could give a better >>> answer than me. :) >>> Please refer to the following url: >>> >>> http://www.spinics.net/lists/linux-pci/msg17667.html >>> >>> However, this is not applied into the pci tree yet. >> We have worked out a patch set to clean up the logic for PCI/ACPI binding >> relationship. It updates PCI/ACPI binding relationship by registering bus >> notification onto pci_bus_type instead of hooking into the ACPI/glue.c. > > Thanks for the info and pointer. Tang, I'd suggest you add such info to > the comment so that others know that this step is needed for removing > PCI bridges. It helps us to know where to look at... OK, I'll add it in the next version. :) > >> To accommodate that patch set, the ACPI device destroy process has been >> split into two steps: >> 1) acpi_bus_trim(device,0) to unbind ACPI drivers > > Does this step also detach PCI drivers from PCI cards as well? Yes, it calls device_release_driver() to release the device driver. device_release_driver() |->__device_release_driver() |->dev->driver = NULL; Thanks. :) > > Thanks, > -Toshi > > -- 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 Fri, 2012-10-26 at 13:43 +0800, Tang Chen wrote: > Hi Toshi, > > On 10/26/2012 01:20 AM, Toshi Kani wrote: > ... > >>>> Why do you need to call acpi_bus_trim(device,0) to stop the container > >>>> device first? > >>> > >>> This issue was introduced by Lu Yinghai, I think he could give a better > >>> answer than me. :) > >>> Please refer to the following url: > >>> > >>> http://www.spinics.net/lists/linux-pci/msg17667.html > >>> > >>> However, this is not applied into the pci tree yet. > >> We have worked out a patch set to clean up the logic for PCI/ACPI binding > >> relationship. It updates PCI/ACPI binding relationship by registering bus > >> notification onto pci_bus_type instead of hooking into the ACPI/glue.c. > > > > Thanks for the info and pointer. Tang, I'd suggest you add such info to > > the comment so that others know that this step is needed for removing > > PCI bridges. It helps us to know where to look at... > > OK, I'll add it in the next version. :) > > > > >> To accommodate that patch set, the ACPI device destroy process has been > >> split into two steps: > >> 1) acpi_bus_trim(device,0) to unbind ACPI drivers > > > > Does this step also detach PCI drivers from PCI cards as well? > > Yes, it calls device_release_driver() to release the device driver. > > device_release_driver() > |->__device_release_driver() > |->dev->driver = NULL; I see. Thanks for the info. -Toshi > > Thanks. :) > > > > > Thanks, > > -Toshi > > > > > -- 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/container.c b/drivers/acpi/container.c index d300e03..9676578 100644 --- a/drivers/acpi/container.c +++ b/drivers/acpi/container.c @@ -166,6 +166,35 @@ static int container_device_add(struct acpi_device **device, acpi_handle handle) return result; } +static int container_device_remove(struct acpi_device *device) +{ + int ret; + struct acpi_eject_event *ej_event; + + /* stop container device at first */ + ret = acpi_bus_trim(device, 0); + printk(KERN_WARNING "acpi_bus_trim stop return %x\n", ret); + if (ret) + return ret; + + /* event originated from ACPI eject notification */ + device->flags.eject_pending = 1; + + /* send the uevent before remove the device */ + kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE); + + ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL); + if (!ej_event) + return -ENOMEM; + + ej_event->device = device; + ej_event->event = ACPI_NOTIFY_EJECT_REQUEST; + + acpi_bus_hot_remove_device(ej_event); + + return 0; +} + static void __container_notify_cb(struct work_struct *work) { struct acpi_device *device = NULL; @@ -181,6 +210,9 @@ static void __container_notify_cb(struct work_struct *work) handle = hp_work->handle; type = hp_work->type; + present = is_device_present(handle); + status = acpi_bus_get_device(handle, &device); + switch (type) { case ACPI_NOTIFY_BUS_CHECK: /* Fall through */ @@ -189,13 +221,14 @@ static void __container_notify_cb(struct work_struct *work) (type == ACPI_NOTIFY_BUS_CHECK) ? "ACPI_NOTIFY_BUS_CHECK" : "ACPI_NOTIFY_DEVICE_CHECK"); - present = is_device_present(handle); - status = acpi_bus_get_device(handle, &device); if (!present) { if (ACPI_SUCCESS(status)) { /* device exist and this is a remove request */ - device->flags.eject_pending = 1; - kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE); + result = container_device_remove(device); + if (result) { + printk(KERN_WARNING "Failed to remove container\n"); + break; + } return; } break; @@ -215,12 +248,19 @@ static void __container_notify_cb(struct work_struct *work) break; case ACPI_NOTIFY_EJECT_REQUEST: - if (!acpi_bus_get_device(handle, &device) && device) { - device->flags.eject_pending = 1; - kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE); - return; + printk(KERN_WARNING "Container driver received %s event\n", + "ACPI_NOTIFY_EJECT_REQUEST"); + + if (!present || ACPI_FAILURE(status) || !device) + break; + + result = container_device_remove(device); + if (result) { + printk(KERN_WARNING "Failed to remove container\n"); + break; } - break; + + return; default: /* non-hotplug event; possibly handled by other handler */
This patch introduces a new function container_device_remove() to do the container hot-remove job. It works like the following: 1. call acpi_bus_trim(device, 0) to stop the container device. 2. generate the KOBJ_OFFLINE uevent. (Did I do this correct ?) 3. call acpi_bus_hot_remove_device(), which will call acpi_bus_trim(device, 1) to remove the container. This patch is based on Lu Yinghai's work. git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-split-pci-root-hp-2 Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com> --- drivers/acpi/container.c | 58 ++++++++++++++++++++++++++++++++++++++------- 1 files changed, 49 insertions(+), 9 deletions(-)