[PATCH 0/2] PCI/ACPI: move _OSC code to pci_root.c (v.2)

[PATCH 0/2] PCI/ACPI: move _OSC code to pci_root.c (v.2)

[Kenji Kaneshige]

Hi,

Here are updated version (v.2) of "PCI/ACPI: move _OSC code to
pci_root.c" patches. Patches are

  - [PATCH 1/2] PCI/ACPI: move _OSC code to pci_root.c
  - [PATCH 2/2] PCI/ACPI: rename pci_osc_control_set()

ChangeLog
---------
[v.1 -> v.2]
  - Change the debug message.
    [old]: "_OSC FW not grant req. control."
    [new]: "Firmware did not grant requested _OSC control"
    (Based on the feedback from Andrew Patterson and Alex Chiang)

  - Rename pci_osc_control_set() to acpi_pci_osc_control_set() and
    move _OSC related definitions from include/linux/pci-acpi.h to
    include/linux/acpi.h.
    (Based on the feedback from Andrew Patterson)

Thanks,
Kenji Kaneshige

[PATCH 1/2] PCI/ACPI: move _OSC code to pci_root.c

[Kenji Kaneshige]

Move PCI _OSC management code from drivers/pci/pci-acpi.c to
drivers/acpi/pci_root.c. The benefits are

- We no longer need struct osc_data and its management code (contents
  are moved to struct acpi_pci_root). This simplify the code, and we
  no longer care about kmalloc() failure.

- We can make pci_acpi_osc_support() be a static function, which is
  called only from drivers/acpi/pci_root.c.

Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>

---
 drivers/acpi/pci_root.c  |  180 ++++++++++++++++++++++++++++++++++++++-
 drivers/pci/pci-acpi.c   |  215 -----------------------------------------------
 include/linux/pci-acpi.h |    1 
 3 files changed, 178 insertions(+), 218 deletions(-)

Index: 20090205/drivers/acpi/pci_root.c
===================================================================
--- 20090205.orig/drivers/acpi/pci_root.c
+++ 20090205/drivers/acpi/pci_root.c
@@ -66,11 +66,18 @@ struct acpi_pci_root {
 	struct acpi_device * device;
 	struct acpi_pci_id id;
 	struct pci_bus *bus;
+
+	u32 osc_support_set;	/* _OSC state of support bits */
+	u32 osc_control_set;	/* _OSC state of control bits */
+	u32 osc_control_qry;	/* the latest _OSC query result */
+
+	u32 osc_queried:1;	/* has _OSC control been queried? */
 };
 
 static LIST_HEAD(acpi_pci_roots);
 
 static struct acpi_pci_driver *sub_driver;
+static DEFINE_MUTEX(osc_lock);
 
 int acpi_pci_register_driver(struct acpi_pci_driver *driver)
 {
@@ -185,6 +192,175 @@ static void acpi_pci_bridge_scan(struct 
 		}
 }
 
+static u8 OSC_UUID[16] = {0x5B, 0x4D, 0xDB, 0x33, 0xF7, 0x1F, 0x1C, 0x40,
+			  0x96, 0x57, 0x74, 0x41, 0xC0, 0x3D, 0xD7, 0x66};
+
+static acpi_status acpi_pci_run_osc(acpi_handle handle,
+				    const u32 *capbuf, u32 *retval)
+{
+	acpi_status status;
+	struct acpi_object_list input;
+	union acpi_object in_params[4];
+	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
+	union acpi_object *out_obj;
+	u32 errors;
+
+	/* Setting up input parameters */
+	input.count = 4;
+	input.pointer = in_params;
+	in_params[0].type 		= ACPI_TYPE_BUFFER;
+	in_params[0].buffer.length 	= 16;
+	in_params[0].buffer.pointer	= OSC_UUID;
+	in_params[1].type 		= ACPI_TYPE_INTEGER;
+	in_params[1].integer.value 	= 1;
+	in_params[2].type 		= ACPI_TYPE_INTEGER;
+	in_params[2].integer.value	= 3;
+	in_params[3].type		= ACPI_TYPE_BUFFER;
+	in_params[3].buffer.length 	= 12;
+	in_params[3].buffer.pointer 	= (u8 *)capbuf;
+
+	status = acpi_evaluate_object(handle, "_OSC", &input, &output);
+	if (ACPI_FAILURE(status))
+		return status;
+
+	if (!output.length)
+		return AE_NULL_OBJECT;
+
+	out_obj = output.pointer;
+	if (out_obj->type != ACPI_TYPE_BUFFER) {
+		printk(KERN_DEBUG "Evaluate _OSC returns wrong type\n");
+		status = AE_TYPE;
+		goto out_kfree;
+	}
+	/* Need to ignore the bit0 in result code */
+	errors = *((u32 *)out_obj->buffer.pointer) & ~(1 << 0);
+	if (errors) {
+		if (errors & OSC_REQUEST_ERROR)
+			printk(KERN_DEBUG "_OSC request fails\n");
+		if (errors & OSC_INVALID_UUID_ERROR)
+			printk(KERN_DEBUG "_OSC invalid UUID\n");
+		if (errors & OSC_INVALID_REVISION_ERROR)
+			printk(KERN_DEBUG "_OSC invalid revision\n");
+		if (errors & OSC_CAPABILITIES_MASK_ERROR) {
+			if (capbuf[OSC_QUERY_TYPE] & OSC_QUERY_ENABLE)
+				goto out_success;
+			printk(KERN_DEBUG "Firmware did not grant requested "
+			       "_OSC control\n");
+			status = AE_SUPPORT;
+			goto out_kfree;
+		}
+		status = AE_ERROR;
+		goto out_kfree;
+	}
+out_success:
+	*retval = *((u32 *)(out_obj->buffer.pointer + 8));
+	status = AE_OK;
+
+out_kfree:
+	kfree(output.pointer);
+	return status;
+}
+
+static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root, u32 flags)
+{
+	acpi_status status;
+	u32 support_set, result, capbuf[3];
+
+	/* do _OSC query for all possible controls */
+	support_set = root->osc_support_set | (flags & OSC_SUPPORT_MASKS);
+	capbuf[OSC_QUERY_TYPE] = OSC_QUERY_ENABLE;
+	capbuf[OSC_SUPPORT_TYPE] = support_set;
+	capbuf[OSC_CONTROL_TYPE] = OSC_CONTROL_MASKS;
+
+	status = acpi_pci_run_osc(root->device->handle, capbuf, &result);
+	if (ACPI_SUCCESS(status)) {
+		root->osc_support_set = support_set;
+		root->osc_control_qry = result;
+		root->osc_queried = 1;
+	}
+	return status;
+}
+
+static acpi_status acpi_pci_osc_support(struct acpi_pci_root *root, u32 flags)
+{
+	acpi_status status;
+	acpi_handle tmp;
+
+	status = acpi_get_handle(root->device->handle, "_OSC", &tmp);
+	if (ACPI_FAILURE(status))
+		return status;
+	mutex_lock(&osc_lock);
+	status = acpi_pci_query_osc(root, flags);
+	mutex_unlock(&osc_lock);
+	return status;
+}
+
+static struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle)
+{
+	struct acpi_pci_root *root;
+	list_for_each_entry(root, &acpi_pci_roots, node) {
+		if (root->device->handle == handle)
+			return root;
+	}
+	return NULL;
+}
+
+/**
+ * pci_osc_control_set - commit requested control to Firmware
+ * @handle: acpi_handle for the target ACPI object
+ * @flags: driver's requested control bits
+ *
+ * Attempt to take control from Firmware on requested control bits.
+ **/
+acpi_status pci_osc_control_set(acpi_handle handle, u32 flags)
+{
+	acpi_status status;
+	u32 control_req, result, capbuf[3];
+	acpi_handle tmp;
+	struct acpi_pci_root *root;
+
+	status = acpi_get_handle(handle, "_OSC", &tmp);
+	if (ACPI_FAILURE(status))
+		return status;
+
+	control_req = (flags & OSC_CONTROL_MASKS);
+	if (!control_req)
+		return AE_TYPE;
+
+	root = acpi_pci_find_root(handle);
+	if (!root)
+		return AE_NOT_EXIST;
+
+	mutex_lock(&osc_lock);
+	/* No need to evaluate _OSC if the control was already granted. */
+	if ((root->osc_control_set & control_req) == control_req)
+		goto out;
+
+	/* Need to query controls first before requesting them */
+	if (!root->osc_queried) {
+		status = acpi_pci_query_osc(root, root->osc_support_set);
+		if (ACPI_FAILURE(status))
+			goto out;
+	}
+	if ((root->osc_control_qry & control_req) != control_req) {
+		printk(KERN_DEBUG
+		       "Firmware did not grant requested _OSC control\n");
+		status = AE_SUPPORT;
+		goto out;
+	}
+
+	capbuf[OSC_QUERY_TYPE] = 0;
+	capbuf[OSC_SUPPORT_TYPE] = root->osc_support_set;
+	capbuf[OSC_CONTROL_TYPE] = root->osc_control_set | control_req;
+	status = acpi_pci_run_osc(handle, capbuf, &result);
+	if (ACPI_SUCCESS(status))
+		root->osc_control_set = result;
+out:
+	mutex_unlock(&osc_lock);
+	return status;
+}
+EXPORT_SYMBOL(pci_osc_control_set);
+
 static int __devinit acpi_pci_root_add(struct acpi_device *device)
 {
 	int result = 0;
@@ -217,7 +393,7 @@ static int __devinit acpi_pci_root_add(s
 	 * PCI domains, so we indicate this in _OSC support capabilities.
 	 */
 	flags = base_flags = OSC_PCI_SEGMENT_GROUPS_SUPPORT;
-	pci_acpi_osc_support(device->handle, flags);
+	acpi_pci_osc_support(root, flags);
 
 	/* 
 	 * Segment
@@ -353,7 +529,7 @@ static int __devinit acpi_pci_root_add(s
 	if (pci_msi_enabled())
 		flags |= OSC_MSI_SUPPORT;
 	if (flags != base_flags)
-		pci_acpi_osc_support(device->handle, flags);
+		acpi_pci_osc_support(root, flags);
 
       end:
 	if (result) {
Index: 20090205/drivers/pci/pci-acpi.c
===================================================================
--- 20090205.orig/drivers/pci/pci-acpi.c
+++ 20090205/drivers/pci/pci-acpi.c
@@ -18,221 +18,6 @@
 #include <linux/pci-acpi.h>
 #include "pci.h"
 
-struct acpi_osc_data {
-	acpi_handle handle;
-	u32 support_set;
-	u32 control_set;
-	u32 control_query;
-	int is_queried;
-	struct list_head sibiling;
-};
-static LIST_HEAD(acpi_osc_data_list);
-
-struct acpi_osc_args {
-	u32 capbuf[3];
-};
-
-static DEFINE_MUTEX(pci_acpi_lock);
-
-static struct acpi_osc_data *acpi_get_osc_data(acpi_handle handle)
-{
-	struct acpi_osc_data *data;
-
-	list_for_each_entry(data, &acpi_osc_data_list, sibiling) {
-		if (data->handle == handle)
-			return data;
-	}
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return NULL;
-	INIT_LIST_HEAD(&data->sibiling);
-	data->handle = handle;
-	list_add_tail(&data->sibiling, &acpi_osc_data_list);
-	return data;
-}
-
-static u8 OSC_UUID[16] = {0x5B, 0x4D, 0xDB, 0x33, 0xF7, 0x1F, 0x1C, 0x40,
-			  0x96, 0x57, 0x74, 0x41, 0xC0, 0x3D, 0xD7, 0x66};
-
-static acpi_status acpi_run_osc(acpi_handle handle,
-				struct acpi_osc_args *osc_args, u32 *retval)
-{
-	acpi_status status;
-	struct acpi_object_list input;
-	union acpi_object in_params[4];
-	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
-	union acpi_object *out_obj;
-	u32 errors, flags = osc_args->capbuf[OSC_QUERY_TYPE];
-
-	/* Setting up input parameters */
-	input.count = 4;
-	input.pointer = in_params;
-	in_params[0].type 		= ACPI_TYPE_BUFFER;
-	in_params[0].buffer.length 	= 16;
-	in_params[0].buffer.pointer	= OSC_UUID;
-	in_params[1].type 		= ACPI_TYPE_INTEGER;
-	in_params[1].integer.value 	= 1;
-	in_params[2].type 		= ACPI_TYPE_INTEGER;
-	in_params[2].integer.value	= 3;
-	in_params[3].type		= ACPI_TYPE_BUFFER;
-	in_params[3].buffer.length 	= 12;
-	in_params[3].buffer.pointer 	= (u8 *)osc_args->capbuf;
-
-	status = acpi_evaluate_object(handle, "_OSC", &input, &output);
-	if (ACPI_FAILURE(status))
-		return status;
-
-	if (!output.length)
-		return AE_NULL_OBJECT;
-
-	out_obj = output.pointer;
-	if (out_obj->type != ACPI_TYPE_BUFFER) {
-		printk(KERN_DEBUG "Evaluate _OSC returns wrong type\n");
-		status = AE_TYPE;
-		goto out_kfree;
-	}
-	/* Need to ignore the bit0 in result code */
-	errors = *((u32 *)out_obj->buffer.pointer) & ~(1 << 0);
-	if (errors) {
-		if (errors & OSC_REQUEST_ERROR)
-			printk(KERN_DEBUG "_OSC request fails\n"); 
-		if (errors & OSC_INVALID_UUID_ERROR)
-			printk(KERN_DEBUG "_OSC invalid UUID\n"); 
-		if (errors & OSC_INVALID_REVISION_ERROR)
-			printk(KERN_DEBUG "_OSC invalid revision\n"); 
-		if (errors & OSC_CAPABILITIES_MASK_ERROR) {
-			if (flags & OSC_QUERY_ENABLE)
-				goto out_success;
-			printk(KERN_DEBUG "_OSC FW not grant req. control\n");
-			status = AE_SUPPORT;
-			goto out_kfree;
-		}
-		status = AE_ERROR;
-		goto out_kfree;
-	}
-out_success:
-	*retval = *((u32 *)(out_obj->buffer.pointer + 8));
-	status = AE_OK;
-
-out_kfree:
-	kfree(output.pointer);
-	return status;
-}
-
-static acpi_status __acpi_query_osc(u32 flags, struct acpi_osc_data *osc_data)
-{
-	acpi_status status;
-	u32 support_set, result;
-	struct acpi_osc_args osc_args;
-
-	/* do _OSC query for all possible controls */
-	support_set = osc_data->support_set | (flags & OSC_SUPPORT_MASKS);
-	osc_args.capbuf[OSC_QUERY_TYPE] = OSC_QUERY_ENABLE;
-	osc_args.capbuf[OSC_SUPPORT_TYPE] = support_set;
-	osc_args.capbuf[OSC_CONTROL_TYPE] = OSC_CONTROL_MASKS;
-
-	status = acpi_run_osc(osc_data->handle, &osc_args, &result);
-	if (ACPI_SUCCESS(status)) {
-		osc_data->support_set = support_set;
-		osc_data->control_query = result;
-		osc_data->is_queried = 1;
-	}
-
-	return status;
-}
-
-/*
- * pci_acpi_osc_support: Invoke _OSC indicating support for the given feature
- * @flags: Bitmask of flags to support
- *
- * See the ACPI spec for the definition of the flags
- */
-int pci_acpi_osc_support(acpi_handle handle, u32 flags)
-{
-	acpi_status status;
-	acpi_handle tmp;
-	struct acpi_osc_data *osc_data;
-	int rc = 0;
-
-	status = acpi_get_handle(handle, "_OSC", &tmp);
-	if (ACPI_FAILURE(status))
-		return -ENOTTY;
-
-	mutex_lock(&pci_acpi_lock);
-	osc_data = acpi_get_osc_data(handle);
-	if (!osc_data) {
-		printk(KERN_ERR "acpi osc data array is full\n");
-		rc = -ENOMEM;
-		goto out;
-	}
-
-	__acpi_query_osc(flags, osc_data);
-out:
-	mutex_unlock(&pci_acpi_lock);
-	return rc;
-}
-
-/**
- * pci_osc_control_set - commit requested control to Firmware
- * @handle: acpi_handle for the target ACPI object
- * @flags: driver's requested control bits
- *
- * Attempt to take control from Firmware on requested control bits.
- **/
-acpi_status pci_osc_control_set(acpi_handle handle, u32 flags)
-{
-	acpi_status status;
-	u32 control_req, control_set, result;
-	acpi_handle tmp;
-	struct acpi_osc_data *osc_data;
-	struct acpi_osc_args osc_args;
-
-	status = acpi_get_handle(handle, "_OSC", &tmp);
-	if (ACPI_FAILURE(status))
-		return status;
-
-	mutex_lock(&pci_acpi_lock);
-	osc_data = acpi_get_osc_data(handle);
-	if (!osc_data) {
-		printk(KERN_ERR "acpi osc data array is full\n");
-		status = AE_ERROR;
-		goto out;
-	}
-
-	control_req = (flags & OSC_CONTROL_MASKS);
-	if (!control_req) {
-		status = AE_TYPE;
-		goto out;
-	}
-
-	/* No need to evaluate _OSC if the control was already granted. */
-	if ((osc_data->control_set & control_req) == control_req)
-		goto out;
-
-	if (!osc_data->is_queried) {
-		status = __acpi_query_osc(osc_data->support_set, osc_data);
-		if (ACPI_FAILURE(status))
-			goto out;
-	}
-
-	if ((osc_data->control_query & control_req) != control_req) {
-		status = AE_SUPPORT;
-		goto out;
-	}
-
-	control_set = osc_data->control_set | control_req;
-	osc_args.capbuf[OSC_QUERY_TYPE] = 0;
-	osc_args.capbuf[OSC_SUPPORT_TYPE] = osc_data->support_set;
-	osc_args.capbuf[OSC_CONTROL_TYPE] = control_set;
-	status = acpi_run_osc(handle, &osc_args, &result);
-	if (ACPI_SUCCESS(status))
-		osc_data->control_set = result;
-out:
-	mutex_unlock(&pci_acpi_lock);
-	return status;
-}
-EXPORT_SYMBOL(pci_osc_control_set);
-
 /*
  * _SxD returns the D-state with the highest power
  * (lowest D-state number) supported in the S-state "x".
Index: 20090205/include/linux/pci-acpi.h
===================================================================
--- 20090205.orig/include/linux/pci-acpi.h
+++ 20090205/include/linux/pci-acpi.h
@@ -50,7 +50,6 @@
 
 #ifdef CONFIG_ACPI
 extern acpi_status pci_osc_control_set(acpi_handle handle, u32 flags);
-int pci_acpi_osc_support(acpi_handle handle, u32 flags);
 static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
 {
 	/* Find root host bridge */

Re: [PATCH 1/2] PCI/ACPI: move _OSC code to pci_root.c

[Alex Chiang]

Hi Kenji-san,

Sorry, I didn't read your patch in detail last time, and only saw
the one wording change that Andrew suggested.

Reading through now, I have a few more suggestions.

> +	status = acpi_evaluate_object(handle, "_OSC", &input, &output);
> +	if (ACPI_FAILURE(status))
> +		return status;
> +
> +	if (!output.length)
> +		return AE_NULL_OBJECT;
> +
> +	out_obj = output.pointer;
> +	if (out_obj->type != ACPI_TYPE_BUFFER) {
> +		printk(KERN_DEBUG "Evaluate _OSC returns wrong type\n");

"_OSC evaluation returned wrong type\n"

> +	if (errors) {
> +		if (errors & OSC_REQUEST_ERROR)
> +			printk(KERN_DEBUG "_OSC request fails\n");

"_OSC request failed\n"

> +		if (errors & OSC_INVALID_UUID_ERROR)
> +			printk(KERN_DEBUG "_OSC invalid UUID\n");
> +		if (errors & OSC_INVALID_REVISION_ERROR)
> +			printk(KERN_DEBUG "_OSC invalid revision\n");
> +		if (errors & OSC_CAPABILITIES_MASK_ERROR) {
> +			if (capbuf[OSC_QUERY_TYPE] & OSC_QUERY_ENABLE)
> +				goto out_success;
> +			printk(KERN_DEBUG"Firmware did not grant requested "
> +			       "_OSC control\n");

I prefer to see this message on one line (and break the 80 column
rule) to make grepping easier. But this is a judgement call, and
I'll let you decide.

Thanks.

/ac

RE: [PATCH 1/2] PCI/ACPI: move _OSC code to pci_root.c

[Moore, Robert]

The acpi_evaluate_object_typed interface automatically handles the two error cases you check for:

1) Checks for the case where no object is returned
2) Checks for the correct return object type (buffer in this case)

Use of this interface would simplify and shrink the code.

Bob


>-----Original Message-----
>From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
>owner@vger.kernel.org] On Behalf Of Alex Chiang
>Sent: Thursday, February 05, 2009 7:50 AM
>To: Kenji Kaneshige
>Cc: Jesse Barnes; linux-pci@vger.kernel.org; linux acpi; Andrew Patterson
>Subject: Re: [PATCH 1/2] PCI/ACPI: move _OSC code to pci_root.c
>
>Hi Kenji-san,
>
>Sorry, I didn't read your patch in detail last time, and only saw
>the one wording change that Andrew suggested.
>
>Reading through now, I have a few more suggestions.
>
>> +    status = acpi_evaluate_object(handle, "_OSC", &input, &output);
>> +    if (ACPI_FAILURE(status))
>> +            return status;
>> +
>> +    if (!output.length)
>> +            return AE_NULL_OBJECT;
>> +
>> +    out_obj = output.pointer;
>> +    if (out_obj->type != ACPI_TYPE_BUFFER) {
>> +            printk(KERN_DEBUG "Evaluate _OSC returns wrong type\n");
>
>"_OSC evaluation returned wrong type\n"
>
>> +    if (errors) {
>> +            if (errors & OSC_REQUEST_ERROR)
>> +                    printk(KERN_DEBUG "_OSC request fails\n");
>
>"_OSC request failed\n"
>
>> +            if (errors & OSC_INVALID_UUID_ERROR)
>> +                    printk(KERN_DEBUG "_OSC invalid UUID\n");
>> +            if (errors & OSC_INVALID_REVISION_ERROR)
>> +                    printk(KERN_DEBUG "_OSC invalid revision\n");
>> +            if (errors & OSC_CAPABILITIES_MASK_ERROR) {
>> +                    if (capbuf[OSC_QUERY_TYPE] & OSC_QUERY_ENABLE)
>> +                            goto out_success;
>> +                    printk(KERN_DEBUG"Firmware did not grant requested "
>> +                           "_OSC control\n");
>
>I prefer to see this message on one line (and break the 80 column
>rule) to make grepping easier. But this is a judgement call, and
>I'll let you decide.
>
>Thanks.
>
>/ac
>
>--
>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/2] PCI/ACPI: move _OSC code to pci_root.c

[Kenji Kaneshige]

Moore, Robert wrote:
> The acpi_evaluate_object_typed interface automatically handles the two error cases you check for:
> 
> 1) Checks for the case where no object is returned
> 2) Checks for the correct return object type (buffer in this case)
> 
> Use of this interface would simplify and shrink the code.
> 

Sounds good to me, but I have a question.

The acpi_evaluate_object_typed() function seems to be compiled
only when ACPI_FUTURE_USAGE is defined. And it seems that there
is no prototype declaration. I don't know about ACPI_FUTURE_USAGE
at all. Could you please tell me what I should do to use
acpi_evaluate_object_typed()?

Thanks,
Kenji Kaneshige

RE: [PATCH 1/2] PCI/ACPI: move _OSC code to pci_root.c

[Moore, Robert]

I guess this means that Linux does not currently use this interface.

I think I remember that Linux implements a utility module with an interface something like "acpi_evaluate_integer", and a couple more. Perhaps acpi_evaluate_buffer already exists, you could use that.


>-----Original Message-----
>From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
>owner@vger.kernel.org] On Behalf Of Kenji Kaneshige
>Sent: Thursday, February 05, 2009 9:43 PM
>To: Moore, Robert
>Cc: Alex Chiang; Jesse Barnes; linux-pci@vger.kernel.org; linux acpi;
>Andrew Patterson
>Subject: Re: [PATCH 1/2] PCI/ACPI: move _OSC code to pci_root.c
>
>Moore, Robert wrote:
>> The acpi_evaluate_object_typed interface automatically handles the two
>error cases you check for:
>>
>> 1) Checks for the case where no object is returned
>> 2) Checks for the correct return object type (buffer in this case)
>>
>> Use of this interface would simplify and shrink the code.
>>
>
>Sounds good to me, but I have a question.
>
>The acpi_evaluate_object_typed() function seems to be compiled
>only when ACPI_FUTURE_USAGE is defined. And it seems that there
>is no prototype declaration. I don't know about ACPI_FUTURE_USAGE
>at all. Could you please tell me what I should do to use
>acpi_evaluate_object_typed()?
>
>Thanks,
>Kenji Kaneshige
>
>
>--
>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/2] PCI/ACPI: move _OSC code to pci_root.c

[Kenji Kaneshige]

Moore, Robert wrote:
> I guess this means that Linux does not currently use this interface.
> 
> I think I remember that Linux implements a utility module with an interface something like "acpi_evaluate_integer", and a couple more. Perhaps acpi_evaluate_buffer already exists, you could use that.
> 

Thank you for suggestion.

Unfortunately, I could not find acpi_evaluate_buffer() and something
like this. So I'd like to leave the code as it is, and wait for
acpi_evaluate_object_typed().

Thanks,
Kenji Kaneshige


> 
>> -----Original Message-----
>> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
>> owner@vger.kernel.org] On Behalf Of Kenji Kaneshige
>> Sent: Thursday, February 05, 2009 9:43 PM
>> To: Moore, Robert
>> Cc: Alex Chiang; Jesse Barnes; linux-pci@vger.kernel.org; linux acpi;
>> Andrew Patterson
>> Subject: Re: [PATCH 1/2] PCI/ACPI: move _OSC code to pci_root.c
>>
>> Moore, Robert wrote:
>>> The acpi_evaluate_object_typed interface automatically handles the two
>> error cases you check for:
>>> 1) Checks for the case where no object is returned
>>> 2) Checks for the correct return object type (buffer in this case)
>>>
>>> Use of this interface would simplify and shrink the code.
>>>
>> Sounds good to me, but I have a question.
>>
>> The acpi_evaluate_object_typed() function seems to be compiled
>> only when ACPI_FUTURE_USAGE is defined. And it seems that there
>> is no prototype declaration. I don't know about ACPI_FUTURE_USAGE
>> at all. Could you please tell me what I should do to use
>> acpi_evaluate_object_typed()?
>>
>> Thanks,
>> Kenji Kaneshige
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 1/2] PCI/ACPI: move _OSC code to pci_root.c

[Kenji Kaneshige]

Alex Chiang wrote:
> Hi Kenji-san,
> 
> Sorry, I didn't read your patch in detail last time, and only saw
> the one wording change that Andrew suggested.
> 
> Reading through now, I have a few more suggestions.
>

Thank you for suggestions.

>> +	status = acpi_evaluate_object(handle, "_OSC", &input, &output);
>> +	if (ACPI_FAILURE(status))
>> +		return status;
>> +
>> +	if (!output.length)
>> +		return AE_NULL_OBJECT;
>> +
>> +	out_obj = output.pointer;
>> +	if (out_obj->type != ACPI_TYPE_BUFFER) {
>> +		printk(KERN_DEBUG "Evaluate _OSC returns wrong type\n");
> 
> "_OSC evaluation returned wrong type\n"
> 

I'll fix it. But this debug message would be removed if we use
acpi_evaluate_object_typed() instead of acpi_evaluate_object() as
Robert suggested.

>> +	if (errors) {
>> +		if (errors & OSC_REQUEST_ERROR)
>> +			printk(KERN_DEBUG "_OSC request fails\n");
> 
> "_OSC request failed\n"
> 

I'll fix it.

>> +		if (errors & OSC_INVALID_UUID_ERROR)
>> +			printk(KERN_DEBUG "_OSC invalid UUID\n");
>> +		if (errors & OSC_INVALID_REVISION_ERROR)
>> +			printk(KERN_DEBUG "_OSC invalid revision\n");
>> +		if (errors & OSC_CAPABILITIES_MASK_ERROR) {
>> +			if (capbuf[OSC_QUERY_TYPE] & OSC_QUERY_ENABLE)
>> +				goto out_success;
>> +			printk(KERN_DEBUG"Firmware did not grant requested "
>> +			       "_OSC control\n");
> 
> I prefer to see this message on one line (and break the 80 column
> rule) to make grepping easier. But this is a judgement call, and
> I'll let you decide.
> 

Ok, I think one line is better. So I'll bread the 80 column rule:)

Thanks,
Kenji Kaneshige

[PATCH 2/2] PCI/ACPI: rename pci_osc_control_set()

[Kenji Kaneshige]

- Rename pci_osc_control_set() to acpi_pci_osc_control_set() according
  to the other API names in drivers/acpi/pci_root.c.

- Move _OSC related definitions to include/linux/acpi.h because _OSC
  related API is implemented in drivers/acpi/pci_root.c now.

Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>

---
 drivers/acpi/pci_root.c            |    6 ++--
 drivers/pci/hotplug/acpi_pcihp.c   |    5 +---
 drivers/pci/pcie/aer/aerdrv_acpi.c |    2 -
 include/linux/acpi.h               |   34 +++++++++++++++++++++++++++
 include/linux/pci-acpi.h           |   45 -------------------------------------
 5 files changed, 40 insertions(+), 52 deletions(-)

Index: 20090205/drivers/acpi/pci_root.c
===================================================================
--- 20090205.orig/drivers/acpi/pci_root.c
+++ 20090205/drivers/acpi/pci_root.c
@@ -306,13 +306,13 @@ static struct acpi_pci_root *acpi_pci_fi
 }
 
 /**
- * pci_osc_control_set - commit requested control to Firmware
+ * acpi_pci_osc_control_set - commit requested control to Firmware
  * @handle: acpi_handle for the target ACPI object
  * @flags: driver's requested control bits
  *
  * Attempt to take control from Firmware on requested control bits.
  **/
-acpi_status pci_osc_control_set(acpi_handle handle, u32 flags)
+acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 flags)
 {
 	acpi_status status;
 	u32 control_req, result, capbuf[3];
@@ -359,7 +359,7 @@ out:
 	mutex_unlock(&osc_lock);
 	return status;
 }
-EXPORT_SYMBOL(pci_osc_control_set);
+EXPORT_SYMBOL(acpi_pci_osc_control_set);
 
 static int __devinit acpi_pci_root_add(struct acpi_device *device)
 {
Index: 20090205/include/linux/pci-acpi.h
===================================================================
--- 20090205.orig/include/linux/pci-acpi.h
+++ 20090205/include/linux/pci-acpi.h
@@ -10,46 +10,7 @@
 
 #include <linux/acpi.h>
 
-#define OSC_QUERY_TYPE			0
-#define OSC_SUPPORT_TYPE 		1
-#define OSC_CONTROL_TYPE		2
-#define OSC_SUPPORT_MASKS		0x1f
-
-/*
- * _OSC DW0 Definition 
- */
-#define OSC_QUERY_ENABLE		1
-#define OSC_REQUEST_ERROR		2
-#define OSC_INVALID_UUID_ERROR		4
-#define OSC_INVALID_REVISION_ERROR	8
-#define OSC_CAPABILITIES_MASK_ERROR	16
-
-/*
- * _OSC DW1 Definition (OS Support Fields)
- */
-#define OSC_EXT_PCI_CONFIG_SUPPORT		1
-#define OSC_ACTIVE_STATE_PWR_SUPPORT 		2
-#define OSC_CLOCK_PWR_CAPABILITY_SUPPORT	4
-#define OSC_PCI_SEGMENT_GROUPS_SUPPORT		8
-#define OSC_MSI_SUPPORT				16
-
-/*
- * _OSC DW1 Definition (OS Control Fields)
- */
-#define OSC_PCI_EXPRESS_NATIVE_HP_CONTROL	1
-#define OSC_SHPC_NATIVE_HP_CONTROL 		2
-#define OSC_PCI_EXPRESS_PME_CONTROL		4
-#define OSC_PCI_EXPRESS_AER_CONTROL		8
-#define OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL	16
-
-#define OSC_CONTROL_MASKS 	(OSC_PCI_EXPRESS_NATIVE_HP_CONTROL | 	\
-				OSC_SHPC_NATIVE_HP_CONTROL | 		\
-				OSC_PCI_EXPRESS_PME_CONTROL |		\
-				OSC_PCI_EXPRESS_AER_CONTROL |		\
-				OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL)
-
 #ifdef CONFIG_ACPI
-extern acpi_status pci_osc_control_set(acpi_handle handle, u32 flags);
 static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
 {
 	/* Find root host bridge */
@@ -69,12 +30,6 @@ static inline acpi_handle acpi_pci_get_b
 	return acpi_get_pci_rootbridge_handle(seg, busnr);
 }
 #else
-#if !defined(AE_ERROR)
-typedef u32 		acpi_status;
-#define AE_ERROR      	(acpi_status) (0x0001)
-#endif    
-static inline acpi_status pci_osc_control_set(acpi_handle handle, u32 flags)
-{return AE_ERROR;}
 static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
 { return NULL; }
 #endif
Index: 20090205/include/linux/acpi.h
===================================================================
--- 20090205.orig/include/linux/acpi.h
+++ 20090205/include/linux/acpi.h
@@ -256,6 +256,40 @@ void __init acpi_no_s4_hw_signature(void
 void __init acpi_old_suspend_ordering(void);
 void __init acpi_s4_no_nvs(void);
 #endif /* CONFIG_PM_SLEEP */
+
+#define OSC_QUERY_TYPE			0
+#define OSC_SUPPORT_TYPE 		1
+#define OSC_CONTROL_TYPE		2
+#define OSC_SUPPORT_MASKS		0x1f
+
+/* _OSC DW0 Definition */
+#define OSC_QUERY_ENABLE		1
+#define OSC_REQUEST_ERROR		2
+#define OSC_INVALID_UUID_ERROR		4
+#define OSC_INVALID_REVISION_ERROR	8
+#define OSC_CAPABILITIES_MASK_ERROR	16
+
+/* _OSC DW1 Definition (OS Support Fields) */
+#define OSC_EXT_PCI_CONFIG_SUPPORT		1
+#define OSC_ACTIVE_STATE_PWR_SUPPORT 		2
+#define OSC_CLOCK_PWR_CAPABILITY_SUPPORT	4
+#define OSC_PCI_SEGMENT_GROUPS_SUPPORT		8
+#define OSC_MSI_SUPPORT				16
+
+/* _OSC DW1 Definition (OS Control Fields) */
+#define OSC_PCI_EXPRESS_NATIVE_HP_CONTROL	1
+#define OSC_SHPC_NATIVE_HP_CONTROL 		2
+#define OSC_PCI_EXPRESS_PME_CONTROL		4
+#define OSC_PCI_EXPRESS_AER_CONTROL		8
+#define OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL	16
+
+#define OSC_CONTROL_MASKS 	(OSC_PCI_EXPRESS_NATIVE_HP_CONTROL | 	\
+				OSC_SHPC_NATIVE_HP_CONTROL | 		\
+				OSC_PCI_EXPRESS_PME_CONTROL |		\
+				OSC_PCI_EXPRESS_AER_CONTROL |		\
+				OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL)
+
+extern acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 flags);
 #else	/* CONFIG_ACPI */
 
 static inline int early_acpi_boot_init(void)
Index: 20090205/drivers/pci/hotplug/acpi_pcihp.c
===================================================================
--- 20090205.orig/drivers/pci/hotplug/acpi_pcihp.c
+++ 20090205/drivers/pci/hotplug/acpi_pcihp.c
@@ -30,9 +30,8 @@
 #include <linux/types.h>
 #include <linux/pci.h>
 #include <linux/pci_hotplug.h>
+#include <linux/acpi.h>
 #include <linux/pci-acpi.h>
-#include <acpi/acpi.h>
-#include <acpi/acpi_bus.h>
 
 #define MY_NAME	"acpi_pcihp"
 
@@ -408,7 +407,7 @@ int acpi_get_hp_hw_control_from_firmware
 		acpi_get_name(handle, ACPI_FULL_PATHNAME, &string);
 		dbg("Trying to get hotplug control for %s\n",
 				(char *)string.pointer);
-		status = pci_osc_control_set(handle, flags);
+		status = acpi_pci_osc_control_set(handle, flags);
 		if (ACPI_SUCCESS(status))
 			goto got_one;
 		kfree(string.pointer);
Index: 20090205/drivers/pci/pcie/aer/aerdrv_acpi.c
===================================================================
--- 20090205.orig/drivers/pci/pcie/aer/aerdrv_acpi.c
+++ 20090205/drivers/pci/pcie/aer/aerdrv_acpi.c
@@ -38,7 +38,7 @@ int aer_osc_setup(struct pcie_device *pc
 
 	handle = acpi_find_root_bridge_handle(pdev);
 	if (handle) {
-		status = pci_osc_control_set(handle,
+		status = acpi_pci_osc_control_set(handle,
 					OSC_PCI_EXPRESS_AER_CONTROL |
 					OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL);
 	}