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

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

[Toshi Kani]

This series of patches supports ACPI _OST (OSPM Status Indication)
method for ACPI-based CPU and memory hotplug operations.  After
ACPI-based hotplug operation completed, OSPM calls _OST to convey
the completion status to ACPI firmware.  If _OST is not present,
this change has no effect on the platform.

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 change was tested by overriding DSDT with fake _OST methods.

---
Toshi Kani (4):
 ACPI: Set hotplug _OST support bit to _OSC
 ACPI: Add acpi_evaluate_ost() for calling _OST
 ACPI: Add _OST support for ACPI CPU hotplug
 ACPI: Add _OST support for ACPI memory hotplug

 drivers/acpi/acpi_memhotplug.c  |   43 +++++++++++++++++++++++++++++----------
 drivers/acpi/bus.c              |    5 ++++
 drivers/acpi/processor_driver.c |   28 ++++++++++++++++++-------
 drivers/acpi/utils.c            |   34 ++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h         |    3 ++
 include/linux/acpi.h            |   26 ++++++++++++++++++++++-
 6 files changed, 119 insertions(+), 20 deletions(-)

[PATCH 1/4] ACPI: Set hotplug _OST support bit to _OSC

[Toshi Kani]

Added macro definitions of _OST source event 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.

This patch sets OSC_SB_HOTPLUG_OST_SUPPORT to the platform-wide
OSPM capabilities when CONFIG option of ACPI CPU hotplug or memory
hotplug is enabled.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 drivers/acpi/bus.c   |    5 +++++
 include/linux/acpi.h |   26 +++++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 3263b68..a492d64 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -544,6 +544,11 @@ static void acpi_bus_osc_support(void)
 	capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_PPC_OST_SUPPORT;
 #endif
 
+#if defined(CONFIG_ACPI_HOTPLUG_CPU) || defined(CONFIG_ACPI_HOTPLUG_MEMORY) ||\
+		 defined(CONFIG_ACPI_HOTPLUG_MEMORY_MODULE)
+	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)))
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index f421dd8..4127df8 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,30 @@ extern bool osc_sb_apei_support_acked;
 
 extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
 					     u32 *mask, u32 req);
+
+/* _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

[PATCH 2/4] ACPI: Add acpi_evaluate_ost() for calling _OST

[Toshi Kani]

Added a new function, acpi_evaluate_ost(), to utils.c.  This
function can be called from ACPI-based drivers / modules to
invoke ACPI _OST method.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 drivers/acpi/utils.c    |   34 ++++++++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h |    3 +++
 2 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index b002a47..09b48c3 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -382,3 +382,37 @@ acpi_evaluate_reference(acpi_handle handle,
 }
 
 EXPORT_SYMBOL(acpi_evaluate_reference);
+
+/*
+ * acpi_evaluate_ost: Evaluate ACPI OSPM Status Indication method
+ * @handle: ACPI device handle
+ * @source_event: source event code
+ * @status_code: status code
+ * @status_buf: optional detailed information (NULL if none)
+ */
+acpi_status
+acpi_evaluate_ost(acpi_handle handle, u32 source_event, u32 status_code,
+		struct acpi_buffer *status_buf)
+{
+	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;
+}
+EXPORT_SYMBOL(acpi_evaluate_ost);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index f1c8ca6..0dac8cb 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_ost(acpi_handle handle, u32 source_event, u32 status_code,
+			struct acpi_buffer *status_buf);
 
 #ifdef CONFIG_ACPI
 
-- 
1.7.7.6

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

[Toshi Kani]

Changed acpi_processor_hotplug_notify() to call ACPI _OST method
when ACPI-based CPU hotplug operation 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..5d9b5a8 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_ost(handle, event, ost_code, NULL);
 	return;
 }
 
-- 
1.7.7.6

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

[Toshi Kani]

Changed acpi_memory_device_notify() to call ACPI _OST method
when ACPI-based memory hotplug operation 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..4682418 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_ost(handle, event, ost_code, NULL);
 	return;
 }
 
-- 
1.7.7.6

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

[Shuah Khan]

On Tue, 2012-04-10 at 16:21 -0600, Toshi Kani wrote:
> This series of patches supports ACPI _OST (OSPM Status Indication)
> method for ACPI-based CPU and memory hotplug operations.  After
> ACPI-based hotplug operation completed, OSPM calls _OST to convey
> the completion status to ACPI firmware.  If _OST is not present,
> this change has no effect on the platform.
> 
> 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 change was tested by overriding DSDT with fake _OST methods.

Could you please elaborate what it means by fake _OST method? I am
assuming based on the above that this patch set was never tested on real
hardware that has support for _OST method? Is that correct? If so why
not wait until you have the opportunity to test it on real hardware
before sending the patch?

-- Shuah
> 
> ---
> Toshi Kani (4):
>  ACPI: Set hotplug _OST support bit to _OSC
>  ACPI: Add acpi_evaluate_ost() for calling _OST
>  ACPI: Add _OST support for ACPI CPU hotplug
>  ACPI: Add _OST support for ACPI memory hotplug
> 
>  drivers/acpi/acpi_memhotplug.c  |   43 +++++++++++++++++++++++++++++----------
>  drivers/acpi/bus.c              |    5 ++++
>  drivers/acpi/processor_driver.c |   28 ++++++++++++++++++-------
>  drivers/acpi/utils.c            |   34 ++++++++++++++++++++++++++++++
>  include/acpi/acpi_bus.h         |    3 ++
>  include/linux/acpi.h            |   26 ++++++++++++++++++++++-
>  6 files changed, 119 insertions(+), 20 deletions(-)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

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

[Toshi Kani]

On Wed, 2012-04-11 at 10:33 -0600, Shuah Khan wrote:
> On Tue, 2012-04-10 at 16:21 -0600, Toshi Kani wrote:
> > This series of patches supports ACPI _OST (OSPM Status Indication)
> > method for ACPI-based CPU and memory hotplug operations.  After
> > ACPI-based hotplug operation completed, OSPM calls _OST to convey
> > the completion status to ACPI firmware.  If _OST is not present,
> > this change has no effect on the platform.
> > 
> > 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 change was tested by overriding DSDT with fake _OST methods.
> 
> Could you please elaborate what it means by fake _OST method? I am
> assuming based on the above that this patch set was never tested on real
> hardware that has support for _OST method? Is that correct? If so why
> not wait until you have the opportunity to test it on real hardware
> before sending the patch?

Hi Shuah,

The fake _OST methods print arguments to verify successful execution of
the methods.  Since the purpose of _OST is firmware-internal
communication, there is no difference in testing on real firmware from
the OS perspective.  Overriding DSDT with fake ACPI methods is a
well-established and widely used testing method as well.

Pre-enablement of new features will benefit us because:

 1. There is a timeline gap between support in the upstream kernel and
distribution kernels.  Also, a new platform often needs to support older
stable kernels as well.

 2. Pre-enablement in the OS allows platform vendors to design new
platforms that utilize the new feature.

Thanks,
-Toshi


> -- Shuah
> > 
> > ---
> > Toshi Kani (4):
> >  ACPI: Set hotplug _OST support bit to _OSC
> >  ACPI: Add acpi_evaluate_ost() for calling _OST
> >  ACPI: Add _OST support for ACPI CPU hotplug
> >  ACPI: Add _OST support for ACPI memory hotplug
> > 
> >  drivers/acpi/acpi_memhotplug.c  |   43 +++++++++++++++++++++++++++++----------
> >  drivers/acpi/bus.c              |    5 ++++
> >  drivers/acpi/processor_driver.c |   28 ++++++++++++++++++-------
> >  drivers/acpi/utils.c            |   34 ++++++++++++++++++++++++++++++
> >  include/acpi/acpi_bus.h         |    3 ++
> >  include/linux/acpi.h            |   26 ++++++++++++++++++++++-
> >  6 files changed, 119 insertions(+), 20 deletions(-)
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> > 
> 
> 

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

[Shuah Khan]

> Hi Shuah,
> 
> The fake _OST methods print arguments to verify successful execution of
> the methods.  Since the purpose of _OST is firmware-internal
> communication, there is no difference in testing on real firmware from
> the OS perspective.  Overriding DSDT with fake ACPI methods is a
> well-established and widely used testing method as well.

Toshi,

This patch hasn't gone through sufficient new functionality testing as
the only testing that was done was exercising it with stubbed out
firmware calls. I am not clear on how much regression testing this patch
has undergone. Sorry, it doesn't give me the warm and fuzzies.

-- Shuah

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

[Jiang Liu]

Hi Shuah and Toshi,
	As I can tell, _OST method has been defined in ACPI4.0 spec and
there are some platforms already implemented the _OST method. For example,
Quanta QSSC-S4R server implements _OST for hot-pluggable PCI slots, but 
it's a little pity that it doesn't implement _OST for the memory board,
so we can't test Toshi's patch on that platform.
	According to my understanding, other than notifying BIOS about the
event processing result, _OST could also be used to notify BIOS about the
IN_PROGRESS state, so BIOS could track the event handling progress and avoid
timeout. We have used _OST method to avoid timeout on one of our prototype
platforms.

On 04/12/2012 10:19 PM, Shuah Khan wrote:
> 
>> Hi Shuah,
>>
>> The fake _OST methods print arguments to verify successful execution of
>> the methods.  Since the purpose of _OST is firmware-internal
>> communication, there is no difference in testing on real firmware from
>> the OS perspective.  Overriding DSDT with fake ACPI methods is a
>> well-established and widely used testing method as well.
> 
> Toshi,
> 
> This patch hasn't gone through sufficient new functionality testing as
> the only testing that was done was exercising it with stubbed out
> firmware calls. I am not clear on how much regression testing this patch
> has undergone. Sorry, it doesn't give me the warm and fuzzies.
> 
> -- 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 0/5] ACPI: Add _OST support for ACPI hotplug

[Shuah Khan]

On Fri, 2012-04-13 at 22:24 +0800, Jiang Liu wrote:
> Hi Shuah and Toshi,
> 	As I can tell, _OST method has been defined in ACPI4.0 spec and
> there are some platforms already implemented the _OST method. For example,
> Quanta QSSC-S4R server implements _OST for hot-pluggable PCI slots, but 
> it's a little pity that it doesn't implement _OST for the memory board,
> so we can't test Toshi's patch on that platform.
> 	According to my understanding, other than notifying BIOS about the
> event processing result, _OST could also be used to notify BIOS about the
> IN_PROGRESS state, so BIOS could track the event handling progress and avoid
> timeout. We have used _OST method to avoid timeout on one of our prototype
> platforms.

Thanks Jiang. This is good information about _OST method's timeout
use-case. Alos good to know there are platforms out there that already
implement it. Too bad you don't have any platforms to test Toshis's
patch. Maybe there are others that have one available.

-- Shuah

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

[Toshi Kani]

On Fri, 2012-04-13 at 09:23 -0600, Shuah Khan wrote:
> On Fri, 2012-04-13 at 22:24 +0800, Jiang Liu wrote:
> > Hi Shuah and Toshi,
> > 	As I can tell, _OST method has been defined in ACPI4.0 spec and
> > there are some platforms already implemented the _OST method. For example,
> > Quanta QSSC-S4R server implements _OST for hot-pluggable PCI slots, but 
> > it's a little pity that it doesn't implement _OST for the memory board,
> > so we can't test Toshi's patch on that platform.
> > 	According to my understanding, other than notifying BIOS about the
> > event processing result, _OST could also be used to notify BIOS about the
> > IN_PROGRESS state, so BIOS could track the event handling progress and avoid
> > timeout. We have used _OST method to avoid timeout on one of our prototype
> > platforms.
> 
> Thanks Jiang. This is good information about _OST method's timeout
> use-case. Alos good to know there are platforms out there that already
> implement it. Too bad you don't have any platforms to test Toshis's
> patch. Maybe there are others that have one available.

Thanks Jiang for the valuable information!

I would like to explain more detail about my testing.  Unlike other ACPI
methods, the functionality of _OST method is not defined by the spec.
It is all up to the firmware implementation.  For instance, ACPI spec
describes the following example _OST.  It checks Arg0 and Arg1, and
conditionally stores values to control LED.  Other possible
implementation may be for the firmware to manage the progress /
completion of the operation.  In any case, the functionality is a black
box to the OS.

Method(_OST, 3, Serialized) {
                :
  Switch(And(Arg0,0xFF)) // Mask to retain low byte
  {
     Case(0x03)              // Ejection request
     {
        Switch(Arg1)
        {
           Case(Package(){0x80, 0x81, 0x82, 0x83})
           { // Ejection Failure for some reason
             Store(Zero, ^^S3LE) // Turn off Ejection Progress LED
             Store(One, ^^S3LF)  // Turn on Ejection Failure LED
           }
           Case(0x84) // Eject request pending
           {
             Store(One, ^^S3LE)  // Turn on Ejection Request LED
             Store(Zero, ^^S3LF) // Turn off Ejection Failure LED
           }
        }
     }
  }
}

My emulated _OST method is simple, but it essentially does a similar
operation in AML.  Store to Debug is displayed to the console when
aml_debug_output is enabled.  As far as the OS testing is concerned, it
verifies successful execution of the method.

Method (_OST, 3, NotSerialized)
{
     Store ("AML> _OST", Debug)
     Store (Arg0, Debug)
     Store (Arg1, Debug)
     Store (Arg2, Debug)
}

I have tested the emulated _OST method on both CPU and Memory hotplug
operations.  For memory hotplug, I have followed the steps below to
emulate memory hotplug operation as well (some details needed to be
updated for the latest kernel).  It also overrides DSDT to create a
memory device object, and emulates a SCI for the memory hotplug event. 
http://www.spinics.net/lists/hotplug/msg00613.html

Unfortunately, I do not have access to a platform that implements _OST
today.  If anyone has access to such platform, I am willing to work
together for the testing.  Also, if testing on emulated environment is
not adequate, please let me know.  I am open to the suggestion, and
would like to improve my test procedure going forward.

Thanks,
-Toshi

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

[Shuah Khan]

> I would like to explain more detail about my testing.  Unlike other ACPI
> methods, the functionality of _OST method is not defined by the spec.
> It is all up to the firmware implementation.  

A good reason why this patch requires more testing than on simulated
firmware code that is based on one single firmware implementation.

-- Shuah

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

[Toshi Kani]

On Fri, 2012-04-13 at 12:34 -0600, Shuah Khan wrote:
> > I would like to explain more detail about my testing.  Unlike other ACPI
> > methods, the functionality of _OST method is not defined by the spec.
> > It is all up to the firmware implementation.  
> 
> A good reason why this patch requires more testing than on simulated
> firmware code that is based on one single firmware implementation.

Thanks Shuah for the guidance.  Do they summarize your point properly?

1. Testing a new feature on simulated / virtualized environment alone is
not sufficient.

2. Pre-enablement of a new feature is not allowed.

3. Contributing code that supports a new feature shall wait until there
are multiple firmware implementations available for the testing.

I like clear guidance, but this may be challenging for emerging
features.
-Toshi 

Re: [PATCH 1/4] ACPI: Set hotplug _OST support bit to _OSC

[Bjorn Helgaas]

On Tue, Apr 10, 2012 at 4:21 PM, Toshi Kani <toshi.kani@hp.com> wrote:
> Added macro definitions of _OST source event 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.
>
> This patch sets OSC_SB_HOTPLUG_OST_SUPPORT to the platform-wide
> OSPM capabilities when CONFIG option of ACPI CPU hotplug or memory
> hotplug is enabled.
>
> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> ---
>  drivers/acpi/bus.c   |    5 +++++
>  include/linux/acpi.h |   26 +++++++++++++++++++++++++-
>  2 files changed, 30 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 3263b68..a492d64 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -544,6 +544,11 @@ static void acpi_bus_osc_support(void)
>        capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_PPC_OST_SUPPORT;
>  #endif
>
> +#if defined(CONFIG_ACPI_HOTPLUG_CPU) || defined(CONFIG_ACPI_HOTPLUG_MEMORY) ||\
> +                defined(CONFIG_ACPI_HOTPLUG_MEMORY_MODULE)
> +       capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_HOTPLUG_OST_SUPPORT;
> +#endif

This seems a bit strange to me.  For one thing, the _OSC discussion
doesn't seem to indicate that _OST support is specific to CPU or
memory hotplug.  If we tell the platform that we support _OST, the
platform can assume that we'll evaluate _OST for *any* device, which
is not the case.  I guess this is just another reason why we need
hotplug support in the ACPI core, not in the individual drivers.  Then
we wouldn't have the ifdefs at all.

The second thing is that the kernel should be correct at every point
in a patch series.  In this case, if only patch [1/4] is applied, we
claim to support _OST, but we actually don't.  So I think the patches
that add support for evaluating _OST should be first, and this one
should be last.  Or even better, a single patch could enable _OST
evaluation and add this _OSC bit at the same time.

Bjorn

> +
>        if (!ghes_disable)
>                capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_APEI_SUPPORT;
>        if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index f421dd8..4127df8 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,30 @@ extern bool osc_sb_apei_support_acked;
>
>  extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
>                                             u32 *mask, u32 req);
> +
> +/* _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
>
> --
> 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

Re: [PATCH 1/4] ACPI: Set hotplug _OST support bit to _OSC

[Bjorn Helgaas]

On Tue, Apr 10, 2012 at 4:21 PM, Toshi Kani <toshi.kani@hp.com> wrote:
> Added macro definitions of _OST source event 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.
>
> This patch sets OSC_SB_HOTPLUG_OST_SUPPORT to the platform-wide
> OSPM capabilities when CONFIG option of ACPI CPU hotplug or memory
> hotplug is enabled.
>
> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> ---
>  drivers/acpi/bus.c   |    5 +++++
>  include/linux/acpi.h |   26 +++++++++++++++++++++++++-
>  2 files changed, 30 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 3263b68..a492d64 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -544,6 +544,11 @@ static void acpi_bus_osc_support(void)
>        capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_PPC_OST_SUPPORT;
>  #endif
>
> +#if defined(CONFIG_ACPI_HOTPLUG_CPU) || defined(CONFIG_ACPI_HOTPLUG_MEMORY) ||\
> +                defined(CONFIG_ACPI_HOTPLUG_MEMORY_MODULE)
> +       capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_HOTPLUG_OST_SUPPORT;
> +#endif

This seems a bit strange to me.  For one thing, the _OSC discussion
doesn't seem to indicate that _OST support is specific to CPU or
memory hotplug.  If we tell the platform that we support _OST, the
platform can assume that we'll evaluate _OST for *any* device, which
is not the case.  I guess this is just another reason why we need
hotplug support in the ACPI core, not in the individual drivers.  Then
we wouldn't have the ifdefs at all.

The second thing is that the kernel should be correct at every point
in a patch series.  In this case, if only patch [1/4] is applied, we
claim to support _OST, but we actually don't.  So I think the patches
that add support for evaluating _OST should be first, and this one
should be last.  Or even better, a single patch could enable _OST
evaluation and add this _OSC bit at the same time.

Bjorn

> +
>        if (!ghes_disable)
>                capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_APEI_SUPPORT;
>        if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index f421dd8..4127df8 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,30 @@ extern bool osc_sb_apei_support_acked;
>
>  extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
>                                             u32 *mask, u32 req);
> +
> +/* _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
>
> --
> 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 3/4] ACPI: Add _OST support for ACPI CPU hotplug

[Bjorn Helgaas]

On Tue, Apr 10, 2012 at 4:21 PM, Toshi Kani <toshi.kani@hp.com> wrote:
> Changed acpi_processor_hotplug_notify() to call ACPI _OST method
> when ACPI-based CPU hotplug operation 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..5d9b5a8 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");

You didn't add this problem, but since you're touching the code, I'll
point it out.  This message will look to the user like:

    ACPI: Unable to add the device

which is useless.  The user has no idea what device we're talking
about or why we can't add it, so he can't *do* anything with the
message.  Almost every printk of a constant string has this problem.
Using dev_err() instead of printk() would help a lot, as would
including the error code (result).  Of course, this would be a
separate patch that would update all the messages in the driver at the
same time.

>                        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_ost(handle, event, ost_code, NULL);
>        return;
>  }
>
> --
> 1.7.7.6
>
> --
> 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

Re: [PATCH 3/4] ACPI: Add _OST support for ACPI CPU hotplug

[Bjorn Helgaas]

On Tue, Apr 10, 2012 at 4:21 PM, Toshi Kani <toshi.kani@hp.com> wrote:
> Changed acpi_processor_hotplug_notify() to call ACPI _OST method
> when ACPI-based CPU hotplug operation 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..5d9b5a8 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");

You didn't add this problem, but since you're touching the code, I'll
point it out.  This message will look to the user like:

    ACPI: Unable to add the device

which is useless.  The user has no idea what device we're talking
about or why we can't add it, so he can't *do* anything with the
message.  Almost every printk of a constant string has this problem.
Using dev_err() instead of printk() would help a lot, as would
including the error code (result).  Of course, this would be a
separate patch that would update all the messages in the driver at the
same time.

>                        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_ost(handle, event, ost_code, NULL);
>        return;
>  }
>
> --
> 1.7.7.6
>
> --
> 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 1/4] ACPI: Set hotplug _OST support bit to _OSC

[Toshi Kani]

On Thu, 2012-04-26 at 09:16 -0600, Bjorn Helgaas wrote:
> On Tue, Apr 10, 2012 at 4:21 PM, Toshi Kani <toshi.kani@hp.com> wrote:
> > Added macro definitions of _OST source event 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.
> >
> > This patch sets OSC_SB_HOTPLUG_OST_SUPPORT to the platform-wide
> > OSPM capabilities when CONFIG option of ACPI CPU hotplug or memory
> > hotplug is enabled.
> >
> > Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> > ---
> >  drivers/acpi/bus.c   |    5 +++++
> >  include/linux/acpi.h |   26 +++++++++++++++++++++++++-
> >  2 files changed, 30 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > index 3263b68..a492d64 100644
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -544,6 +544,11 @@ static void acpi_bus_osc_support(void)
> >        capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_PPC_OST_SUPPORT;
> >  #endif
> >
> > +#if defined(CONFIG_ACPI_HOTPLUG_CPU) || defined(CONFIG_ACPI_HOTPLUG_MEMORY) ||\
> > +                defined(CONFIG_ACPI_HOTPLUG_MEMORY_MODULE)
> > +       capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_HOTPLUG_OST_SUPPORT;
> > +#endif
> 
> This seems a bit strange to me.  For one thing, the _OSC discussion
> doesn't seem to indicate that _OST support is specific to CPU or
> memory hotplug.  If we tell the platform that we support _OST, the
> platform can assume that we'll evaluate _OST for *any* device, which
> is not the case.  I guess this is just another reason why we need
> hotplug support in the ACPI core, not in the individual drivers.  Then
> we wouldn't have the ifdefs at all.

I agree, and it should be called for any device.  As you know, the issue
is that the global system notify handler acpi_bus_notify() and driver's
notify handler work independently, and their functionality conflicts
each other.  I have not thought out yet, but my current thinking is that
we may be able to integrate them into acpi_bus_notify() in the following
way.  This model allows a choice -- use the default handler or driver's
handler.  We can then call _OST for any device.  It can also eliminate
redundant ACPI namespace walks performed by drivers when registering
their notify handlers.  What do you think?

struct acpi_device_ops {
		:
   acpi_op_sys_notify sys_notify;  // driver's system notify (new entry)
}

acpi_bus_notify()
{
	driver = lookup-driver-with-HID();

	if ((driver) && (driver->ops->sys_notify)
		driver->ops.sys_notify();
	else
		Call the system default notify handler;

	Call _OST if present;
}


> The second thing is that the kernel should be correct at every point
> in a patch series.  In this case, if only patch [1/4] is applied, we
> claim to support _OST, but we actually don't.  So I think the patches
> that add support for evaluating _OST should be first, and this one
> should be last.  Or even better, a single patch could enable _OST
> evaluation and add this _OSC bit at the same time.

I see.  I will change the ordering, if that is OK.  I am a little afraid
that the patches may be hard to review when combined into a single
patch. 

Thanks!
-Toshi


> 
> Bjorn
> 
> > +
> >        if (!ghes_disable)
> >                capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_APEI_SUPPORT;
> >        if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index f421dd8..4127df8 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,30 @@ extern bool osc_sb_apei_support_acked;
> >
> >  extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
> >                                             u32 *mask, u32 req);
> > +
> > +/* _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
> >
> > --
> > 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 3/4] ACPI: Add _OST support for ACPI CPU hotplug

[Toshi Kani]

On Thu, 2012-04-26 at 09:22 -0600, Bjorn Helgaas wrote:
> On Tue, Apr 10, 2012 at 4:21 PM, Toshi Kani <toshi.kani@hp.com> wrote:
> > Changed acpi_processor_hotplug_notify() to call ACPI _OST method
> > when ACPI-based CPU hotplug operation 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..5d9b5a8 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");
> 
> You didn't add this problem, but since you're touching the code, I'll
> point it out.  This message will look to the user like:
> 
>     ACPI: Unable to add the device
> 
> which is useless.  The user has no idea what device we're talking
> about or why we can't add it, so he can't *do* anything with the
> message.  Almost every printk of a constant string has this problem.
> Using dev_err() instead of printk() would help a lot, as would
> including the error code (result).  Of course, this would be a
> separate patch that would update all the messages in the driver at the
> same time.

I agree.  I will put it as one of my future items. :)

Thanks,
-Toshi

> >                        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_ost(handle, event, ost_code, NULL);
> >        return;
> >  }
> >
> > --
> > 1.7.7.6
> >
> > --
> > 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 1/4] ACPI: Set hotplug _OST support bit to _OSC

[Toshi Kani]

On Thu, 2012-04-26 at 11:10 -0600, Toshi Kani wrote:
> On Thu, 2012-04-26 at 09:16 -0600, Bjorn Helgaas wrote:
> > On Tue, Apr 10, 2012 at 4:21 PM, Toshi Kani <toshi.kani@hp.com> wrote:
> > > Added macro definitions of _OST source event 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.
> > >
> > > This patch sets OSC_SB_HOTPLUG_OST_SUPPORT to the platform-wide
> > > OSPM capabilities when CONFIG option of ACPI CPU hotplug or memory
> > > hotplug is enabled.
> > >
> > > Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> > > ---
> > >  drivers/acpi/bus.c   |    5 +++++
> > >  include/linux/acpi.h |   26 +++++++++++++++++++++++++-
> > >  2 files changed, 30 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > > index 3263b68..a492d64 100644
> > > --- a/drivers/acpi/bus.c
> > > +++ b/drivers/acpi/bus.c
> > > @@ -544,6 +544,11 @@ static void acpi_bus_osc_support(void)
> > >        capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_PPC_OST_SUPPORT;
> > >  #endif
> > >
> > > +#if defined(CONFIG_ACPI_HOTPLUG_CPU) || defined(CONFIG_ACPI_HOTPLUG_MEMORY) ||\
> > > +                defined(CONFIG_ACPI_HOTPLUG_MEMORY_MODULE)
> > > +       capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_HOTPLUG_OST_SUPPORT;
> > > +#endif
> > 
> > This seems a bit strange to me.  For one thing, the _OSC discussion
> > doesn't seem to indicate that _OST support is specific to CPU or
> > memory hotplug.  If we tell the platform that we support _OST, the
> > platform can assume that we'll evaluate _OST for *any* device, which
> > is not the case.  I guess this is just another reason why we need
> > hotplug support in the ACPI core, not in the individual drivers.  Then
> > we wouldn't have the ifdefs at all.
> 
> I agree, and it should be called for any device.  As you know, the issue
> is that the global system notify handler acpi_bus_notify() and driver's
> notify handler work independently, and their functionality conflicts
> each other.  I have not thought out yet, but my current thinking is that
> we may be able to integrate them into acpi_bus_notify() in the following
> way.  This model allows a choice -- use the default handler or driver's
> handler.  We can then call _OST for any device.  It can also eliminate
> redundant ACPI namespace walks performed by drivers when registering
> their notify handlers.  What do you think?
> 
> struct acpi_device_ops {
> 		:
>    acpi_op_sys_notify sys_notify;  // driver's system notify (new entry)
> }
> 
> acpi_bus_notify()
> {
> 	driver = lookup-driver-with-HID();
> 
> 	if ((driver) && (driver->ops->sys_notify)
> 		driver->ops.sys_notify();
> 	else
> 		Call the system default notify handler;
> 
> 	Call _OST if present;
> }

I looked at the this model more carefully.  While the basic idea of
integration seems doable, I realized that calling _OST as described in
the above pseudo code has some issue.  For instance,
container_notify_cb() only generates a KOBJ_OFFLINE event for
ACPI_NOTIFY_EJECT_REQUEST.  Since an eject operation may or may not
happen asynchronously via udev, the notify handler may not call _OST
(other than in-progress update).  So for now, _OST support needs to be
made for each driver basis.  The above model allows drivers to choose
the default notify handler, so it would help to have more consistent
behavior over the time.  Without such changes, acpi_bus_notify() cannot
perform anything since it conflicts with driver's handler. 

-Toshi

Re: [PATCH 1/4] ACPI: Set hotplug _OST support bit to _OSC

[Toshi Kani]

On Fri, 2012-04-27 at 16:05 -0600, Toshi Kani wrote:
> On Thu, 2012-04-26 at 11:10 -0600, Toshi Kani wrote:
> > On Thu, 2012-04-26 at 09:16 -0600, Bjorn Helgaas wrote:
> > > On Tue, Apr 10, 2012 at 4:21 PM, Toshi Kani <toshi.kani@hp.com> wrote:
	:
> > > >
> > > > +#if defined(CONFIG_ACPI_HOTPLUG_CPU) || defined(CONFIG_ACPI_HOTPLUG_MEMORY) ||\
> > > > +                defined(CONFIG_ACPI_HOTPLUG_MEMORY_MODULE)
> > > > +       capbuf[OSC_SUPPORT_TYPE] |= OSC_SB_HOTPLUG_OST_SUPPORT;
> > > > +#endif
> > > 
> > > This seems a bit strange to me.  For one thing, the _OSC discussion
> > > doesn't seem to indicate that _OST support is specific to CPU or
> > > memory hotplug.  If we tell the platform that we support _OST, the
> > > platform can assume that we'll evaluate _OST for *any* device, which
> > > is not the case.  I guess this is just another reason why we need
> > > hotplug support in the ACPI core, not in the individual drivers.  Then
> > > we wouldn't have the ifdefs at all.

Thinking further, I am going to make the following changes to address
this comment.

1. Add CONFIG_ACPI_HOTPLUG_OST (disable by default)
When this option is set, the kernel calls _OSC with the hotplug _OST
flag at boot-time.  This replaces the current use of device-specific
config options, such as CONFIG_ACPI_HOTPLUG_CPU.  Also, the kernel only
calls _OST when this option is set.  In other words, all features in
this patch set is disabled when this option is not set (default).  This
should address Shuah's concerns about pre-enablement as well.

2. Add support of container hotplug
_OST will be supported for three major ACPI hotplug operations; CPU,
memory and container.  (This assumes PCI will use native hotplug going
forward.)  It will also support asynchronous hot-removal, such as
KOBJ_OFFLINE -> udev -> eject.

I will send updated _OST patches when they are ready.

Thanks,
-Toshi