Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1614080 > unrolled thread

[PATCH v2] xen: make functions in xen-acpi-processor return void

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] xen: make functions in xen-acpi-processor return void Juergen Gross <jgross@suse.com> - 2017-03-31 16:50 +0200
    Re: [PATCH v2] xen: make functions in xen-acpi-processor return void Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> - 2017-03-31 17:20 +0200
      Re: [PATCH v2] xen: make functions in xen-acpi-processor return void Juergen Gross <jgross@suse.com> - 2017-03-31 17:40 +0200
    Re: [PATCH v2] xen: make functions in xen-acpi-processor return void Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-03-31 17:20 +0200
      Re: [PATCH v2] xen: make functions in xen-acpi-processor return void Juergen Gross <jgross@suse.com> - 2017-03-31 17:40 +0200

#1614080 — [PATCH v2] xen: make functions in xen-acpi-processor return void

There are several functions in xen-acpi-processor which either always
return the same value or where the returned value is never checked.

Make the functions return void.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/xen-acpi-processor.c | 51 +++++++++++++++-------------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index 23e391d..45be017 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -54,7 +54,7 @@ static unsigned long *acpi_id_present;
 /* And if there is an _CST definition (or a PBLK) for the ACPI IDs */
 static unsigned long *acpi_id_cst_present;
 
-static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
+static void push_cxx_to_hypervisor(struct acpi_processor *_pr)
 {
 	struct xen_platform_op op = {
 		.cmd			= XENPF_set_processor_pminfo,
@@ -70,7 +70,7 @@ static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
 	dst_cx_states = kcalloc(_pr->power.count,
 				sizeof(struct xen_processor_cx), GFP_KERNEL);
 	if (!dst_cx_states)
-		return -ENOMEM;
+		return;
 
 	for (ok = 0, i = 1; i <= _pr->power.count; i++) {
 		cx = &_pr->power.states[i];
@@ -104,7 +104,7 @@ static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
 	if (!ok) {
 		pr_debug("No _Cx for ACPI CPU %u\n", _pr->acpi_id);
 		kfree(dst_cx_states);
-		return -EINVAL;
+		return;
 	}
 	op.u.set_pminfo.power.count = ok;
 	op.u.set_pminfo.power.flags.bm_control = _pr->flags.bm_control;
@@ -135,8 +135,6 @@ static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
 		       ret, _pr->acpi_id);
 
 	kfree(dst_cx_states);
-
-	return ret;
 }
 static struct xen_processor_px *
 xen_copy_pss_data(struct acpi_processor *_pr,
@@ -161,8 +159,8 @@ xen_copy_pss_data(struct acpi_processor *_pr,
 	}
 	return dst_states;
 }
-static int xen_copy_psd_data(struct acpi_processor *_pr,
-			     struct xen_processor_performance *dst)
+static void xen_copy_psd_data(struct acpi_processor *_pr,
+			      struct xen_processor_performance *dst)
 {
 	struct acpi_psd_package *pdomain;
 
@@ -189,10 +187,9 @@ static int xen_copy_psd_data(struct acpi_processor *_pr,
 
 	}
 	memcpy(&(dst->domain_info), pdomain, sizeof(struct acpi_psd_package));
-	return 0;
 }
-static int xen_copy_pct_data(struct acpi_pct_register *pct,
-			     struct xen_pct_register *dst_pct)
+static void xen_copy_pct_data(struct acpi_pct_register *pct,
+			      struct xen_pct_register *dst_pct)
 {
 	/* It would be nice if you could just do 'memcpy(pct, dst_pct') but
 	 * sadly the Xen structure did not have the proper padding so the
@@ -205,9 +202,8 @@ static int xen_copy_pct_data(struct acpi_pct_register *pct,
 	dst_pct->bit_offset = pct->bit_offset;
 	dst_pct->reserved = pct->reserved;
 	dst_pct->address = pct->address;
-	return 0;
 }
-static int push_pxx_to_hypervisor(struct acpi_processor *_pr)
+static void push_pxx_to_hypervisor(struct acpi_processor *_pr)
 {
 	int ret = 0;
 	struct xen_platform_op op = {
@@ -233,13 +229,12 @@ static int push_pxx_to_hypervisor(struct acpi_processor *_pr)
 		set_xen_guest_handle(dst_perf->states, dst_states);
 		dst_perf->flags |= XEN_PX_PSS;
 	}
-	if (!xen_copy_psd_data(_pr, dst_perf))
-		dst_perf->flags |= XEN_PX_PSD;
+	xen_copy_psd_data(_pr, dst_perf);
+	dst_perf->flags |= XEN_PX_PSD;
 
 	if (dst_perf->flags != (XEN_PX_PSD | XEN_PX_PSS | XEN_PX_PCT | XEN_PX_PPC)) {
 		pr_warn("ACPI CPU%u missing some P-state data (%x), skipping\n",
 			_pr->acpi_id, dst_perf->flags);
-		ret = -ENODEV;
 		goto err_free;
 	}
 
@@ -268,26 +263,18 @@ static int push_pxx_to_hypervisor(struct acpi_processor *_pr)
 err_free:
 	if (!IS_ERR_OR_NULL(dst_states))
 		kfree(dst_states);
-
-	return ret;
 }
-static int upload_pm_data(struct acpi_processor *_pr)
+static void upload_pm_data(struct acpi_processor *_pr)
 {
-	int err = 0;
-
 	mutex_lock(&acpi_ids_mutex);
-	if (__test_and_set_bit(_pr->acpi_id, acpi_ids_done)) {
-		mutex_unlock(&acpi_ids_mutex);
-		return -EBUSY;
+	if (!__test_and_set_bit(_pr->acpi_id, acpi_ids_done)) {
+		if (_pr->flags.power)
+			push_cxx_to_hypervisor(_pr);
+
+		if (_pr->performance && _pr->performance->states)
+			push_pxx_to_hypervisor(_pr);
 	}
-	if (_pr->flags.power)
-		err = push_cxx_to_hypervisor(_pr);
-
-	if (_pr->performance && _pr->performance->states)
-		err |= push_pxx_to_hypervisor(_pr);
-
 	mutex_unlock(&acpi_ids_mutex);
-	return err;
 }
 static unsigned int __init get_max_acpi_id(void)
 {
@@ -417,7 +404,7 @@ static int check_acpi_ids(struct acpi_processor *pr_backup)
 			pr_backup->acpi_id = i;
 			/* Mask out C-states if there are no _CST or PBLK */
 			pr_backup->flags.power = test_bit(i, acpi_id_cst_present);
-			(void)upload_pm_data(pr_backup);
+			upload_pm_data(pr_backup);
 		}
 	}
 
@@ -457,7 +444,7 @@ static int xen_upload_processor_pm_data(void)
 			if (pr_backup)
 				memcpy(pr_backup, _pr, sizeof(struct acpi_processor));
 		}
-		(void)upload_pm_data(_pr);
+		upload_pm_data(_pr);
 	}
 
 	rc = check_acpi_ids(pr_backup);
-- 
2.10.2

[toc] | [next] | [standalone]

#1614102

On Fri, Mar 31, 2017 at 04:40:56PM +0200, Juergen Gross wrote:
> There are several functions in xen-acpi-processor which either always
> return the same value or where the returned value is never checked.
> 
> Make the functions return void.

Well, we could actually check it and do some extra error reporting?
(Does it even make sense?)

Granted the code was done this way so that if we did fail - we would
try to continue on instead of giving up.

Perhaps you can include that in the commit description? Thanks
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  drivers/xen/xen-acpi-processor.c | 51 +++++++++++++++-------------------------
>  1 file changed, 19 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
> index 23e391d..45be017 100644
> --- a/drivers/xen/xen-acpi-processor.c
> +++ b/drivers/xen/xen-acpi-processor.c
> @@ -54,7 +54,7 @@ static unsigned long *acpi_id_present;
>  /* And if there is an _CST definition (or a PBLK) for the ACPI IDs */
>  static unsigned long *acpi_id_cst_present;
>  
> -static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
> +static void push_cxx_to_hypervisor(struct acpi_processor *_pr)
>  {
>  	struct xen_platform_op op = {
>  		.cmd			= XENPF_set_processor_pminfo,
> @@ -70,7 +70,7 @@ static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
>  	dst_cx_states = kcalloc(_pr->power.count,
>  				sizeof(struct xen_processor_cx), GFP_KERNEL);
>  	if (!dst_cx_states)
> -		return -ENOMEM;
> +		return;
>  
>  	for (ok = 0, i = 1; i <= _pr->power.count; i++) {
>  		cx = &_pr->power.states[i];
> @@ -104,7 +104,7 @@ static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
>  	if (!ok) {
>  		pr_debug("No _Cx for ACPI CPU %u\n", _pr->acpi_id);
>  		kfree(dst_cx_states);
> -		return -EINVAL;
> +		return;
>  	}
>  	op.u.set_pminfo.power.count = ok;
>  	op.u.set_pminfo.power.flags.bm_control = _pr->flags.bm_control;
> @@ -135,8 +135,6 @@ static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
>  		       ret, _pr->acpi_id);
>  
>  	kfree(dst_cx_states);
> -
> -	return ret;
>  }
>  static struct xen_processor_px *
>  xen_copy_pss_data(struct acpi_processor *_pr,
> @@ -161,8 +159,8 @@ xen_copy_pss_data(struct acpi_processor *_pr,
>  	}
>  	return dst_states;
>  }
> -static int xen_copy_psd_data(struct acpi_processor *_pr,
> -			     struct xen_processor_performance *dst)
> +static void xen_copy_psd_data(struct acpi_processor *_pr,
> +			      struct xen_processor_performance *dst)
>  {
>  	struct acpi_psd_package *pdomain;
>  
> @@ -189,10 +187,9 @@ static int xen_copy_psd_data(struct acpi_processor *_pr,
>  
>  	}
>  	memcpy(&(dst->domain_info), pdomain, sizeof(struct acpi_psd_package));
> -	return 0;
>  }
> -static int xen_copy_pct_data(struct acpi_pct_register *pct,
> -			     struct xen_pct_register *dst_pct)
> +static void xen_copy_pct_data(struct acpi_pct_register *pct,
> +			      struct xen_pct_register *dst_pct)
>  {
>  	/* It would be nice if you could just do 'memcpy(pct, dst_pct') but
>  	 * sadly the Xen structure did not have the proper padding so the
> @@ -205,9 +202,8 @@ static int xen_copy_pct_data(struct acpi_pct_register *pct,
>  	dst_pct->bit_offset = pct->bit_offset;
>  	dst_pct->reserved = pct->reserved;
>  	dst_pct->address = pct->address;
> -	return 0;
>  }
> -static int push_pxx_to_hypervisor(struct acpi_processor *_pr)
> +static void push_pxx_to_hypervisor(struct acpi_processor *_pr)
>  {
>  	int ret = 0;
>  	struct xen_platform_op op = {
> @@ -233,13 +229,12 @@ static int push_pxx_to_hypervisor(struct acpi_processor *_pr)
>  		set_xen_guest_handle(dst_perf->states, dst_states);
>  		dst_perf->flags |= XEN_PX_PSS;
>  	}
> -	if (!xen_copy_psd_data(_pr, dst_perf))
> -		dst_perf->flags |= XEN_PX_PSD;
> +	xen_copy_psd_data(_pr, dst_perf);
> +	dst_perf->flags |= XEN_PX_PSD;
>  
>  	if (dst_perf->flags != (XEN_PX_PSD | XEN_PX_PSS | XEN_PX_PCT | XEN_PX_PPC)) {
>  		pr_warn("ACPI CPU%u missing some P-state data (%x), skipping\n",
>  			_pr->acpi_id, dst_perf->flags);
> -		ret = -ENODEV;
>  		goto err_free;
>  	}
>  
> @@ -268,26 +263,18 @@ static int push_pxx_to_hypervisor(struct acpi_processor *_pr)
>  err_free:
>  	if (!IS_ERR_OR_NULL(dst_states))
>  		kfree(dst_states);
> -
> -	return ret;
>  }
> -static int upload_pm_data(struct acpi_processor *_pr)
> +static void upload_pm_data(struct acpi_processor *_pr)
>  {
> -	int err = 0;
> -
>  	mutex_lock(&acpi_ids_mutex);
> -	if (__test_and_set_bit(_pr->acpi_id, acpi_ids_done)) {
> -		mutex_unlock(&acpi_ids_mutex);
> -		return -EBUSY;
> +	if (!__test_and_set_bit(_pr->acpi_id, acpi_ids_done)) {
> +		if (_pr->flags.power)
> +			push_cxx_to_hypervisor(_pr);
> +
> +		if (_pr->performance && _pr->performance->states)
> +			push_pxx_to_hypervisor(_pr);
>  	}
> -	if (_pr->flags.power)
> -		err = push_cxx_to_hypervisor(_pr);
> -
> -	if (_pr->performance && _pr->performance->states)
> -		err |= push_pxx_to_hypervisor(_pr);
> -
>  	mutex_unlock(&acpi_ids_mutex);
> -	return err;
>  }
>  static unsigned int __init get_max_acpi_id(void)
>  {
> @@ -417,7 +404,7 @@ static int check_acpi_ids(struct acpi_processor *pr_backup)
>  			pr_backup->acpi_id = i;
>  			/* Mask out C-states if there are no _CST or PBLK */
>  			pr_backup->flags.power = test_bit(i, acpi_id_cst_present);
> -			(void)upload_pm_data(pr_backup);
> +			upload_pm_data(pr_backup);
>  		}
>  	}
>  
> @@ -457,7 +444,7 @@ static int xen_upload_processor_pm_data(void)
>  			if (pr_backup)
>  				memcpy(pr_backup, _pr, sizeof(struct acpi_processor));
>  		}
> -		(void)upload_pm_data(_pr);
> +		upload_pm_data(_pr);
>  	}
>  
>  	rc = check_acpi_ids(pr_backup);
> -- 
> 2.10.2
> 

[toc] | [prev] | [next] | [standalone]

#1614124

On 31/03/17 17:15, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 31, 2017 at 04:40:56PM +0200, Juergen Gross wrote:
>> There are several functions in xen-acpi-processor which either always
>> return the same value or where the returned value is never checked.
>>
>> Make the functions return void.
> 
> Well, we could actually check it and do some extra error reporting?
> (Does it even make sense?)
> 
> Granted the code was done this way so that if we did fail - we would
> try to continue on instead of giving up.
> 
> Perhaps you can include that in the commit description? Thanks

Okay. I'll change the commit description.


Juergen

[toc] | [prev] | [next] | [standalone]

#1614104

On 03/31/2017 10:40 AM, Juergen Gross wrote:
> There are several functions in xen-acpi-processor which either always
> return the same value or where the returned value is never checked.
>
> Make the functions return void.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  drivers/xen/xen-acpi-processor.c | 51 +++++++++++++++-------------------------
>  1 file changed, 19 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
> index 23e391d..45be017 100644
> --- a/drivers/xen/xen-acpi-processor.c
> +++ b/drivers/xen/xen-acpi-processor.c
> @@ -54,7 +54,7 @@ static unsigned long *acpi_id_present;
>  /* And if there is an _CST definition (or a PBLK) for the ACPI IDs */
>  static unsigned long *acpi_id_cst_present;
>  
> -static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
> +static void push_cxx_to_hypervisor(struct acpi_processor *_pr)
>  {
>  	struct xen_platform_op op = {
>  		.cmd			= XENPF_set_processor_pminfo,
> @@ -70,7 +70,7 @@ static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
>  	dst_cx_states = kcalloc(_pr->power.count,
>  				sizeof(struct xen_processor_cx), GFP_KERNEL);
>  	if (!dst_cx_states)
> -		return -ENOMEM;
> +		return;

Maybe pr_warn(_once)()?

In any case:

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

[toc] | [prev] | [next] | [standalone]

#1614121

On 31/03/17 17:13, Boris Ostrovsky wrote:
> On 03/31/2017 10:40 AM, Juergen Gross wrote:
>> There are several functions in xen-acpi-processor which either always
>> return the same value or where the returned value is never checked.
>>
>> Make the functions return void.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  drivers/xen/xen-acpi-processor.c | 51 +++++++++++++++-------------------------
>>  1 file changed, 19 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
>> index 23e391d..45be017 100644
>> --- a/drivers/xen/xen-acpi-processor.c
>> +++ b/drivers/xen/xen-acpi-processor.c
>> @@ -54,7 +54,7 @@ static unsigned long *acpi_id_present;
>>  /* And if there is an _CST definition (or a PBLK) for the ACPI IDs */
>>  static unsigned long *acpi_id_cst_present;
>>  
>> -static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
>> +static void push_cxx_to_hypervisor(struct acpi_processor *_pr)
>>  {
>>  	struct xen_platform_op op = {
>>  		.cmd			= XENPF_set_processor_pminfo,
>> @@ -70,7 +70,7 @@ static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
>>  	dst_cx_states = kcalloc(_pr->power.count,
>>  				sizeof(struct xen_processor_cx), GFP_KERNEL);
>>  	if (!dst_cx_states)
>> -		return -ENOMEM;
>> +		return;
> 
> Maybe pr_warn(_once)()?

I don't think so. Memory shortage is probably detectable without that
message. Adding another (random) message will do more harm than good.


Juergen

> 
> In any case:
> 
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web