diff mbox

[v3,8/8] PCI hotplug: clean up acpi_run_hpp()

Message ID 20090902225040.12417.48488.stgit@bob.kio (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Bjorn Helgaas Sept. 2, 2009, 10:50 p.m. UTC 
This patch cleans up acpi_run_hpp() and follows the style of acpi_run_hpx():
    - remove unnecessary METHOD_NAME__HPP #define
    - use ACPI_ALLOCATE_BUFFER rather than evaluating _HPP twice
    - validate _HPP package length (defined as 4 by the spec)
    - avoid ref to undefined data if FW provides < 4 elements
    - remove temporary nui[] array

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
Reviewed-by: Alex Chiang <achiang@hp.com>
---
 drivers/pci/hotplug/acpi_pcihp.c |   84 ++++++++++----------------------------
 1 files changed, 22 insertions(+), 62 deletions(-)


--
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

Comments

Kenji Kaneshige Sept. 3, 2009, 12:57 a.m. UTC | #1 
Reviewed-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Acked-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>


Bjorn Helgaas wrote:
> This patch cleans up acpi_run_hpp() and follows the style of acpi_run_hpx():
>     - remove unnecessary METHOD_NAME__HPP #define
>     - use ACPI_ALLOCATE_BUFFER rather than evaluating _HPP twice
>     - validate _HPP package length (defined as 4 by the spec)
>     - avoid ref to undefined data if FW provides < 4 elements
>     - remove temporary nui[] array
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> Reviewed-by: Alex Chiang <achiang@hp.com>
> ---
>  drivers/pci/hotplug/acpi_pcihp.c |   84 ++++++++++----------------------------
>  1 files changed, 22 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
> index c53f72b..2673407 100644
> --- a/drivers/pci/hotplug/acpi_pcihp.c
> +++ b/drivers/pci/hotplug/acpi_pcihp.c
> @@ -41,7 +41,6 @@
>  #define warn(format, arg...) printk(KERN_WARNING "%s: " format , MY_NAME , ## arg)
>  
>  #define	METHOD_NAME__SUN	"_SUN"
> -#define	METHOD_NAME__HPP	"_HPP"
>  #define	METHOD_NAME_OSHP	"OSHP"
>  
>  static int debug_acpi;
> @@ -215,80 +214,41 @@ acpi_run_hpx(acpi_handle handle, struct hotplug_params *hpx)
>  static acpi_status
>  acpi_run_hpp(acpi_handle handle, struct hotplug_params *hpp)
>  {
> -	acpi_status		status;
> -	u8			nui[4];
> -	struct acpi_buffer	ret_buf = { 0, NULL};
> -	struct acpi_buffer	string = { ACPI_ALLOCATE_BUFFER, NULL };
> -	union acpi_object	*ext_obj, *package;
> -	int			i, len = 0;
> -
> -	acpi_get_name(handle, ACPI_FULL_PATHNAME, &string);
> +	acpi_status status;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *package, *fields;
> +	int i;
>  
> -	/* Clear the return buffer with zeros */
>  	memset(hpp, 0, sizeof(struct hotplug_params));
>  
> -	/* get _hpp */
> -	status = acpi_evaluate_object(handle, METHOD_NAME__HPP, NULL, &ret_buf);
> -	switch (status) {
> -	case AE_BUFFER_OVERFLOW:
> -		ret_buf.pointer = kmalloc (ret_buf.length, GFP_KERNEL);
> -		if (!ret_buf.pointer) {
> -			printk(KERN_ERR "%s:%s alloc for _HPP fail\n",
> -				__func__, (char *)string.pointer);
> -			kfree(string.pointer);
> -			return AE_NO_MEMORY;
> -		}
> -		status = acpi_evaluate_object(handle, METHOD_NAME__HPP,
> -				NULL, &ret_buf);
> -		if (ACPI_SUCCESS(status))
> -			break;
> -	default:
> -		if (ACPI_FAILURE(status)) {
> -			pr_debug("%s:%s _HPP fail=0x%x\n", __func__,
> -				(char *)string.pointer, status);
> -			kfree(string.pointer);
> -			return status;
> -		}
> -	}
> +	status = acpi_evaluate_object(handle, "_HPP", NULL, &buffer);
> +	if (ACPI_FAILURE(status))
> +		return status;
>  
> -	ext_obj = (union acpi_object *) ret_buf.pointer;
> -	if (ext_obj->type != ACPI_TYPE_PACKAGE) {
> -		printk(KERN_ERR "%s:%s _HPP obj not a package\n", __func__,
> -				(char *)string.pointer);
> +	package = (union acpi_object *) buffer.pointer;
> +	if (package->type != ACPI_TYPE_PACKAGE ||
> +	    package->package.count != 4) {
>  		status = AE_ERROR;
> -		goto free_and_return;
> +		goto exit;
>  	}
>  
> -	len = ext_obj->package.count;
> -	package = (union acpi_object *) ret_buf.pointer;
> -	for ( i = 0; (i < len) || (i < 4); i++) {
> -		ext_obj = (union acpi_object *) &package->package.elements[i];
> -		switch (ext_obj->type) {
> -		case ACPI_TYPE_INTEGER:
> -			nui[i] = (u8)ext_obj->integer.value;
> -			break;
> -		default:
> -			printk(KERN_ERR "%s:%s _HPP obj type incorrect\n",
> -				__func__, (char *)string.pointer);
> +	fields = package->package.elements;
> +	for (i = 0; i < 4; i++) {
> +		if (fields[i].type != ACPI_TYPE_INTEGER) {
>  			status = AE_ERROR;
> -			goto free_and_return;
> +			goto exit;
>  		}
>  	}
>  
>  	hpp->t0 = &hpp->type0_data;
> -	hpp->t0->cache_line_size = nui[0];
> -	hpp->t0->latency_timer = nui[1];
> -	hpp->t0->enable_serr = nui[2];
> -	hpp->t0->enable_perr = nui[3];
> -
> -	pr_debug("  _HPP: cache_line_size=0x%x\n", hpp->t0->cache_line_size);
> -	pr_debug("  _HPP: latency timer  =0x%x\n", hpp->t0->latency_timer);
> -	pr_debug("  _HPP: enable SERR    =0x%x\n", hpp->t0->enable_serr);
> -	pr_debug("  _HPP: enable PERR    =0x%x\n", hpp->t0->enable_perr);
> +	hpp->t0->revision        = 1;
> +	hpp->t0->cache_line_size = fields[0].integer.value;
> +	hpp->t0->latency_timer   = fields[1].integer.value;
> +	hpp->t0->enable_serr     = fields[2].integer.value;
> +	hpp->t0->enable_perr     = fields[3].integer.value;
>  
> -free_and_return:
> -	kfree(string.pointer);
> -	kfree(ret_buf.pointer);
> +exit:
> +	kfree(buffer.pointer);
>  	return status;
>  }
>  
> 
> --
> 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
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/hotplug/acpi_pcihp.c b/drivers/pci/hotplug/acpi_pcihp.c
index c53f72b..2673407 100644
--- a/drivers/pci/hotplug/acpi_pcihp.c
+++ b/drivers/pci/hotplug/acpi_pcihp.c
@@ -41,7 +41,6 @@ 
 #define warn(format, arg...) printk(KERN_WARNING "%s: " format , MY_NAME , ## arg)
 
 #define	METHOD_NAME__SUN	"_SUN"
-#define	METHOD_NAME__HPP	"_HPP"
 #define	METHOD_NAME_OSHP	"OSHP"
 
 static int debug_acpi;
@@ -215,80 +214,41 @@  acpi_run_hpx(acpi_handle handle, struct hotplug_params *hpx)
 static acpi_status
 acpi_run_hpp(acpi_handle handle, struct hotplug_params *hpp)
 {
-	acpi_status		status;
-	u8			nui[4];
-	struct acpi_buffer	ret_buf = { 0, NULL};
-	struct acpi_buffer	string = { ACPI_ALLOCATE_BUFFER, NULL };
-	union acpi_object	*ext_obj, *package;
-	int			i, len = 0;
-
-	acpi_get_name(handle, ACPI_FULL_PATHNAME, &string);
+	acpi_status status;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *package, *fields;
+	int i;
 
-	/* Clear the return buffer with zeros */
 	memset(hpp, 0, sizeof(struct hotplug_params));
 
-	/* get _hpp */
-	status = acpi_evaluate_object(handle, METHOD_NAME__HPP, NULL, &ret_buf);
-	switch (status) {
-	case AE_BUFFER_OVERFLOW:
-		ret_buf.pointer = kmalloc (ret_buf.length, GFP_KERNEL);
-		if (!ret_buf.pointer) {
-			printk(KERN_ERR "%s:%s alloc for _HPP fail\n",
-				__func__, (char *)string.pointer);
-			kfree(string.pointer);
-			return AE_NO_MEMORY;
-		}
-		status = acpi_evaluate_object(handle, METHOD_NAME__HPP,
-				NULL, &ret_buf);
-		if (ACPI_SUCCESS(status))
-			break;
-	default:
-		if (ACPI_FAILURE(status)) {
-			pr_debug("%s:%s _HPP fail=0x%x\n", __func__,
-				(char *)string.pointer, status);
-			kfree(string.pointer);
-			return status;
-		}
-	}
+	status = acpi_evaluate_object(handle, "_HPP", NULL, &buffer);
+	if (ACPI_FAILURE(status))
+		return status;
 
-	ext_obj = (union acpi_object *) ret_buf.pointer;
-	if (ext_obj->type != ACPI_TYPE_PACKAGE) {
-		printk(KERN_ERR "%s:%s _HPP obj not a package\n", __func__,
-				(char *)string.pointer);
+	package = (union acpi_object *) buffer.pointer;
+	if (package->type != ACPI_TYPE_PACKAGE ||
+	    package->package.count != 4) {
 		status = AE_ERROR;
-		goto free_and_return;
+		goto exit;
 	}
 
-	len = ext_obj->package.count;
-	package = (union acpi_object *) ret_buf.pointer;
-	for ( i = 0; (i < len) || (i < 4); i++) {
-		ext_obj = (union acpi_object *) &package->package.elements[i];
-		switch (ext_obj->type) {
-		case ACPI_TYPE_INTEGER:
-			nui[i] = (u8)ext_obj->integer.value;
-			break;
-		default:
-			printk(KERN_ERR "%s:%s _HPP obj type incorrect\n",
-				__func__, (char *)string.pointer);
+	fields = package->package.elements;
+	for (i = 0; i < 4; i++) {
+		if (fields[i].type != ACPI_TYPE_INTEGER) {
 			status = AE_ERROR;
-			goto free_and_return;
+			goto exit;
 		}
 	}
 
 	hpp->t0 = &hpp->type0_data;
-	hpp->t0->cache_line_size = nui[0];
-	hpp->t0->latency_timer = nui[1];
-	hpp->t0->enable_serr = nui[2];
-	hpp->t0->enable_perr = nui[3];
-
-	pr_debug("  _HPP: cache_line_size=0x%x\n", hpp->t0->cache_line_size);
-	pr_debug("  _HPP: latency timer  =0x%x\n", hpp->t0->latency_timer);
-	pr_debug("  _HPP: enable SERR    =0x%x\n", hpp->t0->enable_serr);
-	pr_debug("  _HPP: enable PERR    =0x%x\n", hpp->t0->enable_perr);
+	hpp->t0->revision        = 1;
+	hpp->t0->cache_line_size = fields[0].integer.value;
+	hpp->t0->latency_timer   = fields[1].integer.value;
+	hpp->t0->enable_serr     = fields[2].integer.value;
+	hpp->t0->enable_perr     = fields[3].integer.value;
 
-free_and_return:
-	kfree(string.pointer);
-	kfree(ret_buf.pointer);
+exit:
+	kfree(buffer.pointer);
 	return status;
 }