[PATCH v4 0/6] ACPI: Add _OST support for ACPI hotplug

[PATCH v4 0/6] ACPI: Add _OST support for ACPI hotplug

[Toshi Kani]

This patchset supports ACPI OSPM Status Indication (_OST) method for
ACPI CPU/memory/container hotplug operations and sysfs eject. After
an ACPI hotplug operation has completed, OSPM calls _OST to indicate
the result of the operation to the platform. If a platform does not
support _OST, this patchset has no effect on the platform.

This _OST support is enabled when all relevant ACPI hotplug operations,
such as CPU, memory and container hotplug, are enabled. This assures
consistent behavior among the hotplug operations with regarding the
_OST support.

Some platforms may require the OS to support _OST in order to support
ACPI hotplug operations. For example, if a platform has the management
console where user can request a hotplug operation from, this _OST
support would be required for the management console to show the result
of the hotplug request to user.

The _OST definition can be found in section 6.3.5 of ACPI 5.0 spec.
The HPPF spec below also describes hotplug flows with _OST.

  DIG64 Hot-Plug & Partitioning Flow (HPPF) Specification R1.0
  http://www.dig64.org/home/DIG64_HPPF_R1_0.pdf

The changes have been tested with simulated _OST methods.

v4:
 - Removed CONFIG_ACPI_HOTPLUG_OST option.

v3:
 - Added more descriptions to the change logs.

v2:
 - Added CONFIG_ACPI_HOTPLUG_OST option. 
 - Added _OST support for container hotplug and sysfs eject.
 - Reordered patchset to enable _OST support bit of _OSC in the 
   last patch.

---
Toshi Kani (6):
 ACPI: Add an interface to evaluate _OST
 ACPI: Add _OST support for sysfs eject
 ACPI: Add _OST support for ACPI CPU hotplug
 ACPI: Add _OST support for ACPI memory hotplug
 ACPI: Add _OST support for ACPI container hotplug
 ACPI: Set hotplug _OST support bit to _OSC

 drivers/acpi/acpi_memhotplug.c  |   43 +++++++++++++++++++++-------
 drivers/acpi/bus.c              |    4 +++
 drivers/acpi/container.c        |   43 +++++++++++++++++++----------
 drivers/acpi/processor_driver.c |   28 +++++++++++++-----
 drivers/acpi/scan.c             |   58 +++++++++++++++++++++++++++++++++------
 drivers/acpi/utils.c            |   42 ++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h         |   12 +++++++-
 include/linux/acpi.h            |   40 ++++++++++++++++++++++++++-
 8 files changed, 225 insertions(+), 45 deletions(-)

[PATCH v4 1/6] ACPI: Add an interface to evaluate _OST

[Toshi Kani]

Added acpi_evaluate_hotplug_opt(). All ACPI hotplug handlers must call
this function when evaluating _OST for hotplug operations. If the
platform does not support _OST, this function returns AE_NOT_FOUND and
has no effect on the platform.

ACPI_HOTPLUG_OST is defined when all relevant ACPI hotplug operations,
such as CPU, memory and container hotplug, are enabled. This assures
consistent behavior among the hotplug operations with regarding the
_OST support. When ACPI_HOTPLUG_OST is not defined, this function is
a no-op.

ACPI PCI hotplug is not enhanced to support _OST at this time since it
is a legacy method being replaced by PCIe native hotplug. _OST support
for ACPI PCI hotplug may be added in future if necessary.

Some platforms may require the OS to support _OST in order to support
ACPI hotplug operations. For example, if a platform has the management
console where user can request a hotplug operation from, this _OST
support would be required for the management console to show the result
of the hotplug request to user.

Added macro definitions of _OST source events and status codes.
Also renamed OSC_SB_CPUHP_OST_SUPPORT to OSC_SB_HOTPLUG_OST_SUPPORT
since this _OSC bit is not specific to CPU hotplug. This bit is
defined in Table 6-147 of ACPI 5.0 as follows.

  Bits:       3
  Field Name: Insertion / Ejection _OST Processing Support
  Definition: This bit is set if OSPM will evaluate the _OST
              object defined under a device when processing
              insertion and ejection source event codes.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 drivers/acpi/utils.c    |   42 ++++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h |    3 +++
 include/linux/acpi.h    |   40 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 84 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index adbbc1c..3e87c9c 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -412,3 +412,45 @@ out:
 	return status;
 }
 EXPORT_SYMBOL(acpi_get_physical_device_location);
+
+/**
+ * acpi_evaluate_hotplug_ost: Evaluate _OST for hotplug operations
+ * @handle: ACPI device handle
+ * @source_event: source event code
+ * @status_code: status code
+ * @status_buf: optional detailed information (NULL if none)
+ *
+ * Evaluate _OST for hotplug operations. All ACPI hotplug handlers
+ * must call this function when evaluating _OST for hotplug operations.
+ * When the platform does not support _OST, this function has no effect.
+ */
+acpi_status
+acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event,
+		u32 status_code, struct acpi_buffer *status_buf)
+{
+#ifdef ACPI_HOTPLUG_OST
+	union acpi_object params[3] = {
+		{.type = ACPI_TYPE_INTEGER,},
+		{.type = ACPI_TYPE_INTEGER,},
+		{.type = ACPI_TYPE_BUFFER,}
+	};
+	struct acpi_object_list arg_list = {3, params};
+	acpi_status status;
+
+	params[0].integer.value = source_event;
+	params[1].integer.value = status_code;
+	if (status_buf != NULL) {
+		params[2].buffer.pointer = status_buf->pointer;
+		params[2].buffer.length = status_buf->length;
+	} else {
+		params[2].buffer.pointer = NULL;
+		params[2].buffer.length = 0;
+	}
+
+	status = acpi_evaluate_object(handle, "_OST", &arg_list, NULL);
+	return status;
+#else
+	return AE_OK;
+#endif
+}
+EXPORT_SYMBOL(acpi_evaluate_hotplug_ost);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index b0d6282..1139f3a 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -50,6 +50,9 @@ acpi_evaluate_reference(acpi_handle handle,
 			acpi_string pathname,
 			struct acpi_object_list *arguments,
 			struct acpi_handle_list *list);
+acpi_status
+acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event,
+			u32 status_code, struct acpi_buffer *status_buf);
 
 struct acpi_pld {
 	unsigned int revision:7; /* 0 */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index f421dd8..b2b4d2a 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -277,7 +277,7 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context);
 #define OSC_SB_PAD_SUPPORT		1
 #define OSC_SB_PPC_OST_SUPPORT		2
 #define OSC_SB_PR3_SUPPORT		4
-#define OSC_SB_CPUHP_OST_SUPPORT	8
+#define OSC_SB_HOTPLUG_OST_SUPPORT	8
 #define OSC_SB_APEI_SUPPORT		16
 
 extern bool osc_sb_apei_support_acked;
@@ -309,6 +309,44 @@ extern bool osc_sb_apei_support_acked;
 
 extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
 					     u32 *mask, u32 req);
+
+/* Enable _OST when all relevant hotplug operations are enabled */
+#if defined(CONFIG_ACPI_HOTPLUG_CPU) &&			\
+	(defined(CONFIG_ACPI_HOTPLUG_MEMORY) ||		\
+	 defined(CONFIG_ACPI_HOTPLUG_MEMORY_MODULE)) &&	\
+	(defined(CONFIG_ACPI_CONTAINER) ||		\
+	 defined(CONFIG_ACPI_CONTAINER_MODULE))
+#define ACPI_HOTPLUG_OST
+#endif
+
+/* _OST Source Event Code (OSPM Action) */
+#define ACPI_OST_EC_OSPM_SHUTDOWN		0x100
+#define ACPI_OST_EC_OSPM_EJECT			0x103
+#define ACPI_OST_EC_OSPM_INSERTION		0x200
+
+/* _OST General Processing Status Code */
+#define ACPI_OST_SC_SUCCESS			0x0
+#define ACPI_OST_SC_NON_SPECIFIC_FAILURE	0x1
+#define ACPI_OST_SC_UNRECOGNIZED_NOTIFY		0x2
+
+/* _OST OS Shutdown Processing (0x100) Status Code */
+#define ACPI_OST_SC_OS_SHUTDOWN_DENIED		0x80
+#define ACPI_OST_SC_OS_SHUTDOWN_IN_PROGRESS	0x81
+#define ACPI_OST_SC_OS_SHUTDOWN_COMPLETED	0x82
+#define ACPI_OST_SC_OS_SHUTDOWN_NOT_SUPPORTED	0x83
+
+/* _OST Ejection Request (0x3, 0x103) Status Code */
+#define ACPI_OST_SC_EJECT_NOT_SUPPORTED		0x80
+#define ACPI_OST_SC_DEVICE_IN_USE		0x81
+#define ACPI_OST_SC_DEVICE_BUSY			0x82
+#define ACPI_OST_SC_EJECT_DEPENDENCY_BUSY	0x83
+#define ACPI_OST_SC_EJECT_IN_PROGRESS		0x84
+
+/* _OST Insertion Request (0x200) Status Code */
+#define ACPI_OST_SC_INSERT_IN_PROGRESS		0x80
+#define ACPI_OST_SC_DRIVER_LOAD_FAILURE		0x81
+#define ACPI_OST_SC_INSERT_NOT_SUPPORTED	0x82
+
 extern void acpi_early_init(void);
 
 extern int acpi_nvs_register(__u64 start, __u64 size);
-- 
1.7.7.6

Re: [PATCH v4 1/6] ACPI: Add an interface to evaluate _OST

[Shuah Khan]

On Wed, 2012-05-23 at 20:25 -0600, Toshi Kani wrote:
> Added acpi_evaluate_hotplug_opt(). All ACPI hotplug handlers must call
> this function when evaluating _OST for hotplug operations. If the
> platform does not support _OST, this function returns AE_NOT_FOUND and
> has no effect on the platform.
> 
> ACPI_HOTPLUG_OST is defined when all relevant ACPI hotplug operations,
> such as CPU, memory and container hotplug, are enabled. This assures
> consistent behavior among the hotplug operations with regarding the
> _OST support. When ACPI_HOTPLUG_OST is not defined, this function is
> a no-op.
> 
> ACPI PCI hotplug is not enhanced to support _OST at this time since it
> is a legacy method being replaced by PCIe native hotplug. _OST support
> for ACPI PCI hotplug may be added in future if necessary.
> 
> Some platforms may require the OS to support _OST in order to support
> ACPI hotplug operations. For example, if a platform has the management
> console where user can request a hotplug operation from, this _OST
> support would be required for the management console to show the result
> of the hotplug request to user.
> 
> Added macro definitions of _OST source events and status codes.
> Also renamed OSC_SB_CPUHP_OST_SUPPORT to OSC_SB_HOTPLUG_OST_SUPPORT
> since this _OSC bit is not specific to CPU hotplug. This bit is
> defined in Table 6-147 of ACPI 5.0 as follows.
> 
>   Bits:       3
>   Field Name: Insertion / Ejection _OST Processing Support
>   Definition: This bit is set if OSPM will evaluate the _OST
>               object defined under a device when processing
>               insertion and ejection source event codes.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> ---
>  drivers/acpi/utils.c    |   42 ++++++++++++++++++++++++++++++++++++++++++
>  include/acpi/acpi_bus.h |    3 +++
>  include/linux/acpi.h    |   40 +++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 84 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
> index adbbc1c..3e87c9c 100644
> --- a/drivers/acpi/utils.c
> +++ b/drivers/acpi/utils.c
> @@ -412,3 +412,45 @@ out:
>  	return status;
>  }
>  EXPORT_SYMBOL(acpi_get_physical_device_location);
> +
> +/**
> + * acpi_evaluate_hotplug_ost: Evaluate _OST for hotplug operations
> + * @handle: ACPI device handle
> + * @source_event: source event code
> + * @status_code: status code
> + * @status_buf: optional detailed information (NULL if none)
> + *
> + * Evaluate _OST for hotplug operations. All ACPI hotplug handlers
> + * must call this function when evaluating _OST for hotplug operations.
> + * When the platform does not support _OST, this function has no effect.
> + */
> +acpi_status
> +acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event,
> +		u32 status_code, struct acpi_buffer *status_buf)
> +{
> +#ifdef ACPI_HOTPLUG_OST
> +	union acpi_object params[3] = {
> +		{.type = ACPI_TYPE_INTEGER,},
> +		{.type = ACPI_TYPE_INTEGER,},
> +		{.type = ACPI_TYPE_BUFFER,}
> +	};
> +	struct acpi_object_list arg_list = {3, params};
> +	acpi_status status;
> +
> +	params[0].integer.value = source_event;
> +	params[1].integer.value = status_code;
> +	if (status_buf != NULL) {
> +		params[2].buffer.pointer = status_buf->pointer;
> +		params[2].buffer.length = status_buf->length;
> +	} else {
> +		params[2].buffer.pointer = NULL;
> +		params[2].buffer.length = 0;
> +	}
> +
> +	status = acpi_evaluate_object(handle, "_OST", &arg_list, NULL);
> +	return status;
> +#else
> +	return AE_OK;
> +#endif
> +}
> +EXPORT_SYMBOL(acpi_evaluate_hotplug_ost);
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index b0d6282..1139f3a 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -50,6 +50,9 @@ acpi_evaluate_reference(acpi_handle handle,
>  			acpi_string pathname,
>  			struct acpi_object_list *arguments,
>  			struct acpi_handle_list *list);
> +acpi_status
> +acpi_evaluate_hotplug_ost(acpi_handle handle, u32 source_event,
> +			u32 status_code, struct acpi_buffer *status_buf);
>  
>  struct acpi_pld {
>  	unsigned int revision:7; /* 0 */
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index f421dd8..b2b4d2a 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -277,7 +277,7 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context);
>  #define OSC_SB_PAD_SUPPORT		1
>  #define OSC_SB_PPC_OST_SUPPORT		2
>  #define OSC_SB_PR3_SUPPORT		4
> -#define OSC_SB_CPUHP_OST_SUPPORT	8
> +#define OSC_SB_HOTPLUG_OST_SUPPORT	8
>  #define OSC_SB_APEI_SUPPORT		16
>  
>  extern bool osc_sb_apei_support_acked;
> @@ -309,6 +309,44 @@ extern bool osc_sb_apei_support_acked;
>  
>  extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
>  					     u32 *mask, u32 req);
> +
> +/* Enable _OST when all relevant hotplug operations are enabled */
> +#if defined(CONFIG_ACPI_HOTPLUG_CPU) &&			\
> +	(defined(CONFIG_ACPI_HOTPLUG_MEMORY) ||		\
> +	 defined(CONFIG_ACPI_HOTPLUG_MEMORY_MODULE)) &&	\
> +	(defined(CONFIG_ACPI_CONTAINER) ||		\
> +	 defined(CONFIG_ACPI_CONTAINER_MODULE))
> +#define ACPI_HOTPLUG_OST
> +#endif

Already covered in my general comments, but just in case:

This is restricted to a few of the possible cases _OST is intended for.
What happens when a kernel is configed with all of the above and PCI and
PCIe hotplug configs. _OST will only be evaluated in these cases,
shouldn't OS evaluate _OST in all cases once it tells firmware it
supports _OST in the case of Insertion/Ejection?

> +
> +/* _OST Source Event Code (OSPM Action) */
> +#define ACPI_OST_EC_OSPM_SHUTDOWN		0x100
> +#define ACPI_OST_EC_OSPM_EJECT			0x103
> +#define ACPI_OST_EC_OSPM_INSERTION		0x200
> +
> +/* _OST General Processing Status Code */
> +#define ACPI_OST_SC_SUCCESS			0x0
> +#define ACPI_OST_SC_NON_SPECIFIC_FAILURE	0x1
> +#define ACPI_OST_SC_UNRECOGNIZED_NOTIFY		0x2
> +
> +/* _OST OS Shutdown Processing (0x100) Status Code */
> +#define ACPI_OST_SC_OS_SHUTDOWN_DENIED		0x80
> +#define ACPI_OST_SC_OS_SHUTDOWN_IN_PROGRESS	0x81
> +#define ACPI_OST_SC_OS_SHUTDOWN_COMPLETED	0x82
> +#define ACPI_OST_SC_OS_SHUTDOWN_NOT_SUPPORTED	0x83
> +
> +/* _OST Ejection Request (0x3, 0x103) Status Code */
> +#define ACPI_OST_SC_EJECT_NOT_SUPPORTED		0x80
> +#define ACPI_OST_SC_DEVICE_IN_USE		0x81
> +#define ACPI_OST_SC_DEVICE_BUSY			0x82
> +#define ACPI_OST_SC_EJECT_DEPENDENCY_BUSY	0x83
> +#define ACPI_OST_SC_EJECT_IN_PROGRESS		0x84
> +
> +/* _OST Insertion Request (0x200) Status Code */
> +#define ACPI_OST_SC_INSERT_IN_PROGRESS		0x80
> +#define ACPI_OST_SC_DRIVER_LOAD_FAILURE		0x81
> +#define ACPI_OST_SC_INSERT_NOT_SUPPORTED	0x82
> +
>  extern void acpi_early_init(void);
>  
>  extern int acpi_nvs_register(__u64 start, __u64 size);

Re: [PATCH v4 1/6] ACPI: Add an interface to evaluate _OST

[Toshi Kani]

On Thu, 2012-05-24 at 12:09 -0600, Shuah Khan wrote:
> On Wed, 2012-05-23 at 20:25 -0600, Toshi Kani wrote:
 :
> ACPI_HOTPLUG_OST is defined when all relevant ACPI hotplug operations,
> such as CPU, memory and container hotplug, are enabled. This assures
> consistent behavior among the hotplug operations with regarding the
> _OST support. When ACPI_HOTPLUG_OST is not defined, this function is
> a no-op.
> 
> ACPI PCI hotplug is not enhanced to support _OST at this time since it
> is a legacy method being replaced by PCIe native hotplug. _OST support
> for ACPI PCI hotplug may be added in future if necessary.
 :
> > +
> > +/* Enable _OST when all relevant hotplug operations are enabled */
> > +#if defined(CONFIG_ACPI_HOTPLUG_CPU) &&			\
> > +	(defined(CONFIG_ACPI_HOTPLUG_MEMORY) ||		\
> > +	 defined(CONFIG_ACPI_HOTPLUG_MEMORY_MODULE)) &&	\
> > +	(defined(CONFIG_ACPI_CONTAINER) ||		\
> > +	 defined(CONFIG_ACPI_CONTAINER_MODULE))
> > +#define ACPI_HOTPLUG_OST
> > +#endif
> 
> Already covered in my general comments, but just in case:
> 
> This is restricted to a few of the possible cases _OST is intended for.
> What happens when a kernel is configed with all of the above and PCI and
> PCIe hotplug configs. _OST will only be evaluated in these cases,
> shouldn't OS evaluate _OST in all cases once it tells firmware it
> supports _OST in the case of Insertion/Ejection?

PCIe native hotplug is a non-ACPI based hotplug, and is not supported by
_OST.  ACPI PCI hotplug is being replaced by PCIe native hotplug, so it
is not supported as described in the change log.  Please see the email
below on this.
http://marc.info/?l=linux-acpi&m=133669547631220&w=2


Thanks,
-Toshi

[PATCH v4 2/6] ACPI: Add _OST support for sysfs eject

[Toshi Kani]

Changed acpi_bus_hot_remove_device() to support _OST. This function is
also changed to global so that it can be called from hotplug notify
handlers to perform hot-remove operation.

Changed acpi_eject_store(), which is the sysfs eject handler. It checks
eject_pending to see if the request was originated from ACPI eject
notification. If not, it calls _OST(0x103,84,) per Figure 6-37 in ACPI
5.0 spec.

Added eject_pending bit to acpi_device_flags. This bit is set when the
kernel has received an ACPI eject notification, but does not initiate
its hot-remove operation by itself.

Added struct acpi_eject_event. This structure is used to pass extended
information to acpi_bus_hot_remove_device(), which has a single argument
to support asynchronous call

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 drivers/acpi/scan.c     |   58 +++++++++++++++++++++++++++++++++++++++-------
 include/acpi/acpi_bus.h |    9 ++++++-
 2 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 85cbfdc..bea3ab6 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -83,19 +83,29 @@ acpi_device_modalias_show(struct device *dev, struct device_attribute *attr, cha
 }
 static DEVICE_ATTR(modalias, 0444, acpi_device_modalias_show, NULL);
 
-static void acpi_bus_hot_remove_device(void *context)
+/**
+ * acpi_bus_hot_remove_device: hot-remove a device and its children
+ * @context: struct acpi_eject_event pointer (freed in this func)
+ *
+ * Hot-remove a device and its children. This function frees up the
+ * memory space passed by arg context, so that the caller may call
+ * this function asynchronously through acpi_os_hotplug_execute().
+ */
+void acpi_bus_hot_remove_device(void *context)
 {
+	struct acpi_eject_event *ej_event = (struct acpi_eject_event *) context;
 	struct acpi_device *device;
-	acpi_handle handle = context;
+	acpi_handle handle = ej_event->handle;
 	struct acpi_object_list arg_list;
 	union acpi_object arg;
 	acpi_status status = AE_OK;
+	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
 
 	if (acpi_bus_get_device(handle, &device))
-		return;
+		goto err_out;
 
 	if (!device)
-		return;
+		goto err_out;
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 		"Hot-removing device %s...\n", dev_name(&device->dev)));
@@ -103,7 +113,7 @@ static void acpi_bus_hot_remove_device(void *context)
 	if (acpi_bus_trim(device, 1)) {
 		printk(KERN_ERR PREFIX
 				"Removing device failed\n");
-		return;
+		goto err_out;
 	}
 
 	/* power off device */
@@ -129,10 +139,21 @@ static void acpi_bus_hot_remove_device(void *context)
 	 * TBD: _EJD support.
 	 */
 	status = acpi_evaluate_object(handle, "_EJ0", &arg_list, NULL);
-	if (ACPI_FAILURE(status))
-		printk(KERN_WARNING PREFIX
-				"Eject device failed\n");
+	if (ACPI_FAILURE(status)) {
+		if (status != AE_NOT_FOUND)
+			printk(KERN_WARNING PREFIX
+					"Eject device failed\n");
+		goto err_out;
+	}
+
+	kfree(context);
+	return;
 
+err_out:
+	/* Inform firmware the hot-remove operation has completed w/ error */
+	(void) acpi_evaluate_hotplug_ost(handle,
+				ej_event->event, ost_code, NULL);
+	kfree(context);
 	return;
 }
 
@@ -144,6 +165,7 @@ acpi_eject_store(struct device *d, struct device_attribute *attr,
 	acpi_status status;
 	acpi_object_type type = 0;
 	struct acpi_device *acpi_device = to_acpi_device(d);
+	struct acpi_eject_event *ej_event;
 
 	if ((!count) || (buf[0] != '1')) {
 		return -EINVAL;
@@ -160,7 +182,25 @@ acpi_eject_store(struct device *d, struct device_attribute *attr,
 		goto err;
 	}
 
-	acpi_os_hotplug_execute(acpi_bus_hot_remove_device, acpi_device->handle);
+	ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
+	if (!ej_event) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	ej_event->handle = acpi_device->handle;
+	if (acpi_device->flags.eject_pending) {
+		/* event originated from ACPI eject notification */
+		ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
+		acpi_device->flags.eject_pending = 0;
+	} else {
+		/* event originated from user */
+		ej_event->event = ACPI_OST_EC_OSPM_EJECT;
+		(void) acpi_evaluate_hotplug_ost(ej_event->handle,
+			ej_event->event, ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
+	}
+
+	acpi_os_hotplug_execute(acpi_bus_hot_remove_device, (void *)ej_event);
 err:
 	return ret;
 }
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 1139f3a..62eb514 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -182,7 +182,8 @@ struct acpi_device_flags {
 	u32 suprise_removal_ok:1;
 	u32 power_manageable:1;
 	u32 performance_manageable:1;
-	u32 reserved:24;
+	u32 eject_pending:1;
+	u32 reserved:23;
 };
 
 /* File System */
@@ -334,6 +335,11 @@ struct acpi_bus_event {
 	u32 data;
 };
 
+struct acpi_eject_event {
+	acpi_handle	handle;
+	u32		event;
+};
+
 extern struct kobject *acpi_kobj;
 extern int acpi_bus_generate_netlink_event(const char*, const char*, u8, int);
 void acpi_bus_private_data_handler(acpi_handle, void *);
@@ -371,6 +377,7 @@ int acpi_bus_register_driver(struct acpi_driver *driver);
 void acpi_bus_unregister_driver(struct acpi_driver *driver);
 int acpi_bus_add(struct acpi_device **child, struct acpi_device *parent,
 		 acpi_handle handle, int type);
+void acpi_bus_hot_remove_device(void *context);
 int acpi_bus_trim(struct acpi_device *start, int rmdevice);
 int acpi_bus_start(struct acpi_device *device);
 acpi_status acpi_bus_get_ejd(acpi_handle handle, acpi_handle * ejd);
-- 
1.7.7.6

[PATCH v4 3/6] ACPI: Add _OST support for ACPI CPU hotplug

[Toshi Kani]

Changed acpi_processor_hotplug_notify() to call ACPI _OST method
when ACPI CPU hotplug operation has completed.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 drivers/acpi/processor_driver.c |   28 ++++++++++++++++++++--------
 1 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 0734086..971c454 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -701,9 +701,9 @@ static void acpi_processor_hotplug_notify(acpi_handle handle,
 {
 	struct acpi_processor *pr;
 	struct acpi_device *device = NULL;
+	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
 	int result;
 
-
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
 	case ACPI_NOTIFY_DEVICE_CHECK:
@@ -715,14 +715,18 @@ static void acpi_processor_hotplug_notify(acpi_handle handle,
 		if (!is_processor_present(handle))
 			break;
 
-		if (acpi_bus_get_device(handle, &device)) {
-			result = acpi_processor_device_add(handle, &device);
-			if (result)
-				printk(KERN_ERR PREFIX
-					    "Unable to add the device\n");
+		if (!acpi_bus_get_device(handle, &device))
+			break;
+
+		result = acpi_processor_device_add(handle, &device);
+		if (result) {
+			printk(KERN_ERR PREFIX "Unable to add the device\n");
 			break;
 		}
+
+		ost_code = ACPI_OST_SC_SUCCESS;
 		break;
+
 	case ACPI_NOTIFY_EJECT_REQUEST:
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "received ACPI_NOTIFY_EJECT_REQUEST\n"));
@@ -736,15 +740,23 @@ static void acpi_processor_hotplug_notify(acpi_handle handle,
 		if (!pr) {
 			printk(KERN_ERR PREFIX
 				    "Driver data is NULL, dropping EJECT\n");
-			return;
+			break;
 		}
+
+		/* REVISIT: update when eject is supported */
+		ost_code = ACPI_OST_SC_EJECT_NOT_SUPPORTED;
 		break;
+
 	default:
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "Unsupported event [0x%x]\n", event));
-		break;
+
+		/* non-hotplug event; possibly handled by other handler */
+		return;
 	}
 
+	/* Inform firmware that the hotplug operation has completed */
+	(void) acpi_evaluate_hotplug_ost(handle, event, ost_code, NULL);
 	return;
 }
 
-- 
1.7.7.6

[PATCH v4 4/6] ACPI: Add _OST support for ACPI memory hotplug

[Toshi Kani]

Changed acpi_memory_device_notify() to call ACPI _OST method
when ACPI memory hotplug operation has completed.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 drivers/acpi/acpi_memhotplug.c |   43 +++++++++++++++++++++++++++++----------
 1 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index d985713..24c807f 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -341,7 +341,7 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct acpi_memory_device *mem_device;
 	struct acpi_device *device;
-
+	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
 
 	switch (event) {
 	case ACPI_NOTIFY_BUS_CHECK:
@@ -354,15 +354,20 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
 					  "\nReceived DEVICE CHECK notification for device\n"));
 		if (acpi_memory_get_device(handle, &mem_device)) {
 			printk(KERN_ERR PREFIX "Cannot find driver data\n");
-			return;
+			break;
 		}
 
-		if (!acpi_memory_check_device(mem_device)) {
-			if (acpi_memory_enable_device(mem_device))
-				printk(KERN_ERR PREFIX
-					    "Cannot enable memory device\n");
+		if (acpi_memory_check_device(mem_device))
+			break;
+
+		if (acpi_memory_enable_device(mem_device)) {
+			printk(KERN_ERR PREFIX "Cannot enable memory device\n");
+			break;
 		}
+
+		ost_code = ACPI_OST_SC_SUCCESS;
 		break;
+
 	case ACPI_NOTIFY_EJECT_REQUEST:
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "\nReceived EJECT REQUEST notification for device\n"));
@@ -383,19 +388,35 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
 		 * TBD: Can also be disabled by Callback registration
 		 *      with generic sysfs driver
 		 */
-		if (acpi_memory_disable_device(mem_device))
-			printk(KERN_ERR PREFIX
-				    "Disable memory device\n");
+		if (acpi_memory_disable_device(mem_device)) {
+			printk(KERN_ERR PREFIX "Disable memory device\n");
+			/*
+			 * If _EJ0 was called but failed, _OST is not
+			 * necessary.
+			 */
+			if (mem_device->state == MEMORY_INVALID_STATE)
+				return;
+
+			break;
+		}
+
 		/*
 		 * TBD: Invoke acpi_bus_remove to cleanup data structures
 		 */
-		break;
+
+		/* _EJ0 succeeded; _OST is not necessary */
+		return;
+
 	default:
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "Unsupported event [0x%x]\n", event));
-		break;
+
+		/* non-hotplug event; possibly handled by other handler */
+		return;
 	}
 
+	/* Inform firmware that the hotplug operation has completed */
+	(void) acpi_evaluate_hotplug_ost(handle, event, ost_code, NULL);
 	return;
 }
 
-- 
1.7.7.6

Re: [PATCH v4 4/6] ACPI: Add _OST support for ACPI memory hotplug

[Shuah Khan]

On Wed, 2012-05-23 at 20:25 -0600, Toshi Kani wrote:
> Changed acpi_memory_device_notify() to call ACPI _OST method
> when ACPI memory hotplug operation has completed.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> ---
>  drivers/acpi/acpi_memhotplug.c |   43 +++++++++++++++++++++++++++++----------
>  1 files changed, 32 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> index d985713..24c807f 100644
> --- a/drivers/acpi/acpi_memhotplug.c
> +++ b/drivers/acpi/acpi_memhotplug.c
> @@ -341,7 +341,7 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
>  {
>  	struct acpi_memory_device *mem_device;
>  	struct acpi_device *device;
> -
> +	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
>  
>  	switch (event) {
>  	case ACPI_NOTIFY_BUS_CHECK:
> @@ -354,15 +354,20 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
>  					  "\nReceived DEVICE CHECK notification for device\n"));
>  		if (acpi_memory_get_device(handle, &mem_device)) {
>  			printk(KERN_ERR PREFIX "Cannot find driver data\n");
> -			return;
> +			break;
>  		}
>  
> -		if (!acpi_memory_check_device(mem_device)) {
> -			if (acpi_memory_enable_device(mem_device))
> -				printk(KERN_ERR PREFIX
> -					    "Cannot enable memory device\n");
> +		if (acpi_memory_check_device(mem_device))
> +			break;
> +
> +		if (acpi_memory_enable_device(mem_device)) {
> +			printk(KERN_ERR PREFIX "Cannot enable memory device\n");
> +			break;
>  		}
> +
> +		ost_code = ACPI_OST_SC_SUCCESS;
>  		break;
> +
>  	case ACPI_NOTIFY_EJECT_REQUEST:
>  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>  				  "\nReceived EJECT REQUEST notification for device\n"));
> @@ -383,19 +388,35 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
>  		 * TBD: Can also be disabled by Callback registration
>  		 *      with generic sysfs driver
>  		 */
> -		if (acpi_memory_disable_device(mem_device))
> -			printk(KERN_ERR PREFIX
> -				    "Disable memory device\n");
> +		if (acpi_memory_disable_device(mem_device)) {
> +			printk(KERN_ERR PREFIX "Disable memory device\n");
> +			/*
> +			 * If _EJ0 was called but failed, _OST is not
> +			 * necessary.
> +			 */
> +			if (mem_device->state == MEMORY_INVALID_STATE)
> +				return;
> +
> +			break;
> +		}

Why isn't _OST called in this case to report failure? Isn't this one of
the cases Spec talks about. Reference: 6.3.5 _OST (OSPM Status
Indication) page 302.

-- Shuah

> +
>  		/*
>  		 * TBD: Invoke acpi_bus_remove to cleanup data structures
>  		 */
> -		break;
> +
> +		/* _EJ0 succeeded; _OST is not necessary */
> +		return;
> +
>  	default:
>  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>  				  "Unsupported event [0x%x]\n", event));
> -		break;
> +
> +		/* non-hotplug event; possibly handled by other handler */
> +		return;
>  	}
>  
> +	/* Inform firmware that the hotplug operation has completed */
> +	(void) acpi_evaluate_hotplug_ost(handle, event, ost_code, NULL);
>  	return;
>  }
>  

Re: [PATCH v4 4/6] ACPI: Add _OST support for ACPI memory hotplug

[Toshi Kani]

On Thu, 2012-05-24 at 12:21 -0600, Shuah Khan wrote:
> On Wed, 2012-05-23 at 20:25 -0600, Toshi Kani wrote:
> > Changed acpi_memory_device_notify() to call ACPI _OST method
> > when ACPI memory hotplug operation has completed.
> > 
> > Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> > ---
> >  drivers/acpi/acpi_memhotplug.c |   43 +++++++++++++++++++++++++++++----------
> >  1 files changed, 32 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> > index d985713..24c807f 100644
> > --- a/drivers/acpi/acpi_memhotplug.c
> > +++ b/drivers/acpi/acpi_memhotplug.c
> > @@ -341,7 +341,7 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
> >  {
> >  	struct acpi_memory_device *mem_device;
> >  	struct acpi_device *device;
> > -
> > +	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
> >  
> >  	switch (event) {
> >  	case ACPI_NOTIFY_BUS_CHECK:
> > @@ -354,15 +354,20 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
> >  					  "\nReceived DEVICE CHECK notification for device\n"));
> >  		if (acpi_memory_get_device(handle, &mem_device)) {
> >  			printk(KERN_ERR PREFIX "Cannot find driver data\n");
> > -			return;
> > +			break;
> >  		}
> >  
> > -		if (!acpi_memory_check_device(mem_device)) {
> > -			if (acpi_memory_enable_device(mem_device))
> > -				printk(KERN_ERR PREFIX
> > -					    "Cannot enable memory device\n");
> > +		if (acpi_memory_check_device(mem_device))
> > +			break;
> > +
> > +		if (acpi_memory_enable_device(mem_device)) {
> > +			printk(KERN_ERR PREFIX "Cannot enable memory device\n");
> > +			break;
> >  		}
> > +
> > +		ost_code = ACPI_OST_SC_SUCCESS;
> >  		break;
> > +
> >  	case ACPI_NOTIFY_EJECT_REQUEST:
> >  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> >  				  "\nReceived EJECT REQUEST notification for device\n"));
> > @@ -383,19 +388,35 @@ static void acpi_memory_device_notify(acpi_handle handle, u32 event, void *data)
> >  		 * TBD: Can also be disabled by Callback registration
> >  		 *      with generic sysfs driver
> >  		 */
> > -		if (acpi_memory_disable_device(mem_device))
> > -			printk(KERN_ERR PREFIX
> > -				    "Disable memory device\n");
> > +		if (acpi_memory_disable_device(mem_device)) {
> > +			printk(KERN_ERR PREFIX "Disable memory device\n");
> > +			/*
> > +			 * If _EJ0 was called but failed, _OST is not
> > +			 * necessary.
> > +			 */
> > +			if (mem_device->state == MEMORY_INVALID_STATE)
> > +				return;
> > +
> > +			break;
> > +		}
> 
> Why isn't _OST called in this case to report failure? Isn't this one of
> the cases Spec talks about. Reference: 6.3.5 _OST (OSPM Status
> Indication) page 302.

If an ejection operation failed before calling _EJ0, the OS needs to
call _OST.  Once the OS called _EJ0, the OS need not to call _OST again
since FW has been informed with _EJ0 already.  If _EJ0 failed, FW knows
it failed as well.  Please see page 14, Figure 2, of DIG64 HPPF doc.
The pointer to the doc is described in patch [0/6].

Thanks,
-Toshi


> 
> -- Shuah
> 
> > +
> >  		/*
> >  		 * TBD: Invoke acpi_bus_remove to cleanup data structures
> >  		 */
> > -		break;
> > +
> > +		/* _EJ0 succeeded; _OST is not necessary */
> > +		return;
> > +
> >  	default:
> >  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> >  				  "Unsupported event [0x%x]\n", event));
> > -		break;
> > +
> > +		/* non-hotplug event; possibly handled by other handler */
> > +		return;
> >  	}
> >  
> > +	/* Inform firmware that the hotplug operation has completed */
> > +	(void) acpi_evaluate_hotplug_ost(handle, event, ost_code, NULL);
> >  	return;
> >  }
> >  
> 
> 

[PATCH v4 5/6] ACPI: Add _OST support for ACPI container hotplug

[Toshi Kani]

Changed container_notify_cb() to call ACPI _OST method when ACPI
container hotplug operation has completed. Slightly restructured
the code with the same logic. The function sets eject_pending bit
for an eject request since it does not initiate hot-remove operation.
This bit is checked by the sysfs eject handler to determine if the
request is originated from an ACPI eject notification.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 drivers/acpi/container.c |   43 ++++++++++++++++++++++++++++---------------
 1 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/drivers/acpi/container.c b/drivers/acpi/container.c
index 45cd03b..1f9f7d7 100644
--- a/drivers/acpi/container.c
+++ b/drivers/acpi/container.c
@@ -158,9 +158,7 @@ static void container_notify_cb(acpi_handle handle, u32 type, void *context)
 	int result;
 	int present;
 	acpi_status status;
-
-
-	present = is_device_present(handle);
+	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
 
 	switch (type) {
 	case ACPI_NOTIFY_BUS_CHECK:
@@ -169,32 +167,47 @@ static void container_notify_cb(acpi_handle handle, u32 type, void *context)
 		printk(KERN_WARNING "Container driver received %s event\n",
 		       (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_FAILURE(status) || !device) {
-				result = container_device_add(&device, handle);
-				if (!result)
-					kobject_uevent(&device->dev.kobj,
-						       KOBJ_ONLINE);
-				else
-					printk(KERN_WARNING
-					       "Failed to add container\n");
-			}
-		} else {
+		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);
+				return;
 			}
+			break;
+		}
+
+		if (!ACPI_FAILURE(status) || device)
+			break;
+
+		result = container_device_add(&device, handle);
+		if (result) {
+			printk(KERN_WARNING "Failed to add container\n");
+			break;
 		}
+
+		kobject_uevent(&device->dev.kobj, KOBJ_ONLINE);
+		ost_code = ACPI_OST_SC_SUCCESS;
 		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;
 		}
 		break;
+
 	default:
-		break;
+		/* non-hotplug event; possibly handled by other handler */
+		return;
 	}
+
+	/* Inform firmware that the hotplug operation has completed */
+	(void) acpi_evaluate_hotplug_ost(handle, type, ost_code, NULL);
 	return;
 }
 
-- 
1.7.7.6

Re: [PATCH v4 5/6] ACPI: Add _OST support for ACPI container hotplug

[Yasuaki Ishimatsu]

Hi Toshi,

2012/05/24 11:25, Toshi Kani wrote:
> Changed container_notify_cb() to call ACPI _OST method when ACPI
> container hotplug operation has completed. Slightly restructured
> the code with the same logic. The function sets eject_pending bit
> for an eject request since it does not initiate hot-remove operation.
> This bit is checked by the sysfs eject handler to determine if the
> request is originated from an ACPI eject notification.
> 
> Signed-off-by: Toshi Kani<toshi.kani@hp.com>
> ---
>   drivers/acpi/container.c |   43 ++++++++++++++++++++++++++++---------------
>   1 files changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/acpi/container.c b/drivers/acpi/container.c
> index 45cd03b..1f9f7d7 100644
> --- a/drivers/acpi/container.c
> +++ b/drivers/acpi/container.c
> @@ -158,9 +158,7 @@ static void container_notify_cb(acpi_handle handle, u32 type, void *context)
>   	int result;
>   	int present;
>   	acpi_status status;
> -
> -
> -	present = is_device_present(handle);
> +	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
> 
>   	switch (type) {
>   	case ACPI_NOTIFY_BUS_CHECK:
> @@ -169,32 +167,47 @@ static void container_notify_cb(acpi_handle handle, u32 type, void *context)
>   		printk(KERN_WARNING "Container driver received %s event\n",
>   		       (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_FAILURE(status) || !device) {
> -				result = container_device_add(&device, handle);
> -				if (!result)
> -					kobject_uevent(&device->dev.kobj,
> -						       KOBJ_ONLINE);
> -				else
> -					printk(KERN_WARNING
> -					       "Failed to add container\n");
> -			}
> -		} else {
> +		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);
> +				return;
>   			}
> +			break;
> +		}
> +

> +		if (!ACPI_FAILURE(status) || device)
> +			break;

The logic is not same as previous logic.
I think the following logic is correct.

		if (!ACPI_FAILURE(status) && device)
			break;
Thanks,
Yasuaki Ishimatsu

> +
> +		result = container_device_add(&device, handle);
> +		if (result) {
> +			printk(KERN_WARNING "Failed to add container\n");
> +			break;
>   		}
> +
> +		kobject_uevent(&device->dev.kobj, KOBJ_ONLINE);
> +		ost_code = ACPI_OST_SC_SUCCESS;
>   		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;
>   		}
>   		break;
> +
>   	default:
> -		break;
> +		/* non-hotplug event; possibly handled by other handler */
> +		return;
>   	}
> +
> +	/* Inform firmware that the hotplug operation has completed */
> +	(void) acpi_evaluate_hotplug_ost(handle, type, ost_code, NULL);
>   	return;
>   }
> 

Re: [PATCH v4 5/6] ACPI: Add _OST support for ACPI container hotplug

[Kani, Toshimitsu]

On Jun 4, 2012, at 9:40 PM, "Yasuaki Ishimatsu" <isimatu.yasuaki@jp.fujitsu.com> wrote:

> Hi Toshi,
> 
> 2012/05/24 11:25, Toshi Kani wrote:
>> Changed container_notify_cb() to call ACPI _OST method when ACPI
>> container hotplug operation has completed. Slightly restructured
>> the code with the same logic. The function sets eject_pending bit
>> for an eject request since it does not initiate hot-remove operation.
>> This bit is checked by the sysfs eject handler to determine if the
>> request is originated from an ACPI eject notification.
>> 
>> Signed-off-by: Toshi Kani<toshi.kani@hp.com>
>> ---
>>  drivers/acpi/container.c |   43 ++++++++++++++++++++++++++++---------------
>>  1 files changed, 28 insertions(+), 15 deletions(-)
>> 
>> diff --git a/drivers/acpi/container.c b/drivers/acpi/container.c
>> index 45cd03b..1f9f7d7 100644
>> --- a/drivers/acpi/container.c
>> +++ b/drivers/acpi/container.c
>> @@ -158,9 +158,7 @@ static void container_notify_cb(acpi_handle handle, u32 type, void *context)
>>      int result;
>>      int present;
>>      acpi_status status;
>> -
>> -
>> -    present = is_device_present(handle);
>> +    u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
>> 
>>      switch (type) {
>>      case ACPI_NOTIFY_BUS_CHECK:
>> @@ -169,32 +167,47 @@ static void container_notify_cb(acpi_handle handle, u32 type, void *context)
>>          printk(KERN_WARNING "Container driver received %s event\n",
>>                 (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_FAILURE(status) || !device) {
>> -                result = container_device_add(&device, handle);
>> -                if (!result)
>> -                    kobject_uevent(&device->dev.kobj,
>> -                               KOBJ_ONLINE);
>> -                else
>> -                    printk(KERN_WARNING
>> -                           "Failed to add container\n");
>> -            }
>> -        } else {
>> +        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);
>> +                return;
>>              }
>> +            break;
>> +        }
>> +
> 
>> +        if (!ACPI_FAILURE(status) || device)
>> +            break;
> 
> The logic is not same as previous logic.
> I think the following logic is correct.
> 
>        if (!ACPI_FAILURE(status) && device)
>            break;

Hi Yasuaki,

Great catch! You are right about that. Now the question is what this code should do when the call failed but the device gets set. This is rather hypothetical case, but I think it is safer to fail a hot add request whenever the device is set. What do you think?

Thanks,
-Toshi

> Thanks,
> Yasuaki Ishimatsu
> 
>> +
>> +        result = container_device_add(&device, handle);
>> +        if (result) {
>> +            printk(KERN_WARNING "Failed to add container\n");
>> +            break;
>>          }
>> +
>> +        kobject_uevent(&device->dev.kobj, KOBJ_ONLINE);
>> +        ost_code = ACPI_OST_SC_SUCCESS;
>>          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;
>>          }
>>          break;
>> +
>>      default:
>> -        break;
>> +        /* non-hotplug event; possibly handled by other handler */
>> +        return;
>>      }
>> +
>> +    /* Inform firmware that the hotplug operation has completed */
>> +    (void) acpi_evaluate_hotplug_ost(handle, type, ost_code, NULL);
>>      return;
>>  }
>> 
> 
> 
> --
> 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

Re: [PATCH v4 5/6] ACPI: Add _OST support for ACPI container hotplug

[Yasuaki Ishimatsu]

Hi Toshi,

Sorry for late reply.

2012/06/06 0:36, Kani, Toshimitsu wrote:
> On Jun 4, 2012, at 9:40 PM, "Yasuaki Ishimatsu"<isimatu.yasuaki@jp.fujitsu.com>  wrote:
>
>> Hi Toshi,
>>
>> 2012/05/24 11:25, Toshi Kani wrote:
>>> Changed container_notify_cb() to call ACPI _OST method when ACPI
>>> container hotplug operation has completed. Slightly restructured
>>> the code with the same logic. The function sets eject_pending bit
>>> for an eject request since it does not initiate hot-remove operation.
>>> This bit is checked by the sysfs eject handler to determine if the
>>> request is originated from an ACPI eject notification.
>>>
>>> Signed-off-by: Toshi Kani<toshi.kani@hp.com>
>>> ---
>>>   drivers/acpi/container.c |   43 ++++++++++++++++++++++++++++---------------
>>>   1 files changed, 28 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/acpi/container.c b/drivers/acpi/container.c
>>> index 45cd03b..1f9f7d7 100644
>>> --- a/drivers/acpi/container.c
>>> +++ b/drivers/acpi/container.c
>>> @@ -158,9 +158,7 @@ static void container_notify_cb(acpi_handle handle, u32 type, void *context)
>>>       int result;
>>>       int present;
>>>       acpi_status status;
>>> -
>>> -
>>> -    present = is_device_present(handle);
>>> +    u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
>>>
>>>       switch (type) {
>>>       case ACPI_NOTIFY_BUS_CHECK:
>>> @@ -169,32 +167,47 @@ static void container_notify_cb(acpi_handle handle, u32 type, void *context)
>>>           printk(KERN_WARNING "Container driver received %s event\n",
>>>                  (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_FAILURE(status) || !device) {
>>> -                result = container_device_add(&device, handle);
>>> -                if (!result)
>>> -                    kobject_uevent(&device->dev.kobj,
>>> -                               KOBJ_ONLINE);
>>> -                else
>>> -                    printk(KERN_WARNING
>>> -                           "Failed to add container\n");
>>> -            }
>>> -        } else {
>>> +        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);
>>> +                return;
>>>               }
>>> +            break;
>>> +        }
>>> +
>>
>>> +        if (!ACPI_FAILURE(status) || device)
>>> +            break;
>>
>> The logic is not same as previous logic.
>> I think the following logic is correct.
>>
>>         if (!ACPI_FAILURE(status)&&  device)
>>             break;
>
> Hi Yasuaki,
>
> Great catch! You are right about that. Now the question is what this code should do when the call failed but the device gets set. This is rather hypothetical case, but I think it is safer to fail a hot add request whenever the device is set. What do you think?
>

I think so, too.
So there is no comment on your patch.

Thanks,
Yasuaki Ishimatsu

> Thanks,
> -Toshi
>
>> Thanks,
>> Yasuaki Ishimatsu
>>
>>> +
>>> +        result = container_device_add(&device, handle);
>>> +        if (result) {
>>> +            printk(KERN_WARNING "Failed to add container\n");
>>> +            break;
>>>           }
>>> +
>>> +        kobject_uevent(&device->dev.kobj, KOBJ_ONLINE);
>>> +        ost_code = ACPI_OST_SC_SUCCESS;
>>>           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;
>>>           }
>>>           break;
>>> +
>>>       default:
>>> -        break;
>>> +        /* non-hotplug event; possibly handled by other handler */
>>> +        return;
>>>       }
>>> +
>>> +    /* Inform firmware that the hotplug operation has completed */
>>> +    (void) acpi_evaluate_hotplug_ost(handle, type, ost_code, NULL);
>>>       return;
>>>   }
>>>
>>
>>
>> --
>> 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
>
>

Re: [PATCH v4 5/6] ACPI: Add _OST support for ACPI container hotplug

[Kani, Toshimitsu]

On Jun 11, 2012, at 10:55 AM, "Yasuaki Ishimatsu" <isimatu.yasuaki@jp.fujitsu.com> wrote:

> Hi Toshi,
> 
> Sorry for late reply.
> 
> 2012/06/06 0:36, Kani, Toshimitsu wrote:
>> On Jun 4, 2012, at 9:40 PM, "Yasuaki Ishimatsu"<isimatu.yasuaki@jp.fujitsu.com>  wrote:
>> 
>>> Hi Toshi,
>>> 
>>> 2012/05/24 11:25, Toshi Kani wrote:
>>>> Changed container_notify_cb() to call ACPI _OST method when ACPI
>>>> container hotplug operation has completed. Slightly restructured
>>>> the code with the same logic. The function sets eject_pending bit
>>>> for an eject request since it does not initiate hot-remove operation.
>>>> This bit is checked by the sysfs eject handler to determine if the
>>>> request is originated from an ACPI eject notification.
>>>> 
>>>> Signed-off-by: Toshi Kani<toshi.kani@hp.com>
>>>> ---
>>>>  drivers/acpi/container.c |   43 ++++++++++++++++++++++++++++---------------
>>>>  1 files changed, 28 insertions(+), 15 deletions(-)
>>>> 
>>>> diff --git a/drivers/acpi/container.c b/drivers/acpi/container.c
>>>> index 45cd03b..1f9f7d7 100644
>>>> --- a/drivers/acpi/container.c
>>>> +++ b/drivers/acpi/container.c
>>>> @@ -158,9 +158,7 @@ static void container_notify_cb(acpi_handle handle, u32 type, void *context)
>>>>      int result;
>>>>      int present;
>>>>      acpi_status status;
>>>> -
>>>> -
>>>> -    present = is_device_present(handle);
>>>> +    u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
>>>> 
>>>>      switch (type) {
>>>>      case ACPI_NOTIFY_BUS_CHECK:
>>>> @@ -169,32 +167,47 @@ static void container_notify_cb(acpi_handle handle, u32 type, void *context)
>>>>          printk(KERN_WARNING "Container driver received %s event\n",
>>>>                 (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_FAILURE(status) || !device) {
>>>> -                result = container_device_add(&device, handle);
>>>> -                if (!result)
>>>> -                    kobject_uevent(&device->dev.kobj,
>>>> -                               KOBJ_ONLINE);
>>>> -                else
>>>> -                    printk(KERN_WARNING
>>>> -                           "Failed to add container\n");
>>>> -            }
>>>> -        } else {
>>>> +        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);
>>>> +                return;
>>>>              }
>>>> +            break;
>>>> +        }
>>>> +
>>> 
>>>> +        if (!ACPI_FAILURE(status) || device)
>>>> +            break;
>>> 
>>> The logic is not same as previous logic.
>>> I think the following logic is correct.
>>> 
>>>        if (!ACPI_FAILURE(status)&&  device)
>>>            break;
>> 
>> Hi Yasuaki,
>> 
>> Great catch! You are right about that. Now the question is what this code should do when the call failed but the device gets set. This is rather hypothetical case, but I think it is safer to fail a hot add request whenever the device is set. What do you think?
>> 
> 
> I think so, too.
> So there is no comment on your patch.
> 

Hi Yasuaki,

I will update the change log to clarify this change.
I am traveling now. I will work on the change when I am 
back on Jun 21.

Thanks,
-Toshi

> Thanks,
> Yasuaki Ishimatsu
> 
>> Thanks,
>> -Toshi
>> 
>>> Thanks,
>>> Yasuaki Ishimatsu
>>> 
>>>> +
>>>> +        result = container_device_add(&device, handle);
>>>> +        if (result) {
>>>> +            printk(KERN_WARNING "Failed to add container\n");
>>>> +            break;
>>>>          }
>>>> +
>>>> +        kobject_uevent(&device->dev.kobj, KOBJ_ONLINE);
>>>> +        ost_code = ACPI_OST_SC_SUCCESS;
>>>>          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;
>>>>          }
>>>>          break;
>>>> +
>>>>      default:
>>>> -        break;
>>>> +        /* non-hotplug event; possibly handled by other handler */
>>>> +        return;
>>>>      }
>>>> +
>>>> +    /* Inform firmware that the hotplug operation has completed */
>>>> +    (void) acpi_evaluate_hotplug_ost(handle, type, ost_code, NULL);
>>>>      return;
>>>>  }
>>>> 
>>> 
>>> 
>>> --
>>> 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-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 6/6] ACPI: Set hotplug _OST support bit to _OSC

[Toshi Kani]

When ACPI_HOTPLUG_OST is defined, set hotplug _OST support bit
OSC_SB_HOTPLUG_OST_SUPPORT to indicate that the OS supports hotplug
_OST by calling the platform-wide ACPI Operating System Capabilities
(_OSC).

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 drivers/acpi/bus.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 3188da3..3d4fc7a 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -548,6 +548,10 @@ static void acpi_bus_osc_support(void)
 	capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_PPC_OST_SUPPORT;
 #endif
 
+#ifdef ACPI_HOTPLUG_OST
+	capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_HOTPLUG_OST_SUPPORT;
+#endif
+
 	if (!ghes_disable)
 		capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_APEI_SUPPORT;
 	if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
-- 
1.7.7.6

Re: [PATCH v4 6/6] ACPI: Set hotplug _OST support bit to _OSC

[Shuah Khan]

On Wed, 2012-05-23 at 20:25 -0600, Toshi Kani wrote:
> When ACPI_HOTPLUG_OST is defined, set hotplug _OST support bit
> OSC_SB_HOTPLUG_OST_SUPPORT to indicate that the OS supports hotplug
> _OST by calling the platform-wide ACPI Operating System Capabilities
> (_OSC).
> 
> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> ---
>  drivers/acpi/bus.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 3188da3..3d4fc7a 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -548,6 +548,10 @@ static void acpi_bus_osc_support(void)
>  	capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_PPC_OST_SUPPORT;
>  #endif
>  
> +#ifdef ACPI_HOTPLUG_OST
> +	capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_HOTPLUG_OST_SUPPORT;
> +#endif

Why isn't firmware response to _OSC checked to make sure firmware also
supports _OST? My general comments about compile time switch that is
enabled only a few cases _OST Ejection/Insertion is intended to be used
are applicable here.

-- Shuah




> +
>  	if (!ghes_disable)
>  		capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_APEI_SUPPORT;
>  	if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))

Re: [PATCH v4 6/6] ACPI: Set hotplug _OST support bit to _OSC

[Toshi Kani]

On Thu, 2012-05-24 at 12:27 -0600, Shuah Khan wrote:
> On Wed, 2012-05-23 at 20:25 -0600, Toshi Kani wrote:
> > When ACPI_HOTPLUG_OST is defined, set hotplug _OST support bit
> > OSC_SB_HOTPLUG_OST_SUPPORT to indicate that the OS supports hotplug
> > _OST by calling the platform-wide ACPI Operating System Capabilities
> > (_OSC).
> > 
> > Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> > ---
> >  drivers/acpi/bus.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > index 3188da3..3d4fc7a 100644
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -548,6 +548,10 @@ static void acpi_bus_osc_support(void)
> >  	capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_PPC_OST_SUPPORT;
> >  #endif
> >  
> > +#ifdef ACPI_HOTPLUG_OST
> > +	capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_HOTPLUG_OST_SUPPORT;
> > +#endif
> 
> Why isn't firmware response to _OSC checked to make sure firmware also
> supports _OST? My general comments about compile time switch that is
> enabled only a few cases _OST Ejection/Insertion is intended to be used
> are applicable here.

This is because when FW does not support _OST, it does not implement
_OST method.  acpi_evaluate_hotplug_opt() simply returns with
AE_NOT_FOUND when _OST method does not exist.


Thanks for all good questions and thorough review!  Please let me know
if I did not address any of your comments properly.

Thanks,
-Toshi



> 
> -- Shuah
> 
> 
> 
> 
> > +
> >  	if (!ghes_disable)
> >  		capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_APEI_SUPPORT;
> >  	if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
> 
> 

Re: [PATCH v4 0/6] ACPI: Add _OST support for ACPI hotplug

[Shuah Khan]

On Wed, 2012-05-23 at 20:25 -0600, Toshi Kani wrote:
> This patchset supports ACPI OSPM Status Indication (_OST) method for
> ACPI CPU/memory/container hotplug operations and sysfs eject. After
> an ACPI hotplug operation has completed, OSPM calls _OST to indicate
> the result of the operation to the platform. If a platform does not
> support _OST, this patchset has no effect on the platform.
> 
> This _OST support is enabled when all relevant ACPI hotplug operations,
> such as CPU, memory and container hotplug, are enabled. This assures
> consistent behavior among the hotplug operations with regarding the
> _OST support.
> 
> Some platforms may require the OS to support _OST in order to support
> ACPI hotplug operations. For example, if a platform has the management
> console where user can request a hotplug operation from, this _OST
> support would be required for the management console to show the result
> of the hotplug request to user.
> 
> The _OST definition can be found in section 6.3.5 of ACPI 5.0 spec.
> The HPPF spec below also describes hotplug flows with _OST.
> 
>   DIG64 Hot-Plug & Partitioning Flow (HPPF) Specification R1.0
>   http://www.dig64.org/home/DIG64_HPPF_R1_0.pdf
> 
> The changes have been tested with simulated _OST methods.

Toshi,

First of all thanks for asking for my feedback. :) Having benefited from
reviewing the previous versions of this patch set, my thoughts on the
implementation have evolved.

I have some general comments first in the response, and please find code
specific comments on individual patches.

This patch set enables Insertion/Ejection _OST processing support which
will be a good addition since OS already supports it for Processor
Aggregator Device Support and _PPC.

However, in this case it is enabled as a compile time option and would
require a kernel build when firmware starts supporting _OST method in
some cases. Reference: PATCH v4 1/6. 

It also restricts the support to be all or nothing. i.e _OST is
supported only when all relevant hotplug operations are supported and
these need to be specifically enabled using the config options that
control it. For example, if a platform supports CPU_HOTPLUG and not
MEMORY_HOTPLUG, _OST support will be disabled even when firmware
supports it for cpus. Also the set of hotplug operations is limited as
_OST could be present in other hotplug cases such as PCI and PCIe.

I understand the spirit of this restriction that you are trying to limit
the exposure and it is a good goal. However, it probably could be
achieved in a way that doesn't shoehorn the implementation.

I think here are the goals, 

1. limit exposure so platforms that don't implement _OST are not
penalized evaluation _OST unnecessarily.

2. enable it when needed without requiring special compile time steps
and not worrying about sorting through various config options.

3. don't require all hotplug variants to be enabled in config, before OS
enables _OST support.

I see that you are enabling _OST evaluation and set the cap bit
OSC_SB_PPC_OST_SUPPORT only when ACPI_HOTPLUG_OST is defined. What
happens on when a kernel is configured with the config options that
enable ACPI_HOTPLUG_OST at compile time, and other hotplug options for
example CONFIG_HOTPLUG_PCI_PCIE, and CONFIG_HOTPLUG_PCI.

_OST is a platform capability and once OS tells firmware it can support
it, isn't it expected to call all _OST method if present under any
device object that is hotplug capable? What are the implications and
issues if OS doesn't evaluate _OST on PCI for example, once it tells the
firmware it supports _OST as a platform capability when it evaluates
_OSC? The question is, is it safe? This goes back to the first goal on
exposure.

Also, when _OSC is evaluated, the _OST code in this patch set, tells
firmware it supports _OST, however it doesn't check whether or not the
firmware actually acked the capability or not. Hence, it doesn't make
sure firmware has support for it, before it enables the _OST.

I think you will get closer to implementing a solution that achieves the
stated objectives by changing the design some as outlined below: ( I am
sure there are other ideas )

1. implement this similar to the way APEI support (OSC_SB_APEI_SUPPORT)
is implemented by checking the firmware ack and enabling the apei
support using a bool osc_sb_apei_support_acked which is only set when
firmware acknowledges back saying it can also support _OST. This ensures
that the feature is enabled only when both OS and firmware support it.
It will also get rid of the compile time restrictions.

2. Calling acpi_get_handle() on _OST prior to executing the method. This
will ensure that this method only gets run if it is present under the
device in question. Coupled with what is already outlined in #1 above,
now _OST gets executed only when it is defined under the device object.
Example case in the existing code, please see acpi_processor_ppc_ost()
implementation.

Please see other examples of _OST implementation in the kernel that
implement _OST for PAD and _PPC. These two examples will help you
understand my premise for these review comments.

Thanks,
-- Shuah

Re: [PATCH v4 0/6] ACPI: Add _OST support for ACPI hotplug

[Toshi Kani]

On Thu, 2012-05-24 at 11:34 -0600, Shuah Khan wrote:
> On Wed, 2012-05-23 at 20:25 -0600, Toshi Kani wrote:
> > This patchset supports ACPI OSPM Status Indication (_OST) method for
> > ACPI CPU/memory/container hotplug operations and sysfs eject. After
> > an ACPI hotplug operation has completed, OSPM calls _OST to indicate
> > the result of the operation to the platform. If a platform does not
> > support _OST, this patchset has no effect on the platform.
> > 
> > This _OST support is enabled when all relevant ACPI hotplug operations,
> > such as CPU, memory and container hotplug, are enabled. This assures
> > consistent behavior among the hotplug operations with regarding the
> > _OST support.
> > 
> > Some platforms may require the OS to support _OST in order to support
> > ACPI hotplug operations. For example, if a platform has the management
> > console where user can request a hotplug operation from, this _OST
> > support would be required for the management console to show the result
> > of the hotplug request to user.
> > 
> > The _OST definition can be found in section 6.3.5 of ACPI 5.0 spec.
> > The HPPF spec below also describes hotplug flows with _OST.
> > 
> >   DIG64 Hot-Plug & Partitioning Flow (HPPF) Specification R1.0
> >   http://www.dig64.org/home/DIG64_HPPF_R1_0.pdf
> > 
> > The changes have been tested with simulated _OST methods.
> 
> Toshi,
> 
> First of all thanks for asking for my feedback. :) Having benefited from
> reviewing the previous versions of this patch set, my thoughts on the
> implementation have evolved.

Thanks for reviewing! :)

> I have some general comments first in the response, and please find code
> specific comments on individual patches.
> 
> This patch set enables Insertion/Ejection _OST processing support which
> will be a good addition since OS already supports it for Processor
> Aggregator Device Support and _PPC.

Right.

> However, in this case it is enabled as a compile time option and would
> require a kernel build when firmware starts supporting _OST method in
> some cases. Reference: PATCH v4 1/6. 

Yes, it requires ACPI CPU, Memory and Container hotplug be enabled in the kernel.

> It also restricts the support to be all or nothing. i.e _OST is
> supported only when all relevant hotplug operations are supported and
> these need to be specifically enabled using the config options that
> control it. For example, if a platform supports CPU_HOTPLUG and not
> MEMORY_HOTPLUG, _OST support will be disabled even when firmware
> supports it for cpus. Also the set of hotplug operations is limited as
> _OST could be present in other hotplug cases such as PCI and PCIe.
>
> I understand the spirit of this restriction that you are trying to limit
> the exposure and it is a good goal. However, it probably could be
> achieved in a way that doesn't shoehorn the implementation.

This restriction is to assure that the OS is compliant with the ACPI
spec. When the OS calls _OSC with the hotplug _OST bit set, the OS needs
to support _OST for all relevant ACPI hotplug operations. Unfortunately,
this requires all relevant hotplug modules be enabled in the OS under
the current implementation.

For example, when the platform supports ACPI memory hotplug, but
ACPI_HOTPLUG_MEMORY is undefined in the OS, the OS needs to call _OSC
with the hotplug _OST bit unset. This is because the OS cannot receive
an ACPI notification to a memory object when ACPI_HOTPLUG_MEMORY is
undefined. Without the notify handler, we cannot call _OST.

A long term solution to address this issue is to have the system global
notify handler to receive all hotplug notifications, and call _OST
accordingly. However, it will require restructuring efforts which well
beyond the scope of this patchset. The email below describes this issue
and my thoughts on this.
http://marc.info/?l=linux-acpi&m=133546048929384&w=2

> I think here are the goals, 
> 
> 1. limit exposure so platforms that don't implement _OST are not
> penalized evaluation _OST unnecessarily.

This goal is met since the OS cannot evaluate _OST unless it is
implemented.

> 2. enable it when needed without requiring special compile time steps
> and not worrying about sorting through various config options.

I agree, but as I explained above, this is required to be compliant with
ACPI spec at this point. We can remove this restriction by improving the
notify handler design, but it will take more steps to do so.

> 3. don't require all hotplug variants to be enabled in config, before OS
> enables _OST support.

I agree, but the same reason above.

> I see that you are enabling _OST evaluation and set the cap bit
> OSC_SB_PPC_OST_SUPPORT only when ACPI_HOTPLUG_OST is defined. What
> happens on when a kernel is configured with the config options that
> enable ACPI_HOTPLUG_OST at compile time, and other hotplug options for
> example CONFIG_HOTPLUG_PCI_PCIE, and CONFIG_HOTPLUG_PCI.

Non-ACPI hotplug operations like PCIe native hotplug are irrelevant to _OST.

If user defines ACPI_HOTPLUG_OST at compile time, it forces the _OST to
be supported. It works fine as long as the kernel hotplug capabilities
match with the platform. 

> _OST is a platform capability and once OS tells firmware it can support
> it, isn't it expected to call all _OST method if present under any
> device object that is hotplug capable? What are the implications and
> issues if OS doesn't evaluate _OST on PCI for example, once it tells the
> firmware it supports _OST as a platform capability when it evaluates
> _OSC? The question is, is it safe? This goes back to the first goal on
> exposure.

Yes, and therefore, this _OST support is enabled when all relevant
hotplug operations are enabled.

> Also, when _OSC is evaluated, the _OST code in this patch set, tells
> firmware it supports _OST, however it doesn't check whether or not the
> firmware actually acked the capability or not. Hence, it doesn't make
> sure firmware has support for it, before it enables the _OST.

This step is not necessary because FW does not implement _OST method
when it does not support it.

> I think you will get closer to implementing a solution that achieves the
> stated objectives by changing the design some as outlined below: ( I am
> sure there are other ideas )
> 
> 1. implement this similar to the way APEI support (OSC_SB_APEI_SUPPORT)
> is implemented by checking the firmware ack and enabling the apei
> support using a bool osc_sb_apei_support_acked which is only set when
> firmware acknowledges back saying it can also support _OST. This ensures
> that the feature is enabled only when both OS and firmware support it.
> It will also get rid of the compile time restrictions.

APEI needs the OS to configure FW mode, so this acknowledge is
necessary. This step is not necessary for _OST.

> 2. Calling acpi_get_handle() on _OST prior to executing the method. This
> will ensure that this method only gets run if it is present under the
> device in question. Coupled with what is already outlined in #1 above,
> now _OST gets executed only when it is defined under the device object.
> Example case in the existing code, please see acpi_processor_ppc_ost()
> implementation.

Yes, I did look at acpi_processor_ppc_ost() when I implemented the
function.  I believe calling acpi_get_handle() is redundant since
acpi_ns_get_node() is called within acpi_evaluate_object() as well.
acpi_evaluate_object() simply returns with AE_NOT_FOUND when _OST method
does not exist.

> Please see other examples of _OST implementation in the kernel that
> implement _OST for PAD and _PPC. These two examples will help you
> understand my premise for these review comments.

I think acpi_processor_ppc_ost() has a bug since it calls _OST with 2
parameters. _OST is defined to have 3 parameters. When I called my _OST
method with 2 parameters for testing, it generated a warning message.  


Thanks,
-Toshi


> Thanks,
> -- Shuah
> 

RE: [PATCH v4 0/6] ACPI: Add _OST support for ACPI hotplug

[Moore, Robert]

> > 2. Calling acpi_get_handle() on _OST prior to executing the method.
> > This will ensure that this method only gets run if it is present
> under
> > the device in question. Coupled with what is already outlined in #1
> > above, now _OST gets executed only when it is defined under the
> device object.
> > Example case in the existing code, please see
> acpi_processor_ppc_ost()
> > implementation.
> 
> Yes, I did look at acpi_processor_ppc_ost() when I implemented the
> function.  I believe calling acpi_get_handle() is redundant since
> acpi_ns_get_node() is called within acpi_evaluate_object() as well.
> acpi_evaluate_object() simply returns with AE_NOT_FOUND when _OST
> method does not exist.
>

This is correct. If _OST does not exist, AE_NOT_FOUND will be returned from evaluate_object.




> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> owner@vger.kernel.org] On Behalf Of Toshi Kani
> Sent: Thursday, May 24, 2012 12:49 PM
> To: shuahkhan@gmail.com
> Cc: lenb@kernel.org; linux-acpi@vger.kernel.org; bhelgaas@google.com;
> liuj97@gmail.com; andi@firstfloor.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v4 0/6] ACPI: Add _OST support for ACPI hotplug
> 
> On Thu, 2012-05-24 at 11:34 -0600, Shuah Khan wrote:
> > On Wed, 2012-05-23 at 20:25 -0600, Toshi Kani wrote:
> > > This patchset supports ACPI OSPM Status Indication (_OST) method
> for
> > > ACPI CPU/memory/container hotplug operations and sysfs eject. After
> > > an ACPI hotplug operation has completed, OSPM calls _OST to
> indicate
> > > the result of the operation to the platform. If a platform does not
> > > support _OST, this patchset has no effect on the platform.
> > >
> > > This _OST support is enabled when all relevant ACPI hotplug
> > > operations, such as CPU, memory and container hotplug, are enabled.
> > > This assures consistent behavior among the hotplug operations with
> > > regarding the _OST support.
> > >
> > > Some platforms may require the OS to support _OST in order to
> > > support ACPI hotplug operations. For example, if a platform has the
> > > management console where user can request a hotplug operation from,
> > > this _OST support would be required for the management console to
> > > show the result of the hotplug request to user.
> > >
> > > The _OST definition can be found in section 6.3.5 of ACPI 5.0 spec.
> > > The HPPF spec below also describes hotplug flows with _OST.
> > >
> > >   DIG64 Hot-Plug & Partitioning Flow (HPPF) Specification R1.0
> > >   http://www.dig64.org/home/DIG64_HPPF_R1_0.pdf
> > >
> > > The changes have been tested with simulated _OST methods.
> >
> > Toshi,
> >
> > First of all thanks for asking for my feedback. :) Having benefited
> > from reviewing the previous versions of this patch set, my thoughts
> on
> > the implementation have evolved.
> 
> Thanks for reviewing! :)
> 
> > I have some general comments first in the response, and please find
> > code specific comments on individual patches.
> >
> > This patch set enables Insertion/Ejection _OST processing support
> > which will be a good addition since OS already supports it for
> > Processor Aggregator Device Support and _PPC.
> 
> Right.
> 
> > However, in this case it is enabled as a compile time option and
> would
> > require a kernel build when firmware starts supporting _OST method in
> > some cases. Reference: PATCH v4 1/6.
> 
> Yes, it requires ACPI CPU, Memory and Container hotplug be enabled in
> the kernel.
> 
> > It also restricts the support to be all or nothing. i.e _OST is
> > supported only when all relevant hotplug operations are supported and
> > these need to be specifically enabled using the config options that
> > control it. For example, if a platform supports CPU_HOTPLUG and not
> > MEMORY_HOTPLUG, _OST support will be disabled even when firmware
> > supports it for cpus. Also the set of hotplug operations is limited
> as
> > _OST could be present in other hotplug cases such as PCI and PCIe.
> >
> > I understand the spirit of this restriction that you are trying to
> > limit the exposure and it is a good goal. However, it probably could
> > be achieved in a way that doesn't shoehorn the implementation.
> 
> This restriction is to assure that the OS is compliant with the ACPI
> spec. When the OS calls _OSC with the hotplug _OST bit set, the OS
> needs to support _OST for all relevant ACPI hotplug operations.
> Unfortunately, this requires all relevant hotplug modules be enabled in
> the OS under the current implementation.
> 
> For example, when the platform supports ACPI memory hotplug, but
> ACPI_HOTPLUG_MEMORY is undefined in the OS, the OS needs to call _OSC
> with the hotplug _OST bit unset. This is because the OS cannot receive
> an ACPI notification to a memory object when ACPI_HOTPLUG_MEMORY is
> undefined. Without the notify handler, we cannot call _OST.
> 
> A long term solution to address this issue is to have the system global
> notify handler to receive all hotplug notifications, and call _OST
> accordingly. However, it will require restructuring efforts which well
> beyond the scope of this patchset. The email below describes this issue
> and my thoughts on this.
> http://marc.info/?l=linux-acpi&m=133546048929384&w=2
> 
> > I think here are the goals,
> >
> > 1. limit exposure so platforms that don't implement _OST are not
> > penalized evaluation _OST unnecessarily.
> 
> This goal is met since the OS cannot evaluate _OST unless it is
> implemented.
> 
> > 2. enable it when needed without requiring special compile time steps
> > and not worrying about sorting through various config options.
> 
> I agree, but as I explained above, this is required to be compliant
> with ACPI spec at this point. We can remove this restriction by
> improving the notify handler design, but it will take more steps to do
> so.
> 
> > 3. don't require all hotplug variants to be enabled in config, before
> > OS enables _OST support.
> 
> I agree, but the same reason above.
> 
> > I see that you are enabling _OST evaluation and set the cap bit
> > OSC_SB_PPC_OST_SUPPORT only when ACPI_HOTPLUG_OST is defined. What
> > happens on when a kernel is configured with the config options that
> > enable ACPI_HOTPLUG_OST at compile time, and other hotplug options
> for
> > example CONFIG_HOTPLUG_PCI_PCIE, and CONFIG_HOTPLUG_PCI.
> 
> Non-ACPI hotplug operations like PCIe native hotplug are irrelevant to
> _OST.
> 
> If user defines ACPI_HOTPLUG_OST at compile time, it forces the _OST to
> be supported. It works fine as long as the kernel hotplug capabilities
> match with the platform.
> 
> > _OST is a platform capability and once OS tells firmware it can
> > support it, isn't it expected to call all _OST method if present
> under
> > any device object that is hotplug capable? What are the implications
> > and issues if OS doesn't evaluate _OST on PCI for example, once it
> > tells the firmware it supports _OST as a platform capability when it
> > evaluates _OSC? The question is, is it safe? This goes back to the
> > first goal on exposure.
> 
> Yes, and therefore, this _OST support is enabled when all relevant
> hotplug operations are enabled.
> 
> > Also, when _OSC is evaluated, the _OST code in this patch set, tells
> > firmware it supports _OST, however it doesn't check whether or not
> the
> > firmware actually acked the capability or not. Hence, it doesn't make
> > sure firmware has support for it, before it enables the _OST.
> 
> This step is not necessary because FW does not implement _OST method
> when it does not support it.
> 
> > I think you will get closer to implementing a solution that achieves
> > the stated objectives by changing the design some as outlined below:
> (
> > I am sure there are other ideas )
> >
> > 1. implement this similar to the way APEI support
> > (OSC_SB_APEI_SUPPORT) is implemented by checking the firmware ack and
> > enabling the apei support using a bool osc_sb_apei_support_acked
> which
> > is only set when firmware acknowledges back saying it can also
> support
> > _OST. This ensures that the feature is enabled only when both OS and
> firmware support it.
> > It will also get rid of the compile time restrictions.
> 
> APEI needs the OS to configure FW mode, so this acknowledge is
> necessary. This step is not necessary for _OST.
> 
> > 2. Calling acpi_get_handle() on _OST prior to executing the method.
> > This will ensure that this method only gets run if it is present
> under
> > the device in question. Coupled with what is already outlined in #1
> > above, now _OST gets executed only when it is defined under the
> device object.
> > Example case in the existing code, please see
> acpi_processor_ppc_ost()
> > implementation.
> 
> Yes, I did look at acpi_processor_ppc_ost() when I implemented the
> function.  I believe calling acpi_get_handle() is redundant since
> acpi_ns_get_node() is called within acpi_evaluate_object() as well.
> acpi_evaluate_object() simply returns with AE_NOT_FOUND when _OST
> method does not exist.
> 
> > Please see other examples of _OST implementation in the kernel that
> > implement _OST for PAD and _PPC. These two examples will help you
> > understand my premise for these review comments.
> 
> I think acpi_processor_ppc_ost() has a bug since it calls _OST with 2
> parameters. _OST is defined to have 3 parameters. When I called my _OST
> method with 2 parameters for testing, it generated a warning message.
> 
> 
> Thanks,
> -Toshi
> 
> 
> > Thanks,
> > -- Shuah
> >
> 
> 
> --
> 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

RE: [PATCH v4 0/6] ACPI: Add _OST support for ACPI hotplug

[Shuah Khan]

On Tue, 2012-05-29 at 22:27 +0000, Moore, Robert wrote:
> > > 2. Calling acpi_get_handle() on _OST prior to executing the method.
> > > This will ensure that this method only gets run if it is present
> > under
> > > the device in question. Coupled with what is already outlined in #1
> > > above, now _OST gets executed only when it is defined under the
> > device object.
> > > Example case in the existing code, please see
> > acpi_processor_ppc_ost()
> > > implementation.
> > 
> > Yes, I did look at acpi_processor_ppc_ost() when I implemented the
> > function.  I believe calling acpi_get_handle() is redundant since
> > acpi_ns_get_node() is called within acpi_evaluate_object() as well.
> > acpi_evaluate_object() simply returns with AE_NOT_FOUND when _OST
> > method does not exist.
> >
> 
> This is correct. If _OST does not exist, AE_NOT_FOUND will be returned from evaluate_object.

Yes that is correct from the ACPI Spec and implementation point of view.
My thinking is that a call to acpi_get_handle() might not penalize the
OS as much as acpi_evaluate_object() would on systems that don't
actually implement _OST. In other words, acpi_get_handle() might not go
as deep as acpi_evaluate_object() would go into the ACPI layer, hence
might be a safer measure on platforms that don't actually implement this
optional method under all devices included in this patch set.

-- Shuah

RE: [PATCH v4 0/6] ACPI: Add _OST support for ACPI hotplug

[Toshi Kani]

On Tue, 2012-05-29 at 16:44 -0600, Shuah Khan wrote:
> On Tue, 2012-05-29 at 22:27 +0000, Moore, Robert wrote:
> > > > 2. Calling acpi_get_handle() on _OST prior to executing the method.
> > > > This will ensure that this method only gets run if it is present
> > > under
> > > > the device in question. Coupled with what is already outlined in #1
> > > > above, now _OST gets executed only when it is defined under the
> > > device object.
> > > > Example case in the existing code, please see
> > > acpi_processor_ppc_ost()
> > > > implementation.
> > > 
> > > Yes, I did look at acpi_processor_ppc_ost() when I implemented the
> > > function.  I believe calling acpi_get_handle() is redundant since
> > > acpi_ns_get_node() is called within acpi_evaluate_object() as well.
> > > acpi_evaluate_object() simply returns with AE_NOT_FOUND when _OST
> > > method does not exist.
> > >
> > 
> > This is correct. If _OST does not exist, AE_NOT_FOUND will be returned from evaluate_object.
> 
> Yes that is correct from the ACPI Spec and implementation point of view.
> My thinking is that a call to acpi_get_handle() might not penalize the
> OS as much as acpi_evaluate_object() would on systems that don't
> actually implement _OST. In other words, acpi_get_handle() might not go
> as deep as acpi_evaluate_object() would go into the ACPI layer, hence
> might be a safer measure on platforms that don't actually implement this
> optional method under all devices included in this patch set.
> 

I do not think we need to worry about it.  The code difference is not
that much, and this _OST path is limited to ACPI hotplug operations,
which are infrequent events.  Automatic workload balancing can make
frequent use of the operations, but is not frequent enough to make any
difference here.  I think simpler code works fine.

Thanks,
-Toshi

RE: [PATCH v4 0/6] ACPI: Add _OST support for ACPI hotplug

[Moore, Robert]

It does not make any difference. Essentially, a get_handle is performed by evaluate_object anyway.



>-----Original Message-----
>From: Toshi Kani [mailto:toshi.kani@hp.com]
>Sent: Tuesday, May 29, 2012 4:43 PM
>To: shuahkhan@gmail.com
>Cc: Moore, Robert; lenb@kernel.org; linux-acpi@vger.kernel.org;
>bhelgaas@google.com; liuj97@gmail.com; andi@firstfloor.org; linux-
>kernel@vger.kernel.org
>Subject: RE: [PATCH v4 0/6] ACPI: Add _OST support for ACPI hotplug
>
>On Tue, 2012-05-29 at 16:44 -0600, Shuah Khan wrote:
>> On Tue, 2012-05-29 at 22:27 +0000, Moore, Robert wrote:
>> > > > 2. Calling acpi_get_handle() on _OST prior to executing the method.
>> > > > This will ensure that this method only gets run if it is present
>> > > under
>> > > > the device in question. Coupled with what is already outlined in #1
>> > > > above, now _OST gets executed only when it is defined under the
>> > > device object.
>> > > > Example case in the existing code, please see
>> > > acpi_processor_ppc_ost()
>> > > > implementation.
>> > >
>> > > Yes, I did look at acpi_processor_ppc_ost() when I implemented the
>> > > function.  I believe calling acpi_get_handle() is redundant since
>> > > acpi_ns_get_node() is called within acpi_evaluate_object() as well.
>> > > acpi_evaluate_object() simply returns with AE_NOT_FOUND when _OST
>> > > method does not exist.
>> > >
>> >
>> > This is correct. If _OST does not exist, AE_NOT_FOUND will be returned
>from evaluate_object.
>>
>> Yes that is correct from the ACPI Spec and implementation point of view.
>> My thinking is that a call to acpi_get_handle() might not penalize the
>> OS as much as acpi_evaluate_object() would on systems that don't
>> actually implement _OST. In other words, acpi_get_handle() might not go
>> as deep as acpi_evaluate_object() would go into the ACPI layer, hence
>> might be a safer measure on platforms that don't actually implement this
>> optional method under all devices included in this patch set.
>>
>
>I do not think we need to worry about it.  The code difference is not
>that much, and this _OST path is limited to ACPI hotplug operations,
>which are infrequent events.  Automatic workload balancing can make
>frequent use of the operations, but is not frequent enough to make any
>difference here.  I think simpler code works fine.
>
>Thanks,
>-Toshi

RE: [PATCH v4 0/6] ACPI: Add _OST support for ACPI hotplug

[Toshi Kani]

On Wed, 2012-05-30 at 02:56 +0000, Moore, Robert wrote:
> It does not make any difference. Essentially, a get_handle is performed by evaluate_object anyway.
> 

Thanks Robert for the confirmation!
-Toshi


> >-----Original Message-----
> >From: Toshi Kani [mailto:toshi.kani@hp.com]
> >Sent: Tuesday, May 29, 2012 4:43 PM
> >To: shuahkhan@gmail.com
> >Cc: Moore, Robert; lenb@kernel.org; linux-acpi@vger.kernel.org;
> >bhelgaas@google.com; liuj97@gmail.com; andi@firstfloor.org; linux-
> >kernel@vger.kernel.org
> >Subject: RE: [PATCH v4 0/6] ACPI: Add _OST support for ACPI hotplug
> >
> >On Tue, 2012-05-29 at 16:44 -0600, Shuah Khan wrote:
> >> On Tue, 2012-05-29 at 22:27 +0000, Moore, Robert wrote:
> >> > > > 2. Calling acpi_get_handle() on _OST prior to executing the method.
> >> > > > This will ensure that this method only gets run if it is present
> >> > > under
> >> > > > the device in question. Coupled with what is already outlined in #1
> >> > > > above, now _OST gets executed only when it is defined under the
> >> > > device object.
> >> > > > Example case in the existing code, please see
> >> > > acpi_processor_ppc_ost()
> >> > > > implementation.
> >> > >
> >> > > Yes, I did look at acpi_processor_ppc_ost() when I implemented the
> >> > > function.  I believe calling acpi_get_handle() is redundant since
> >> > > acpi_ns_get_node() is called within acpi_evaluate_object() as well.
> >> > > acpi_evaluate_object() simply returns with AE_NOT_FOUND when _OST
> >> > > method does not exist.
> >> > >
> >> >
> >> > This is correct. If _OST does not exist, AE_NOT_FOUND will be returned
> >from evaluate_object.
> >>
> >> Yes that is correct from the ACPI Spec and implementation point of view.
> >> My thinking is that a call to acpi_get_handle() might not penalize the
> >> OS as much as acpi_evaluate_object() would on systems that don't
> >> actually implement _OST. In other words, acpi_get_handle() might not go
> >> as deep as acpi_evaluate_object() would go into the ACPI layer, hence
> >> might be a safer measure on platforms that don't actually implement this
> >> optional method under all devices included in this patch set.
> >>
> >
> >I do not think we need to worry about it.  The code difference is not
> >that much, and this _OST path is limited to ACPI hotplug operations,
> >which are infrequent events.  Automatic workload balancing can make
> >frequent use of the operations, but is not frequent enough to make any
> >difference here.  I think simpler code works fine.
> >
> >Thanks,
> >-Toshi
> 

Re: [PATCH v4 0/6] ACPI: Add _OST support for ACPI hotplug

[Shuah Khan]

On Thu, 2012-05-24 at 13:48 -0600, Toshi Kani wrote:
> On Thu, 2012-05-24 at 11:34 -0600, Shuah Khan wrote:
> > On Wed, 2012-05-23 at 20:25 -0600, Toshi Kani wrote:
> > > This patchset supports ACPI OSPM Status Indication (_OST) method for
> > > ACPI CPU/memory/container hotplug operations and sysfs eject. After
> > > an ACPI hotplug operation has completed, OSPM calls _OST to indicate
> > > the result of the operation to the platform. If a platform does not
> > > support _OST, this patchset has no effect on the platform.
> > > 
> > > This _OST support is enabled when all relevant ACPI hotplug operations,
> > > such as CPU, memory and container hotplug, are enabled. This assures
> > > consistent behavior among the hotplug operations with regarding the
> > > _OST support.
> > > 
> > > Some platforms may require the OS to support _OST in order to support
> > > ACPI hotplug operations. For example, if a platform has the management
> > > console where user can request a hotplug operation from, this _OST
> > > support would be required for the management console to show the result
> > > of the hotplug request to user.
> > > 
> > > The _OST definition can be found in section 6.3.5 of ACPI 5.0 spec.
> > > The HPPF spec below also describes hotplug flows with _OST.
> > > 
> > >   DIG64 Hot-Plug & Partitioning Flow (HPPF) Specification R1.0
> > >   http://www.dig64.org/home/DIG64_HPPF_R1_0.pdf
> > > 
> > > The changes have been tested with simulated _OST methods.
> > 
> > Toshi,
> > 
> > First of all thanks for asking for my feedback. :) Having benefited from
> > reviewing the previous versions of this patch set, my thoughts on the
> > implementation have evolved.
> 
> Thanks for reviewing! :)
> 
> > I have some general comments first in the response, and please find code
> > specific comments on individual patches.
> > 
> > This patch set enables Insertion/Ejection _OST processing support which
> > will be a good addition since OS already supports it for Processor
> > Aggregator Device Support and _PPC.
> 
> Right.
> 
> > However, in this case it is enabled as a compile time option and would
> > require a kernel build when firmware starts supporting _OST method in
> > some cases. Reference: PATCH v4 1/6. 
> 
> Yes, it requires ACPI CPU, Memory and Container hotplug be enabled in the kernel.
> 
> > It also restricts the support to be all or nothing. i.e _OST is
> > supported only when all relevant hotplug operations are supported and
> > these need to be specifically enabled using the config options that
> > control it. For example, if a platform supports CPU_HOTPLUG and not
> > MEMORY_HOTPLUG, _OST support will be disabled even when firmware
> > supports it for cpus. Also the set of hotplug operations is limited as
> > _OST could be present in other hotplug cases such as PCI and PCIe.
> >
> > I understand the spirit of this restriction that you are trying to limit
> > the exposure and it is a good goal. However, it probably could be
> > achieved in a way that doesn't shoehorn the implementation.
> 
> This restriction is to assure that the OS is compliant with the ACPI
> spec. When the OS calls _OSC with the hotplug _OST bit set, the OS needs
> to support _OST for all relevant ACPI hotplug operations. Unfortunately,
> this requires all relevant hotplug modules be enabled in the OS under
> the current implementation.
> 
> For example, when the platform supports ACPI memory hotplug, but
> ACPI_HOTPLUG_MEMORY is undefined in the OS, the OS needs to call _OSC
> with the hotplug _OST bit unset. This is because the OS cannot receive
> an ACPI notification to a memory object when ACPI_HOTPLUG_MEMORY is
> undefined. Without the notify handler, we cannot call _OST.
> 
> A long term solution to address this issue is to have the system global
> notify handler to receive all hotplug notifications, and call _OST
> accordingly. However, it will require restructuring efforts which well
> beyond the scope of this patchset. The email below describes this issue
> and my thoughts on this.
> http://marc.info/?l=linux-acpi&m=133546048929384&w=2
> 
> > I think here are the goals, 
> > 
> > 1. limit exposure so platforms that don't implement _OST are not
> > penalized evaluation _OST unnecessarily.
> 
> This goal is met since the OS cannot evaluate _OST unless it is
> implemented.
> 
> > 2. enable it when needed without requiring special compile time steps
> > and not worrying about sorting through various config options.
> 
> I agree, but as I explained above, this is required to be compliant with
> ACPI spec at this point. We can remove this restriction by improving the
> notify handler design, but it will take more steps to do so.
> 
> > 3. don't require all hotplug variants to be enabled in config, before OS
> > enables _OST support.
> 
> I agree, but the same reason above.
> 
> > I see that you are enabling _OST evaluation and set the cap bit
> > OSC_SB_PPC_OST_SUPPORT only when ACPI_HOTPLUG_OST is defined. What
> > happens on when a kernel is configured with the config options that
> > enable ACPI_HOTPLUG_OST at compile time, and other hotplug options for
> > example CONFIG_HOTPLUG_PCI_PCIE, and CONFIG_HOTPLUG_PCI.
> 
> Non-ACPI hotplug operations like PCIe native hotplug are irrelevant to _OST.

Yes I agree with your statement about PCIe native hot-plug operations.
However, as Jiang Liu pointed out in one of the reviews of an earlier
version of this patch set, _OST method has been defined in ACPI4.0 spec
and there are some platforms that already implement the _OST method. For
example,
Quanta QSSC-S4R server implements _OST for hot-pluggable PCI slots.

So, we do have one example of a server that implements it for
hot-pluggable PCI slots. Even if APCI PCI hotplug becomes legacy only,
it still needs to be supported.

Based on my reading of the ACPI 5.0 Spec, _OST method as it is defined
under the scope of Device Ejection/Insertion is applicable to not just
memory, cpu, container, and PCI slots, it could also be applicable
depending how a platform chooses implement it, "even in the cases of
docking and undocking mobile platforms to and from a peripheral
expansion dock." Reference: 6.3 of ACPI 5.0 Spec.

So I think it is wrong and narrow scoped to assume _OST will be and is
implemented only in the device ejection/insertion cases this patch set
addresses.

-- Shuah

Re: [PATCH v4 0/6] ACPI: Add _OST support for ACPI hotplug

[Toshi Kani]

On Wed, 2012-05-30 at 09:24 -0600, Shuah Khan wrote:
> On Thu, 2012-05-24 at 13:48 -0600, Toshi Kani wrote:
> > On Thu, 2012-05-24 at 11:34 -0600, Shuah Khan wrote:
> > > On Wed, 2012-05-23 at 20:25 -0600, Toshi Kani wrote:
> > > > This patchset supports ACPI OSPM Status Indication (_OST) method for
> > > > ACPI CPU/memory/container hotplug operations and sysfs eject. After
> > > > an ACPI hotplug operation has completed, OSPM calls _OST to indicate
> > > > the result of the operation to the platform. If a platform does not
> > > > support _OST, this patchset has no effect on the platform.
> > > > 
> > > > This _OST support is enabled when all relevant ACPI hotplug operations,
> > > > such as CPU, memory and container hotplug, are enabled. This assures
> > > > consistent behavior among the hotplug operations with regarding the
> > > > _OST support.
> > > > 
> > > > Some platforms may require the OS to support _OST in order to support
> > > > ACPI hotplug operations. For example, if a platform has the management
> > > > console where user can request a hotplug operation from, this _OST
> > > > support would be required for the management console to show the result
> > > > of the hotplug request to user.
> > > > 
> > > > The _OST definition can be found in section 6.3.5 of ACPI 5.0 spec.
> > > > The HPPF spec below also describes hotplug flows with _OST.
> > > > 
> > > >   DIG64 Hot-Plug & Partitioning Flow (HPPF) Specification R1.0
> > > >   http://www.dig64.org/home/DIG64_HPPF_R1_0.pdf
> > > > 
> > > > The changes have been tested with simulated _OST methods.
> > > 
> > > Toshi,
> > > 
> > > First of all thanks for asking for my feedback. :) Having benefited from
> > > reviewing the previous versions of this patch set, my thoughts on the
> > > implementation have evolved.
> > 
> > Thanks for reviewing! :)
> > 
> > > I have some general comments first in the response, and please find code
> > > specific comments on individual patches.
> > > 
> > > This patch set enables Insertion/Ejection _OST processing support which
> > > will be a good addition since OS already supports it for Processor
> > > Aggregator Device Support and _PPC.
> > 
> > Right.
> > 
> > > However, in this case it is enabled as a compile time option and would
> > > require a kernel build when firmware starts supporting _OST method in
> > > some cases. Reference: PATCH v4 1/6. 
> > 
> > Yes, it requires ACPI CPU, Memory and Container hotplug be enabled in the kernel.
> > 
> > > It also restricts the support to be all or nothing. i.e _OST is
> > > supported only when all relevant hotplug operations are supported and
> > > these need to be specifically enabled using the config options that
> > > control it. For example, if a platform supports CPU_HOTPLUG and not
> > > MEMORY_HOTPLUG, _OST support will be disabled even when firmware
> > > supports it for cpus. Also the set of hotplug operations is limited as
> > > _OST could be present in other hotplug cases such as PCI and PCIe.
> > >
> > > I understand the spirit of this restriction that you are trying to limit
> > > the exposure and it is a good goal. However, it probably could be
> > > achieved in a way that doesn't shoehorn the implementation.
> > 
> > This restriction is to assure that the OS is compliant with the ACPI
> > spec. When the OS calls _OSC with the hotplug _OST bit set, the OS needs
> > to support _OST for all relevant ACPI hotplug operations. Unfortunately,
> > this requires all relevant hotplug modules be enabled in the OS under
> > the current implementation.
> > 
> > For example, when the platform supports ACPI memory hotplug, but
> > ACPI_HOTPLUG_MEMORY is undefined in the OS, the OS needs to call _OSC
> > with the hotplug _OST bit unset. This is because the OS cannot receive
> > an ACPI notification to a memory object when ACPI_HOTPLUG_MEMORY is
> > undefined. Without the notify handler, we cannot call _OST.
> > 
> > A long term solution to address this issue is to have the system global
> > notify handler to receive all hotplug notifications, and call _OST
> > accordingly. However, it will require restructuring efforts which well
> > beyond the scope of this patchset. The email below describes this issue
> > and my thoughts on this.
> > http://marc.info/?l=linux-acpi&m=133546048929384&w=2
> > 
> > > I think here are the goals, 
> > > 
> > > 1. limit exposure so platforms that don't implement _OST are not
> > > penalized evaluation _OST unnecessarily.
> > 
> > This goal is met since the OS cannot evaluate _OST unless it is
> > implemented.
> > 
> > > 2. enable it when needed without requiring special compile time steps
> > > and not worrying about sorting through various config options.
> > 
> > I agree, but as I explained above, this is required to be compliant with
> > ACPI spec at this point. We can remove this restriction by improving the
> > notify handler design, but it will take more steps to do so.
> > 
> > > 3. don't require all hotplug variants to be enabled in config, before OS
> > > enables _OST support.
> > 
> > I agree, but the same reason above.
> > 
> > > I see that you are enabling _OST evaluation and set the cap bit
> > > OSC_SB_PPC_OST_SUPPORT only when ACPI_HOTPLUG_OST is defined. What
> > > happens on when a kernel is configured with the config options that
> > > enable ACPI_HOTPLUG_OST at compile time, and other hotplug options for
> > > example CONFIG_HOTPLUG_PCI_PCIE, and CONFIG_HOTPLUG_PCI.
> > 
> > Non-ACPI hotplug operations like PCIe native hotplug are irrelevant to _OST.
> 
> Yes I agree with your statement about PCIe native hot-plug operations.
> However, as Jiang Liu pointed out in one of the reviews of an earlier
> version of this patch set, _OST method has been defined in ACPI4.0 spec
> and there are some platforms that already implement the _OST method. For
> example,
> Quanta QSSC-S4R server implements _OST for hot-pluggable PCI slots.

This means this server is already working fine without Linux's _OST
support or with some private patch.

> So, we do have one example of a server that implements it for
> hot-pluggable PCI slots. Even if APCI PCI hotplug becomes legacy only,
> it still needs to be supported.

This can be incremental effort if we indeed need _OST support for legacy
ACPI hotplug.  Jiang and I had also agreed on this.

> Based on my reading of the ACPI 5.0 Spec, _OST method as it is defined
> under the scope of Device Ejection/Insertion is applicable to not just
> memory, cpu, container, and PCI slots, it could also be applicable
> depending how a platform chooses implement it, "even in the cases of
> docking and undocking mobile platforms to and from a peripheral
> expansion dock." Reference: 6.3 of ACPI 5.0 Spec.

The OS-FW interface of docking / undocking operations is fairly
well-established with unique _DCK method.  It is not clear to me if
there is any need to modify the current interface with _OST.  There
might be such need in future, but I do not want to mess up with this
procedure with my speculation.  

> So I think it is wrong and narrow scoped to assume _OST will be and is
> implemented only in the device ejection/insertion cases this patch set
> addresses.

I agree with your concerns.  However, this patchset is the first step,
not the final step.  This first step is targeted to support the _OST
use-cases defined in the DIG64 HPPF spec.  We can continue to enhance it
as we find more needs.  I am willing to help anyone who has plan to
implement _OST for other use-cases.

Thanks,
-Toshi 


> -- Shuah
> 
>