[PATCH 0/3] ACPI hotplug fixes for 3.13

[PATCH 0/3] ACPI hotplug fixes for 3.13

[Rafael J. Wysocki]

Hi,

The following three patches are fixes on top of my 3.13-rc1 pull request that
haven't been pulled yet.

Thanks,
Rafael

[PATCH 1/3] ACPI / hotplug: Fix acpi_bus_get_device() return value check

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since acpi_bus_get_device() returns plain int and not acpi_status,
ACPI_FAILURE() should not be used for checking its return value.  Fix
that.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -411,8 +411,7 @@ static void acpi_hotplug_notify_cb(acpi_
 		break;
 	case ACPI_NOTIFY_EJECT_REQUEST:
 		acpi_handle_debug(handle, "ACPI_NOTIFY_EJECT_REQUEST event\n");
-		status = acpi_bus_get_device(handle, &adev);
-		if (ACPI_FAILURE(status))
+		if (acpi_bus_get_device(handle, &adev))
 			goto err_out;
 
 		get_device(&adev->dev);

Re: [PATCH 1/3] ACPI / hotplug: Fix acpi_bus_get_device() return value check

[Toshi Kani]

On Thu, 2013-11-14 at 00:15 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since acpi_bus_get_device() returns plain int and not acpi_status,
> ACPI_FAILURE() should not be used for checking its return value.  Fix
> that.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Toshi Kani <toshi.kani@hp.com>

Thanks,
-Toshi

[PATCH 2/3] ACPI / hotplug: Fix PCI host bridge hot removal

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since the PCI host bridge scan handler does not set hotplug.enabled,
the check of it in acpi_bus_device_eject() effectively prevents the
root bridge hot removal from working after commit a3b1b1ef78cd
(ACPI / hotplug: Merge device hot-removal routines).  However, that
check is not necessary, because the other acpi_bus_device_eject()
users, acpi_hotplug_notify_cb and acpi_eject_store(), do the same
check by themselves before executing that function.

For this reason, remove the scan handler check from
acpi_bus_device_eject() to make PCI hot bridge hot removal work
again.

Fixes: a3b1b1ef78cd (ACPI / hotplug: Merge device hot-removal routines)
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -289,24 +289,17 @@ void acpi_bus_device_eject(void *data, u
 {
 	struct acpi_device *device = data;
 	acpi_handle handle = device->handle;
-	struct acpi_scan_handler *handler;
 	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE;
 	int error;
 
 	lock_device_hotplug();
 	mutex_lock(&acpi_scan_lock);
 
-	handler = device->handler;
-	if (!handler || !handler->hotplug.enabled) {
-		put_device(&device->dev);
-		goto err_support;
-	}
-
 	if (ost_src == ACPI_NOTIFY_EJECT_REQUEST)
 		acpi_evaluate_hotplug_ost(handle, ACPI_NOTIFY_EJECT_REQUEST,
 					  ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
 
-	if (handler->hotplug.mode == AHM_CONTAINER)
+	if (device->handler && device->handler->hotplug.mode == AHM_CONTAINER)
 		kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
 
 	error = acpi_scan_hot_remove(device);

Re: [PATCH 2/3] ACPI / hotplug: Fix PCI host bridge hot removal

[Toshi Kani]

On Thu, 2013-11-14 at 00:16 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since the PCI host bridge scan handler does not set hotplug.enabled,
> the check of it in acpi_bus_device_eject() effectively prevents the
> root bridge hot removal from working after commit a3b1b1ef78cd
> (ACPI / hotplug: Merge device hot-removal routines).  However, that
> check is not necessary, because the other acpi_bus_device_eject()
> users, acpi_hotplug_notify_cb and acpi_eject_store(), do the same
> check by themselves before executing that function.
> 
> For this reason, remove the scan handler check from
> acpi_bus_device_eject() to make PCI hot bridge hot removal work
> again.

I am curious why the PCI host bridge scan handler does not set
hotplug.enabled.  Is this how it disables hotplug via sysfs eject but
enables via ACPI notification?  

Thanks,
-Toshi

Re: [PATCH 2/3] ACPI / hotplug: Fix PCI host bridge hot removal

[Rafael J. Wysocki]

On Monday, November 18, 2013 11:10:05 AM Toshi Kani wrote:
> On Thu, 2013-11-14 at 00:16 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Since the PCI host bridge scan handler does not set hotplug.enabled,
> > the check of it in acpi_bus_device_eject() effectively prevents the
> > root bridge hot removal from working after commit a3b1b1ef78cd
> > (ACPI / hotplug: Merge device hot-removal routines).  However, that
> > check is not necessary, because the other acpi_bus_device_eject()
> > users, acpi_hotplug_notify_cb and acpi_eject_store(), do the same
> > check by themselves before executing that function.
> > 
> > For this reason, remove the scan handler check from
> > acpi_bus_device_eject() to make PCI hot bridge hot removal work
> > again.
> 
> I am curious why the PCI host bridge scan handler does not set
> hotplug.enabled.  Is this how it disables hotplug via sysfs eject but
> enables via ACPI notification? 

It just doesn't register for hotplug at all.  I guess it could set that
bit alone, but then it would be quite confusing and the check is not
necessary anyway.

Thanks,
Rafael

Re: [PATCH 2/3] ACPI / hotplug: Fix PCI host bridge hot removal

[Toshi Kani]

On Mon, 2013-11-18 at 22:39 +0100, Rafael J. Wysocki wrote:
> On Monday, November 18, 2013 11:10:05 AM Toshi Kani wrote:
> > On Thu, 2013-11-14 at 00:16 +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > Since the PCI host bridge scan handler does not set hotplug.enabled,
> > > the check of it in acpi_bus_device_eject() effectively prevents the
> > > root bridge hot removal from working after commit a3b1b1ef78cd
> > > (ACPI / hotplug: Merge device hot-removal routines).  However, that
> > > check is not necessary, because the other acpi_bus_device_eject()
> > > users, acpi_hotplug_notify_cb and acpi_eject_store(), do the same
> > > check by themselves before executing that function.
> > > 
> > > For this reason, remove the scan handler check from
> > > acpi_bus_device_eject() to make PCI hot bridge hot removal work
> > > again.
> > 
> > I am curious why the PCI host bridge scan handler does not set
> > hotplug.enabled.  Is this how it disables hotplug via sysfs eject but
> > enables via ACPI notification? 
> 
> It just doesn't register for hotplug at all.  I guess it could set that
> bit alone, but then it would be quite confusing and the check is not
> necessary anyway.

I see.  Given how the PCI host bridge scan handler is integrated today,
the change looks reasonable to me.

Acked-by: Toshi Kani <toshi.kani@hp.com>

Thanks,
-Toshi

Re: [PATCH 2/3] ACPI / hotplug: Fix PCI host bridge hot removal

[Toshi Kani]

On Mon, 2013-11-18 at 16:13 -0700, Toshi Kani wrote:
> On Mon, 2013-11-18 at 22:39 +0100, Rafael J. Wysocki wrote:
> > On Monday, November 18, 2013 11:10:05 AM Toshi Kani wrote:
> > > On Thu, 2013-11-14 at 00:16 +0100, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > 
> > > > Since the PCI host bridge scan handler does not set hotplug.enabled,
> > > > the check of it in acpi_bus_device_eject() effectively prevents the
> > > > root bridge hot removal from working after commit a3b1b1ef78cd
> > > > (ACPI / hotplug: Merge device hot-removal routines).  However, that
> > > > check is not necessary, because the other acpi_bus_device_eject()
> > > > users, acpi_hotplug_notify_cb and acpi_eject_store(), do the same
> > > > check by themselves before executing that function.
> > > > 
> > > > For this reason, remove the scan handler check from
> > > > acpi_bus_device_eject() to make PCI hot bridge hot removal work
> > > > again.
> > > 
> > > I am curious why the PCI host bridge scan handler does not set
> > > hotplug.enabled.  Is this how it disables hotplug via sysfs eject but
> > > enables via ACPI notification? 
> > 
> > It just doesn't register for hotplug at all.  I guess it could set that
> > bit alone, but then it would be quite confusing and the check is not
> > necessary anyway.
> 
> I see.  Given how the PCI host bridge scan handler is integrated today,
> the change looks reasonable to me.

Looking further, I noticed that there is one more issue to address.  The
patch below applies on top of your patchset.

Thanks,
-Toshi

====
From: Toshi Kani <toshi.kani@hp.com>
Subject: [PATCH] ACPI / hotplug: Fix conflicted PCI bridge notify
handlers

The PCI host bridge scan handler installs its own notify handler,
handle_hotplug_event_root(), by itself.  Nevertheless, the ACPI
hotplug framework also installs the common notify handler,
acpi_hotplug_notify_cb(), for PCI root bridges.  This causes
acpi_hotplug_notify_cb() to call _OST method with unsupported
error as hotplug.enabled is not set.

To address this issue, introduce hotplug.self_install flag, which
indicates that the scan handler installs its own notify handler by
itself.  The ACPI hotplug framework does not install the common
notify handler when this flag is set.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 drivers/acpi/pci_root.c |    3 +++
 drivers/acpi/scan.c     |    2 +-
 include/acpi/acpi_bus.h |    1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 0703bff..ab52541 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -65,6 +65,9 @@ static struct acpi_scan_handler pci_root_handler = {
 	.ids = root_device_ids,
 	.attach = acpi_pci_root_add,
 	.detach = acpi_pci_root_remove,
+	.hotplug = {
+		.self_install = true,
+	},
 };
 
 static DEFINE_MUTEX(osc_lock);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 337109d..ec95a19 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1772,7 +1772,7 @@ static void acpi_scan_init_hotplug(acpi_handle
handle, int type)
 	 */
 	list_for_each_entry(hwid, &pnp.ids, list) {
 		handler = acpi_scan_match_handler(hwid->id, NULL);
-		if (handler) {
+		if (handler && !handler->hotplug.self_install) {
 			acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
 					acpi_hotplug_notify_cb, handler);
 			break;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 89c60b0..87c918e 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -100,6 +100,7 @@ enum acpi_hotplug_mode {
 struct acpi_hotplug_profile {
 	struct kobject kobj;
 	bool enabled:1;
+	bool self_install:1;
 	enum acpi_hotplug_mode mode;
 };
 

Re: [PATCH 2/3] ACPI / hotplug: Fix PCI host bridge hot removal

[Rafael J. Wysocki]

> On Tuesday, November 19, 2013 10:48:51 AM Toshi Kani wrote:
> > On Mon, 2013-11-18 at 22:39 +0100, Rafael J. Wysocki wrote:
> > > On Monday, November 18, 2013 11:10:05 AM Toshi Kani wrote:
> > > > On Thu, 2013-11-14 at 00:16 +0100, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > 
> > > > > Since the PCI host bridge scan handler does not set hotplug.enabled,
> > > > > the check of it in acpi_bus_device_eject() effectively prevents the
> > > > > root bridge hot removal from working after commit a3b1b1ef78cd
> > > > > (ACPI / hotplug: Merge device hot-removal routines).  However, that
> > > > > check is not necessary, because the other acpi_bus_device_eject()
> > > > > users, acpi_hotplug_notify_cb and acpi_eject_store(), do the same
> > > > > check by themselves before executing that function.
> > > > > 
> > > > > For this reason, remove the scan handler check from
> > > > > acpi_bus_device_eject() to make PCI hot bridge hot removal work
> > > > > again.
> > > > 
> > > > I am curious why the PCI host bridge scan handler does not set
> > > > hotplug.enabled.  Is this how it disables hotplug via sysfs eject but
> > > > enables via ACPI notification?
> > > 
> > > It just doesn't register for hotplug at all.  I guess it could set that
> > > bit alone, but then it would be quite confusing and the check is not
> > > necessary anyway.
> > 
> > I see.  Given how the PCI host bridge scan handler is integrated today,
> > the change looks reasonable to me.
> 
> Looking further, I noticed that there is one more issue to address.  The
> patch below applies on top of your patchset.
>
> From: Toshi Kani <toshi.kani@hp.com>
> Subject: [PATCH] ACPI / hotplug: Fix conflicted PCI bridge notify
> handlers
> 
> The PCI host bridge scan handler installs its own notify handler,
> handle_hotplug_event_root(), by itself.  Nevertheless, the ACPI
> hotplug framework also installs the common notify handler,
> acpi_hotplug_notify_cb(), for PCI root bridges.  This causes
> acpi_hotplug_notify_cb() to call _OST method with unsupported
> error as hotplug.enabled is not set.
> 
> To address this issue, introduce hotplug.self_install flag, which
> indicates that the scan handler installs its own notify handler by
> itself.  The ACPI hotplug framework does not install the common
> notify handler when this flag is set.

Good catch!

Still, I don't think we need a new flag, because we know that that
scan handler doesn't support hotplug, because its hotplug profile
hasn't been registered (that actually applies to all scan handlers
without hotplug support, not only the root host bridge).  Moreover,
if it does support hotplug, but the hotplug profile hasn't been
registered due to an error, we still should not install the notify
handler I think.  So, I prefer the patch below.

And this fix will remain useful after my recent series at
http://marc.info/?l=linux-pci&m=138470560909690&w=4

Thanks,
Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: ACPI / hotplug: Ignore notifications for scan handlers without hotplug

If a scan handler's hotplug profile has not been registered, do not
install acpi_hotplug_notify_cb() as a notify handler for devices
with the given scan handler attached.

This fixes a problem with the PCI host bridge scan handler that
installs its own notify handler, handle_hotplug_event_root(), by
itself and doesn't register its hotplug profile.  Toshi says

"Nevertheless, the ACPI hotplug framework also installs the common
 notify handler, acpi_hotplug_notify_cb(), for PCI root bridges.
 This causes acpi_hotplug_notify_cb() to call _OST method with
 unsupported error as hotplug.enabled is not set."

Moreover, it is pointless to install acpi_hotplug_notify_cb() as
a notify handler for device objects whose scan handlers don't
support hotplug, because it will always fail for them.

Reported-by: Toshi Kani <toshi.kani@hp.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: 3.9+ <stable@vger.kernel.org> # 3.9+
---
 drivers/acpi/scan.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -84,6 +84,11 @@ int acpi_scan_add_handler_with_hotplug(s
 	return 0;
 }
 
+static bool acpi_scan_handler_with_hotplug(struct acpi_scan_handler *handler)
+{
+	return handler && handler->hotplug.kobj.state_in_sysfs;
+}
+
 /*
  * Creates hid/cid(s) string needed for modalias and uevent
  * e.g. on a device with hid:IBM0001 and cid:ACPI0001 you get:
@@ -1841,7 +1846,7 @@ static void acpi_scan_init_hotplug(acpi_
 	 */
 	list_for_each_entry(hwid, &pnp.ids, list) {
 		handler = acpi_scan_match_handler(hwid->id, NULL);
-		if (handler) {
+		if (acpi_scan_handler_with_hotplug(handler)) {
 			acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
 					acpi_hotplug_notify_cb, handler);
 			break;

Re: [PATCH 2/3] ACPI / hotplug: Fix PCI host bridge hot removal

[Toshi Kani]

On Tue, 2013-11-19 at 22:10 +0100, Rafael J. Wysocki wrote:
> > On Tuesday, November 19, 2013 10:48:51 AM Toshi Kani wrote:
> > > On Mon, 2013-11-18 at 22:39 +0100, Rafael J. Wysocki wrote:
> > > > On Monday, November 18, 2013 11:10:05 AM Toshi Kani wrote:
> > > > > On Thu, 2013-11-14 at 00:16 +0100, Rafael J. Wysocki wrote:
> > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > 
> > > > > > Since the PCI host bridge scan handler does not set hotplug.enabled,
> > > > > > the check of it in acpi_bus_device_eject() effectively prevents the
> > > > > > root bridge hot removal from working after commit a3b1b1ef78cd
> > > > > > (ACPI / hotplug: Merge device hot-removal routines).  However, that
> > > > > > check is not necessary, because the other acpi_bus_device_eject()
> > > > > > users, acpi_hotplug_notify_cb and acpi_eject_store(), do the same
> > > > > > check by themselves before executing that function.
> > > > > > 
> > > > > > For this reason, remove the scan handler check from
> > > > > > acpi_bus_device_eject() to make PCI hot bridge hot removal work
> > > > > > again.
> > > > > 
> > > > > I am curious why the PCI host bridge scan handler does not set
> > > > > hotplug.enabled.  Is this how it disables hotplug via sysfs eject but
> > > > > enables via ACPI notification?
> > > > 
> > > > It just doesn't register for hotplug at all.  I guess it could set that
> > > > bit alone, but then it would be quite confusing and the check is not
> > > > necessary anyway.
> > > 
> > > I see.  Given how the PCI host bridge scan handler is integrated today,
> > > the change looks reasonable to me.
> > 
> > Looking further, I noticed that there is one more issue to address.  The
> > patch below applies on top of your patchset.
> >
> > From: Toshi Kani <toshi.kani@hp.com>
> > Subject: [PATCH] ACPI / hotplug: Fix conflicted PCI bridge notify
> > handlers
> > 
> > The PCI host bridge scan handler installs its own notify handler,
> > handle_hotplug_event_root(), by itself.  Nevertheless, the ACPI
> > hotplug framework also installs the common notify handler,
> > acpi_hotplug_notify_cb(), for PCI root bridges.  This causes
> > acpi_hotplug_notify_cb() to call _OST method with unsupported
> > error as hotplug.enabled is not set.
> > 
> > To address this issue, introduce hotplug.self_install flag, which
> > indicates that the scan handler installs its own notify handler by
> > itself.  The ACPI hotplug framework does not install the common
> > notify handler when this flag is set.
> 
> Good catch!
> 
> Still, I don't think we need a new flag, because we know that that
> scan handler doesn't support hotplug, because its hotplug profile
> hasn't been registered (that actually applies to all scan handlers
> without hotplug support, not only the root host bridge).  

When a scan handler does not support hotplug at all, the common notify
handler should be installed so that it can call _OST with an appropriate
response.

> Moreover,
> if it does support hotplug, but the hotplug profile hasn't been
> registered due to an error, we still should not install the notify
> handler I think.  

This case, I think the common notify handler should be installed so that
it can call _OST for error response as well.  The question is what to do
when a scan handler has its own notify handler.

> So, I prefer the patch below.
>
> And this fix will remain useful after my recent series at
> http://marc.info/?l=linux-pci&m=138470560909690&w=4

I agree that the PCI host bridge scan handler should use the common
notify handler, and your change is on the right direction.  But as I
understand, this patchset is for 3.14.  For 3.13, I thought we have to
live with the separate PCI root notify handler, and this patch avoids
the conflict for the time being.  If necessary, the new flag can be
removed in 3.14.

Thanks,
-Toshi

Re: [PATCH 2/3] ACPI / hotplug: Fix PCI host bridge hot removal

[Rafael J. Wysocki]

On Tuesday, November 19, 2013 02:58:40 PM Toshi Kani wrote:
> On Tue, 2013-11-19 at 22:10 +0100, Rafael J. Wysocki wrote:
> > > On Tuesday, November 19, 2013 10:48:51 AM Toshi Kani wrote:
> > > > On Mon, 2013-11-18 at 22:39 +0100, Rafael J. Wysocki wrote:
> > > > > On Monday, November 18, 2013 11:10:05 AM Toshi Kani wrote:
> > > > > > On Thu, 2013-11-14 at 00:16 +0100, Rafael J. Wysocki wrote:
> > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > 
> > > > > > > Since the PCI host bridge scan handler does not set hotplug.enabled,
> > > > > > > the check of it in acpi_bus_device_eject() effectively prevents the
> > > > > > > root bridge hot removal from working after commit a3b1b1ef78cd
> > > > > > > (ACPI / hotplug: Merge device hot-removal routines).  However, that
> > > > > > > check is not necessary, because the other acpi_bus_device_eject()
> > > > > > > users, acpi_hotplug_notify_cb and acpi_eject_store(), do the same
> > > > > > > check by themselves before executing that function.
> > > > > > > 
> > > > > > > For this reason, remove the scan handler check from
> > > > > > > acpi_bus_device_eject() to make PCI hot bridge hot removal work
> > > > > > > again.
> > > > > > 
> > > > > > I am curious why the PCI host bridge scan handler does not set
> > > > > > hotplug.enabled.  Is this how it disables hotplug via sysfs eject but
> > > > > > enables via ACPI notification?
> > > > > 
> > > > > It just doesn't register for hotplug at all.  I guess it could set that
> > > > > bit alone, but then it would be quite confusing and the check is not
> > > > > necessary anyway.
> > > > 
> > > > I see.  Given how the PCI host bridge scan handler is integrated today,
> > > > the change looks reasonable to me.
> > > 
> > > Looking further, I noticed that there is one more issue to address.  The
> > > patch below applies on top of your patchset.
> > >
> > > From: Toshi Kani <toshi.kani@hp.com>
> > > Subject: [PATCH] ACPI / hotplug: Fix conflicted PCI bridge notify
> > > handlers
> > > 
> > > The PCI host bridge scan handler installs its own notify handler,
> > > handle_hotplug_event_root(), by itself.  Nevertheless, the ACPI
> > > hotplug framework also installs the common notify handler,
> > > acpi_hotplug_notify_cb(), for PCI root bridges.  This causes
> > > acpi_hotplug_notify_cb() to call _OST method with unsupported
> > > error as hotplug.enabled is not set.
> > > 
> > > To address this issue, introduce hotplug.self_install flag, which
> > > indicates that the scan handler installs its own notify handler by
> > > itself.  The ACPI hotplug framework does not install the common
> > > notify handler when this flag is set.
> > 
> > Good catch!
> > 
> > Still, I don't think we need a new flag, because we know that that
> > scan handler doesn't support hotplug, because its hotplug profile
> > hasn't been registered (that actually applies to all scan handlers
> > without hotplug support, not only the root host bridge).  
> 
> When a scan handler does not support hotplug at all, the common notify
> handler should be installed so that it can call _OST with an appropriate
> response.

That creates an arbitrary difference between devices that have scan handlers
and devices that don't have them (PCI, USB, SATA etc).  So if we want _OST
to be called for all devices for which hotplug is not supported, that
should be implemented in a different way and not necessarily in 3.13.

> > Moreover,
> > if it does support hotplug, but the hotplug profile hasn't been
> > registered due to an error, we still should not install the notify
> > handler I think.  
> 
> This case, I think the common notify handler should be installed so that
> it can call _OST for error response as well.  The question is what to do
> when a scan handler has its own notify handler.

Well, to be precise, in the case of the PCI host bridge the notify handler is
installed via a separate init routine that doesn't belong to the scan handler.

Semantics aside, my original intention was not to install notify handlers for
all devices with scan handlers, but only for those whose scan handlers support
hotplug, because the default for all devices without hotplug support is to
avoid installing any notify handlers at all.

> > So, I prefer the patch below.
> >
> > And this fix will remain useful after my recent series at
> > http://marc.info/?l=linux-pci&m=138470560909690&w=4
> 
> I agree that the PCI host bridge scan handler should use the common
> notify handler, and your change is on the right direction.  But as I
> understand, this patchset is for 3.14.  For 3.13, I thought we have to
> live with the separate PCI root notify handler, and this patch avoids
> the conflict for the time being.  If necessary, the new flag can be
> removed in 3.14.

Well, it could, but then the current behavior is not intentional as I
said and obviously buggy for PCI host bridges.  So in my opinion we
should bring things back to the way they were *intended* to behave in the
first place.  If that causes problems to happen for someone, we can still
special case the PCI host bridge, but I'd prefer to avoid that if possible.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/3] ACPI / hotplug: Fix PCI host bridge hot removal

[Rafael J. Wysocki]

On Wednesday, November 20, 2013 12:42:28 AM Rafael J. Wysocki wrote:
> On Tuesday, November 19, 2013 02:58:40 PM Toshi Kani wrote:
> > On Tue, 2013-11-19 at 22:10 +0100, Rafael J. Wysocki wrote:
> > > > On Tuesday, November 19, 2013 10:48:51 AM Toshi Kani wrote:
> > > > > On Mon, 2013-11-18 at 22:39 +0100, Rafael J. Wysocki wrote:
> > > > > > On Monday, November 18, 2013 11:10:05 AM Toshi Kani wrote:
> > > > > > > On Thu, 2013-11-14 at 00:16 +0100, Rafael J. Wysocki wrote:
> > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > > 
> > > > > > > > Since the PCI host bridge scan handler does not set hotplug.enabled,
> > > > > > > > the check of it in acpi_bus_device_eject() effectively prevents the
> > > > > > > > root bridge hot removal from working after commit a3b1b1ef78cd
> > > > > > > > (ACPI / hotplug: Merge device hot-removal routines).  However, that
> > > > > > > > check is not necessary, because the other acpi_bus_device_eject()
> > > > > > > > users, acpi_hotplug_notify_cb and acpi_eject_store(), do the same
> > > > > > > > check by themselves before executing that function.
> > > > > > > > 
> > > > > > > > For this reason, remove the scan handler check from
> > > > > > > > acpi_bus_device_eject() to make PCI hot bridge hot removal work
> > > > > > > > again.
> > > > > > > 
> > > > > > > I am curious why the PCI host bridge scan handler does not set
> > > > > > > hotplug.enabled.  Is this how it disables hotplug via sysfs eject but
> > > > > > > enables via ACPI notification?
> > > > > > 
> > > > > > It just doesn't register for hotplug at all.  I guess it could set that
> > > > > > bit alone, but then it would be quite confusing and the check is not
> > > > > > necessary anyway.
> > > > > 
> > > > > I see.  Given how the PCI host bridge scan handler is integrated today,
> > > > > the change looks reasonable to me.
> > > > 
> > > > Looking further, I noticed that there is one more issue to address.  The
> > > > patch below applies on top of your patchset.
> > > >
> > > > From: Toshi Kani <toshi.kani@hp.com>
> > > > Subject: [PATCH] ACPI / hotplug: Fix conflicted PCI bridge notify
> > > > handlers
> > > > 
> > > > The PCI host bridge scan handler installs its own notify handler,
> > > > handle_hotplug_event_root(), by itself.  Nevertheless, the ACPI
> > > > hotplug framework also installs the common notify handler,
> > > > acpi_hotplug_notify_cb(), for PCI root bridges.  This causes
> > > > acpi_hotplug_notify_cb() to call _OST method with unsupported
> > > > error as hotplug.enabled is not set.
> > > > 
> > > > To address this issue, introduce hotplug.self_install flag, which
> > > > indicates that the scan handler installs its own notify handler by
> > > > itself.  The ACPI hotplug framework does not install the common
> > > > notify handler when this flag is set.
> > > 
> > > Good catch!
> > > 
> > > Still, I don't think we need a new flag, because we know that that
> > > scan handler doesn't support hotplug, because its hotplug profile
> > > hasn't been registered (that actually applies to all scan handlers
> > > without hotplug support, not only the root host bridge).  
> > 
> > When a scan handler does not support hotplug at all, the common notify
> > handler should be installed so that it can call _OST with an appropriate
> > response.
> 
> That creates an arbitrary difference between devices that have scan handlers
> and devices that don't have them (PCI, USB, SATA etc).  So if we want _OST
> to be called for all devices for which hotplug is not supported, that
> should be implemented in a different way and not necessarily in 3.13.
> 
> > > Moreover,
> > > if it does support hotplug, but the hotplug profile hasn't been
> > > registered due to an error, we still should not install the notify
> > > handler I think.  
> > 
> > This case, I think the common notify handler should be installed so that
> > it can call _OST for error response as well.  The question is what to do
> > when a scan handler has its own notify handler.

Actually, having considered this particular case a bit more I think that it
is useful to install acpi_hotplug_notify_cb() for things whose scan handlers
register hotplug support, but the registration fails (which should be treated
as "permanently disabled").

However, I still think that devices whose scan handlers don't support hotplug
at all should be treated consistently with devices without scan handlers.

So, what about the slightly modified patch below?

Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: ACPI / hotplug: Ignore notifications for scan handlers without hotplug

If a scan handler's hotplug profile has not been initialized, do not
install acpi_hotplug_notify_cb() as a notify handler for devices
with the given scan handler attached.

This fixes a problem with the PCI host bridge hotplug which installs
its own notify handler, handle_hotplug_event_root(), by itself and
doesn't register its scan handler's hotplug profile.  Toshi says:

"Nevertheless, the ACPI hotplug framework also installs the common
 notify handler, acpi_hotplug_notify_cb(), for PCI root bridges.
 This causes acpi_hotplug_notify_cb() to call _OST method with
 unsupported error as hotplug.enabled is not set."

Moreover, the default behavior for all devices that don't have any
scan handlers attached is not to install notify handlers for them
and there is no difference between this case and the case when
a device's scan handler doesn't support hotplug.

Reported-by: Toshi Kani <toshi.kani@hp.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: 3.9+ <stable@vger.kernel.org> # 3.9+
---
 drivers/acpi/scan.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -84,6 +84,11 @@ int acpi_scan_add_handler_with_hotplug(s
 	return 0;
 }
 
+static bool acpi_scan_handler_with_hotplug(struct acpi_scan_handler *handler)
+{
+	return handler && handler->hotplug.kobj.state_initialized;
+}
+
 /*
  * Creates hid/cid(s) string needed for modalias and uevent
  * e.g. on a device with hid:IBM0001 and cid:ACPI0001 you get:
@@ -1841,7 +1846,7 @@ static void acpi_scan_init_hotplug(acpi_
 	 */
 	list_for_each_entry(hwid, &pnp.ids, list) {
 		handler = acpi_scan_match_handler(hwid->id, NULL);
-		if (handler) {
+		if (acpi_scan_handler_with_hotplug(handler)) {
 			acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
 					acpi_hotplug_notify_cb, handler);
 			break;

Re: [PATCH 2/3] ACPI / hotplug: Fix PCI host bridge hot removal

[Toshi Kani]

On Wed, 2013-11-20 at 01:08 +0100, Rafael J. Wysocki wrote:
> On Wednesday, November 20, 2013 12:42:28 AM Rafael J. Wysocki wrote:
> > On Tuesday, November 19, 2013 02:58:40 PM Toshi Kani wrote:
> > > On Tue, 2013-11-19 at 22:10 +0100, Rafael J. Wysocki wrote:
> > > > > On Tuesday, November 19, 2013 10:48:51 AM Toshi Kani wrote:
> > > > > > On Mon, 2013-11-18 at 22:39 +0100, Rafael J. Wysocki wrote:
> > > > > > > On Monday, November 18, 2013 11:10:05 AM Toshi Kani wrote:
> > > > > > > > On Thu, 2013-11-14 at 00:16 +0100, Rafael J. Wysocki wrote:
> > > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > > > 
> > > > > > > > > Since the PCI host bridge scan handler does not set hotplug.enabled,
> > > > > > > > > the check of it in acpi_bus_device_eject() effectively prevents the
> > > > > > > > > root bridge hot removal from working after commit a3b1b1ef78cd
> > > > > > > > > (ACPI / hotplug: Merge device hot-removal routines).  However, that
> > > > > > > > > check is not necessary, because the other acpi_bus_device_eject()
> > > > > > > > > users, acpi_hotplug_notify_cb and acpi_eject_store(), do the same
> > > > > > > > > check by themselves before executing that function.
> > > > > > > > > 
> > > > > > > > > For this reason, remove the scan handler check from
> > > > > > > > > acpi_bus_device_eject() to make PCI hot bridge hot removal work
> > > > > > > > > again.
> > > > > > > > 
> > > > > > > > I am curious why the PCI host bridge scan handler does not set
> > > > > > > > hotplug.enabled.  Is this how it disables hotplug via sysfs eject but
> > > > > > > > enables via ACPI notification?
> > > > > > > 
> > > > > > > It just doesn't register for hotplug at all.  I guess it could set that
> > > > > > > bit alone, but then it would be quite confusing and the check is not
> > > > > > > necessary anyway.
> > > > > > 
> > > > > > I see.  Given how the PCI host bridge scan handler is integrated today,
> > > > > > the change looks reasonable to me.
> > > > > 
> > > > > Looking further, I noticed that there is one more issue to address.  The
> > > > > patch below applies on top of your patchset.
> > > > >
> > > > > From: Toshi Kani <toshi.kani@hp.com>
> > > > > Subject: [PATCH] ACPI / hotplug: Fix conflicted PCI bridge notify
> > > > > handlers
> > > > > 
> > > > > The PCI host bridge scan handler installs its own notify handler,
> > > > > handle_hotplug_event_root(), by itself.  Nevertheless, the ACPI
> > > > > hotplug framework also installs the common notify handler,
> > > > > acpi_hotplug_notify_cb(), for PCI root bridges.  This causes
> > > > > acpi_hotplug_notify_cb() to call _OST method with unsupported
> > > > > error as hotplug.enabled is not set.
> > > > > 
> > > > > To address this issue, introduce hotplug.self_install flag, which
> > > > > indicates that the scan handler installs its own notify handler by
> > > > > itself.  The ACPI hotplug framework does not install the common
> > > > > notify handler when this flag is set.
> > > > 
> > > > Good catch!
> > > > 
> > > > Still, I don't think we need a new flag, because we know that that
> > > > scan handler doesn't support hotplug, because its hotplug profile
> > > > hasn't been registered (that actually applies to all scan handlers
> > > > without hotplug support, not only the root host bridge).  
> > > 
> > > When a scan handler does not support hotplug at all, the common notify
> > > handler should be installed so that it can call _OST with an appropriate
> > > response.
> > 
> > That creates an arbitrary difference between devices that have scan handlers
> > and devices that don't have them (PCI, USB, SATA etc).  So if we want _OST
> > to be called for all devices for which hotplug is not supported, that
> > should be implemented in a different way and not necessarily in 3.13.

I do not think we have an immediate issue since it only matters when the
firmware supports ACPI hotplug with _OST.  Such device types are CPU,
memory, container, and PCI bridge, which uses scan handlers.  USB, SATA,
etc. do not use ACPI hotplug.  That said, ideally, we should be able to
call _OST for any device types.
 
> > > > Moreover,
> > > > if it does support hotplug, but the hotplug profile hasn't been
> > > > registered due to an error, we still should not install the notify
> > > > handler I think.  
> > > 
> > > This case, I think the common notify handler should be installed so that
> > > it can call _OST for error response as well.  The question is what to do
> > > when a scan handler has its own notify handler.
> 
> Actually, having considered this particular case a bit more I think that it
> is useful to install acpi_hotplug_notify_cb() for things whose scan handlers
> register hotplug support, but the registration fails (which should be treated
> as "permanently disabled").
>
> However, I still think that devices whose scan handlers don't support hotplug
> at all should be treated consistently with devices without scan handlers.

Basically, the kernel needs to be compliant with ACPI spec.  Once the
kernel tells firmware that it supports _OST, it needs to call _OST for
an ACPI hotplug event when _OST is implemented on the object.  Although
we cannot assure this behavior for the devices without scan handler, we
should do so for the devices with scan handlers.

> So, what about the slightly modified patch below?

Well, I do not think it is right.  The kernel is supposed to tell
firmware that it does not support hotplug when it doesn't... 

Thanks,
-Toshi

Re: [PATCH 2/3] ACPI / hotplug: Fix PCI host bridge hot removal

[Rafael J. Wysocki]

On Tuesday, November 19, 2013 06:22:07 PM Toshi Kani wrote:
> On Wed, 2013-11-20 at 01:08 +0100, Rafael J. Wysocki wrote:
> > On Wednesday, November 20, 2013 12:42:28 AM Rafael J. Wysocki wrote:
> > > On Tuesday, November 19, 2013 02:58:40 PM Toshi Kani wrote:
> > > > On Tue, 2013-11-19 at 22:10 +0100, Rafael J. Wysocki wrote:
> > > > > > On Tuesday, November 19, 2013 10:48:51 AM Toshi Kani wrote:
> > > > > > > On Mon, 2013-11-18 at 22:39 +0100, Rafael J. Wysocki wrote:
> > > > > > > > On Monday, November 18, 2013 11:10:05 AM Toshi Kani wrote:
> > > > > > > > > On Thu, 2013-11-14 at 00:16 +0100, Rafael J. Wysocki wrote:
> > > > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > > > > 
> > > > > > > > > > Since the PCI host bridge scan handler does not set hotplug.enabled,
> > > > > > > > > > the check of it in acpi_bus_device_eject() effectively prevents the
> > > > > > > > > > root bridge hot removal from working after commit a3b1b1ef78cd
> > > > > > > > > > (ACPI / hotplug: Merge device hot-removal routines).  However, that
> > > > > > > > > > check is not necessary, because the other acpi_bus_device_eject()
> > > > > > > > > > users, acpi_hotplug_notify_cb and acpi_eject_store(), do the same
> > > > > > > > > > check by themselves before executing that function.
> > > > > > > > > > 
> > > > > > > > > > For this reason, remove the scan handler check from
> > > > > > > > > > acpi_bus_device_eject() to make PCI hot bridge hot removal work
> > > > > > > > > > again.
> > > > > > > > > 
> > > > > > > > > I am curious why the PCI host bridge scan handler does not set
> > > > > > > > > hotplug.enabled.  Is this how it disables hotplug via sysfs eject but
> > > > > > > > > enables via ACPI notification?
> > > > > > > > 
> > > > > > > > It just doesn't register for hotplug at all.  I guess it could set that
> > > > > > > > bit alone, but then it would be quite confusing and the check is not
> > > > > > > > necessary anyway.
> > > > > > > 
> > > > > > > I see.  Given how the PCI host bridge scan handler is integrated today,
> > > > > > > the change looks reasonable to me.
> > > > > > 
> > > > > > Looking further, I noticed that there is one more issue to address.  The
> > > > > > patch below applies on top of your patchset.
> > > > > >
> > > > > > From: Toshi Kani <toshi.kani@hp.com>
> > > > > > Subject: [PATCH] ACPI / hotplug: Fix conflicted PCI bridge notify
> > > > > > handlers
> > > > > > 
> > > > > > The PCI host bridge scan handler installs its own notify handler,
> > > > > > handle_hotplug_event_root(), by itself.  Nevertheless, the ACPI
> > > > > > hotplug framework also installs the common notify handler,
> > > > > > acpi_hotplug_notify_cb(), for PCI root bridges.  This causes
> > > > > > acpi_hotplug_notify_cb() to call _OST method with unsupported
> > > > > > error as hotplug.enabled is not set.
> > > > > > 
> > > > > > To address this issue, introduce hotplug.self_install flag, which
> > > > > > indicates that the scan handler installs its own notify handler by
> > > > > > itself.  The ACPI hotplug framework does not install the common
> > > > > > notify handler when this flag is set.
> > > > > 
> > > > > Good catch!
> > > > > 
> > > > > Still, I don't think we need a new flag, because we know that that
> > > > > scan handler doesn't support hotplug, because its hotplug profile
> > > > > hasn't been registered (that actually applies to all scan handlers
> > > > > without hotplug support, not only the root host bridge).  
> > > > 
> > > > When a scan handler does not support hotplug at all, the common notify
> > > > handler should be installed so that it can call _OST with an appropriate
> > > > response.
> > > 
> > > That creates an arbitrary difference between devices that have scan handlers
> > > and devices that don't have them (PCI, USB, SATA etc).  So if we want _OST
> > > to be called for all devices for which hotplug is not supported, that
> > > should be implemented in a different way and not necessarily in 3.13.
> 
> I do not think we have an immediate issue since it only matters when the
> firmware supports ACPI hotplug with _OST.  Such device types are CPU,
> memory, container, and PCI bridge, which uses scan handlers.  USB, SATA,
> etc. do not use ACPI hotplug.  That said, ideally, we should be able to
> call _OST for any device types.
>  
> > > > > Moreover,
> > > > > if it does support hotplug, but the hotplug profile hasn't been
> > > > > registered due to an error, we still should not install the notify
> > > > > handler I think.  
> > > > 
> > > > This case, I think the common notify handler should be installed so that
> > > > it can call _OST for error response as well.  The question is what to do
> > > > when a scan handler has its own notify handler.
> > 
> > Actually, having considered this particular case a bit more I think that it
> > is useful to install acpi_hotplug_notify_cb() for things whose scan handlers
> > register hotplug support, but the registration fails (which should be treated
> > as "permanently disabled").
> >
> > However, I still think that devices whose scan handlers don't support hotplug
> > at all should be treated consistently with devices without scan handlers.
> 
> Basically, the kernel needs to be compliant with ACPI spec.  Once the
> kernel tells firmware that it supports _OST, it needs to call _OST for
> an ACPI hotplug event when _OST is implemented on the object.  Although
> we cannot assure this behavior for the devices without scan handler, we
> should do so for the devices with scan handlers.
> 
> > So, what about the slightly modified patch below?
> 
> Well, I do not think it is right.  The kernel is supposed to tell
> firmware that it does not support hotplug when it doesn't... 

OK, I see your point.

I'll apply your patch, then, but I'm going to rename the new flag to
"ignore".

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/3] ACPI / hotplug: Fix PCI host bridge hot removal

[Toshi Kani]

On Wed, 2013-11-20 at 12:56 +0100, Rafael J. Wysocki wrote:
> On Tuesday, November 19, 2013 06:22:07 PM Toshi Kani wrote:
> > On Wed, 2013-11-20 at 01:08 +0100, Rafael J. Wysocki wrote:
> > > On Wednesday, November 20, 2013 12:42:28 AM Rafael J. Wysocki wrote:
> > > > On Tuesday, November 19, 2013 02:58:40 PM Toshi Kani wrote:
> > > > > On Tue, 2013-11-19 at 22:10 +0100, Rafael J. Wysocki wrote:
> > > > > > > On Tuesday, November 19, 2013 10:48:51 AM Toshi Kani wrote:
> > > > > > > > On Mon, 2013-11-18 at 22:39 +0100, Rafael J. Wysocki wrote:
> > > > > > > > > On Monday, November 18, 2013 11:10:05 AM Toshi Kani wrote:
> > > > > > > > > > On Thu, 2013-11-14 at 00:16 +0100, Rafael J. Wysocki wrote:
> > > > > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > > > > > 
> > > > > > > > > > > Since the PCI host bridge scan handler does not set hotplug.enabled,
> > > > > > > > > > > the check of it in acpi_bus_device_eject() effectively prevents the
> > > > > > > > > > > root bridge hot removal from working after commit a3b1b1ef78cd
> > > > > > > > > > > (ACPI / hotplug: Merge device hot-removal routines).  However, that
> > > > > > > > > > > check is not necessary, because the other acpi_bus_device_eject()
> > > > > > > > > > > users, acpi_hotplug_notify_cb and acpi_eject_store(), do the same
> > > > > > > > > > > check by themselves before executing that function.
> > > > > > > > > > > 
> > > > > > > > > > > For this reason, remove the scan handler check from
> > > > > > > > > > > acpi_bus_device_eject() to make PCI hot bridge hot removal work
> > > > > > > > > > > again.
> > > > > > > > > > 
> > > > > > > > > > I am curious why the PCI host bridge scan handler does not set
> > > > > > > > > > hotplug.enabled.  Is this how it disables hotplug via sysfs eject but
> > > > > > > > > > enables via ACPI notification?
> > > > > > > > > 
> > > > > > > > > It just doesn't register for hotplug at all.  I guess it could set that
> > > > > > > > > bit alone, but then it would be quite confusing and the check is not
> > > > > > > > > necessary anyway.
> > > > > > > > 
> > > > > > > > I see.  Given how the PCI host bridge scan handler is integrated today,
> > > > > > > > the change looks reasonable to me.
> > > > > > > 
> > > > > > > Looking further, I noticed that there is one more issue to address.  The
> > > > > > > patch below applies on top of your patchset.
> > > > > > >
> > > > > > > From: Toshi Kani <toshi.kani@hp.com>
> > > > > > > Subject: [PATCH] ACPI / hotplug: Fix conflicted PCI bridge notify
> > > > > > > handlers
> > > > > > > 
> > > > > > > The PCI host bridge scan handler installs its own notify handler,
> > > > > > > handle_hotplug_event_root(), by itself.  Nevertheless, the ACPI
> > > > > > > hotplug framework also installs the common notify handler,
> > > > > > > acpi_hotplug_notify_cb(), for PCI root bridges.  This causes
> > > > > > > acpi_hotplug_notify_cb() to call _OST method with unsupported
> > > > > > > error as hotplug.enabled is not set.
> > > > > > > 
> > > > > > > To address this issue, introduce hotplug.self_install flag, which
> > > > > > > indicates that the scan handler installs its own notify handler by
> > > > > > > itself.  The ACPI hotplug framework does not install the common
> > > > > > > notify handler when this flag is set.
> > > > > > 
> > > > > > Good catch!
> > > > > > 
> > > > > > Still, I don't think we need a new flag, because we know that that
> > > > > > scan handler doesn't support hotplug, because its hotplug profile
> > > > > > hasn't been registered (that actually applies to all scan handlers
> > > > > > without hotplug support, not only the root host bridge).  
> > > > > 
> > > > > When a scan handler does not support hotplug at all, the common notify
> > > > > handler should be installed so that it can call _OST with an appropriate
> > > > > response.
> > > > 
> > > > That creates an arbitrary difference between devices that have scan handlers
> > > > and devices that don't have them (PCI, USB, SATA etc).  So if we want _OST
> > > > to be called for all devices for which hotplug is not supported, that
> > > > should be implemented in a different way and not necessarily in 3.13.
> > 
> > I do not think we have an immediate issue since it only matters when the
> > firmware supports ACPI hotplug with _OST.  Such device types are CPU,
> > memory, container, and PCI bridge, which uses scan handlers.  USB, SATA,
> > etc. do not use ACPI hotplug.  That said, ideally, we should be able to
> > call _OST for any device types.
> >  
> > > > > > Moreover,
> > > > > > if it does support hotplug, but the hotplug profile hasn't been
> > > > > > registered due to an error, we still should not install the notify
> > > > > > handler I think.  
> > > > > 
> > > > > This case, I think the common notify handler should be installed so that
> > > > > it can call _OST for error response as well.  The question is what to do
> > > > > when a scan handler has its own notify handler.
> > > 
> > > Actually, having considered this particular case a bit more I think that it
> > > is useful to install acpi_hotplug_notify_cb() for things whose scan handlers
> > > register hotplug support, but the registration fails (which should be treated
> > > as "permanently disabled").
> > >
> > > However, I still think that devices whose scan handlers don't support hotplug
> > > at all should be treated consistently with devices without scan handlers.
> > 
> > Basically, the kernel needs to be compliant with ACPI spec.  Once the
> > kernel tells firmware that it supports _OST, it needs to call _OST for
> > an ACPI hotplug event when _OST is implemented on the object.  Although
> > we cannot assure this behavior for the devices without scan handler, we
> > should do so for the devices with scan handlers.
> > 
> > > So, what about the slightly modified patch below?
> > 
> > Well, I do not think it is right.  The kernel is supposed to tell
> > firmware that it does not support hotplug when it doesn't... 
> 
> OK, I see your point.
> 
> I'll apply your patch, then, but I'm going to rename the new flag to
> "ignore".

Great. Thanks Rafael!
-Toshi

[PATCH 3/3] ACPI / PCI root: Clear driver_data before failing enumeration

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If a PCI host bridge cannot be enumerated due to an error in
pci_acpi_scan_root(), its ACPI device object's driver_data field
has to be cleared by acpi_pci_root_add() before freeing the
object pointed to by that field, or some later acpi_pci_find_root()
checks that should fail may succeed and cause quite a bit of
confusion to ensue.

Fix acpi_pci_root_add() to clear device->driver_data before
returning an error code as appropriate.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/pci_root.c |    1 +
 1 file changed, 1 insertion(+)

Index: linux-pm/drivers/acpi/pci_root.c
===================================================================
--- linux-pm.orig/drivers/acpi/pci_root.c
+++ linux-pm/drivers/acpi/pci_root.c
@@ -525,6 +525,7 @@ static int acpi_pci_root_add(struct acpi
 		dev_err(&device->dev,
 			"Bus %04x:%02x not present in PCI namespace\n",
 			root->segment, (unsigned int)root->secondary.start);
+		device->driver_data = NULL;
 		result = -ENODEV;
 		goto end;
 	}

Re: [PATCH 3/3] ACPI / PCI root: Clear driver_data before failing enumeration

[Toshi Kani]

On Thu, 2013-11-14 at 00:17 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> If a PCI host bridge cannot be enumerated due to an error in
> pci_acpi_scan_root(), its ACPI device object's driver_data field
> has to be cleared by acpi_pci_root_add() before freeing the
> object pointed to by that field, or some later acpi_pci_find_root()
> checks that should fail may succeed and cause quite a bit of
> confusion to ensue.
> 
> Fix acpi_pci_root_add() to clear device->driver_data before
> returning an error code as appropriate.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Toshi Kani <toshi.kani@hp.com>

Thanks,
-Toshi