[PATCH 0/2] xen: fix and cleanup of xen-acpi-processor

[PATCH 0/2] xen: fix and cleanup of xen-acpi-processor

[Juergen Gross]

Juergen Gross (2):
  xen: correct error handling in xen-acpi-processor
  xen: make functions in xen-acpi-processor return void

 drivers/xen/xen-acpi-processor.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

-- 
2.10.2

[PATCH 1/2] xen: correct error handling in xen-acpi-processor

[Juergen Gross]

Error handling in upload_pm_data() is rather questionable: possible
errno values from called functions are or'ed together resulting in
strange return values: -EINVAL and -ENOMEM would e.g. result in
returning -EXDEV.

Fix this by bailing out early after the first error.

Cc: stable@vger.kernel.org
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/xen-acpi-processor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index 23e391d..3659ed3 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -283,8 +283,8 @@ static int upload_pm_data(struct acpi_processor *_pr)
 	if (_pr->flags.power)
 		err = push_cxx_to_hypervisor(_pr);
 
-	if (_pr->performance && _pr->performance->states)
-		err |= push_pxx_to_hypervisor(_pr);
+	if (!err && _pr->performance && _pr->performance->states)
+		err = push_pxx_to_hypervisor(_pr);
 
 	mutex_unlock(&acpi_ids_mutex);
 	return err;
-- 
2.10.2

[PATCH 1/2] xen: correct error handling in xen-acpi-processor

[Juergen Gross]

Error handling in upload_pm_data() is rather questionable: possible
errno values from called functions are or'ed together resulting in
strange return values: -EINVAL and -ENOMEM would e.g. result in
returning -EXDEV.

Fix this by bailing out early after the first error.

Cc: stable@vger.kernel.org
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/xen-acpi-processor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index 23e391d..3659ed3 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -283,8 +283,8 @@ static int upload_pm_data(struct acpi_processor *_pr)
 	if (_pr->flags.power)
 		err = push_cxx_to_hypervisor(_pr);
 
-	if (_pr->performance && _pr->performance->states)
-		err |= push_pxx_to_hypervisor(_pr);
+	if (!err && _pr->performance && _pr->performance->states)
+		err = push_pxx_to_hypervisor(_pr);
 
 	mutex_unlock(&acpi_ids_mutex);
 	return err;
-- 
2.10.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Juergen Gross]

The return value of xen_copy_pct_data() is always 0 and never checked.
xen_copy_psd_data() returns always 0, too, so checking the value is
kind of pointless.

Make the functions return void.

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

diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index 3659ed3..8f01585 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -161,8 +161,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 +189,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,7 +204,6 @@ 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)
 {
@@ -233,8 +231,8 @@ 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",
-- 
2.10.2

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

[Juergen Gross]

The return value of xen_copy_pct_data() is always 0 and never checked.
xen_copy_psd_data() returns always 0, too, so checking the value is
kind of pointless.

Make the functions return void.

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

diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index 3659ed3..8f01585 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -161,8 +161,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 +189,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,7 +204,6 @@ 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)
 {
@@ -233,8 +231,8 @@ 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",
-- 
2.10.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] xen: correct error handling in xen-acpi-processor

[Boris Ostrovsky]

On 03/31/2017 07:19 AM, Juergen Gross wrote:
> Error handling in upload_pm_data() is rather questionable: possible
> errno values from called functions are or'ed together resulting in
> strange return values: -EINVAL and -ENOMEM would e.g. result in
> returning -EXDEV.

And it doesn't matter anyway since noone checks return value. (not that
OR-ing errors makes much sense)

>
> Fix this by bailing out early after the first error.

I am not sure about this: why should we not try to load P states if C
states failed to load?

-boris

>
> Cc: stable@vger.kernel.org
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  drivers/xen/xen-acpi-processor.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
> index 23e391d..3659ed3 100644
> --- a/drivers/xen/xen-acpi-processor.c
> +++ b/drivers/xen/xen-acpi-processor.c
> @@ -283,8 +283,8 @@ static int upload_pm_data(struct acpi_processor *_pr)
>  	if (_pr->flags.power)
>  		err = push_cxx_to_hypervisor(_pr);
>  
> -	if (_pr->performance && _pr->performance->states)
> -		err |= push_pxx_to_hypervisor(_pr);
> +	if (!err && _pr->performance && _pr->performance->states)
> +		err = push_pxx_to_hypervisor(_pr);
>  
>  	mutex_unlock(&acpi_ids_mutex);
>  	return err;

Re: [PATCH 1/2] xen: correct error handling in xen-acpi-processor

[Boris Ostrovsky]

On 03/31/2017 07:19 AM, Juergen Gross wrote:
> Error handling in upload_pm_data() is rather questionable: possible
> errno values from called functions are or'ed together resulting in
> strange return values: -EINVAL and -ENOMEM would e.g. result in
> returning -EXDEV.

And it doesn't matter anyway since noone checks return value. (not that
OR-ing errors makes much sense)

>
> Fix this by bailing out early after the first error.

I am not sure about this: why should we not try to load P states if C
states failed to load?

-boris

>
> Cc: stable@vger.kernel.org
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  drivers/xen/xen-acpi-processor.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
> index 23e391d..3659ed3 100644
> --- a/drivers/xen/xen-acpi-processor.c
> +++ b/drivers/xen/xen-acpi-processor.c
> @@ -283,8 +283,8 @@ static int upload_pm_data(struct acpi_processor *_pr)
>  	if (_pr->flags.power)
>  		err = push_cxx_to_hypervisor(_pr);
>  
> -	if (_pr->performance && _pr->performance->states)
> -		err |= push_pxx_to_hypervisor(_pr);
> +	if (!err && _pr->performance && _pr->performance->states)
> +		err = push_pxx_to_hypervisor(_pr);
>  
>  	mutex_unlock(&acpi_ids_mutex);
>  	return err;


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] xen: make functions in xen-acpi-processor return void

[Boris Ostrovsky]

On 03/31/2017 07:19 AM, Juergen Gross wrote:
> The return value of xen_copy_pct_data() is always 0 and never checked.
> xen_copy_psd_data() returns always 0, too, so checking the value is
> kind of pointless.
>
> Make the functions return void.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

Re: [PATCH 2/2] xen: make functions in xen-acpi-processor return void

[Boris Ostrovsky]

On 03/31/2017 07:19 AM, Juergen Gross wrote:
> The return value of xen_copy_pct_data() is always 0 and never checked.
> xen_copy_psd_data() returns always 0, too, so checking the value is
> kind of pointless.
>
> Make the functions return void.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] xen: correct error handling in xen-acpi-processor

[Juergen Gross]

On 31/03/17 16:10, Boris Ostrovsky wrote:
> On 03/31/2017 07:19 AM, Juergen Gross wrote:
>> Error handling in upload_pm_data() is rather questionable: possible
>> errno values from called functions are or'ed together resulting in
>> strange return values: -EINVAL and -ENOMEM would e.g. result in
>> returning -EXDEV.
> 
> And it doesn't matter anyway since noone checks return value. (not that
> OR-ing errors makes much sense)

Ha, you are right!

So I can just fold the patches into one and turn some more functions
into retuning void.


Juergen

> 
>>
>> Fix this by bailing out early after the first error.
> 
> I am not sure about this: why should we not try to load P states if C
> states failed to load?
> 
> -boris
> 
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  drivers/xen/xen-acpi-processor.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
>> index 23e391d..3659ed3 100644
>> --- a/drivers/xen/xen-acpi-processor.c
>> +++ b/drivers/xen/xen-acpi-processor.c
>> @@ -283,8 +283,8 @@ static int upload_pm_data(struct acpi_processor *_pr)
>>  	if (_pr->flags.power)
>>  		err = push_cxx_to_hypervisor(_pr);
>>  
>> -	if (_pr->performance && _pr->performance->states)
>> -		err |= push_pxx_to_hypervisor(_pr);
>> +	if (!err && _pr->performance && _pr->performance->states)
>> +		err = push_pxx_to_hypervisor(_pr);
>>  
>>  	mutex_unlock(&acpi_ids_mutex);
>>  	return err;
> 
> 

Re: [PATCH 1/2] xen: correct error handling in xen-acpi-processor

[Juergen Gross]

On 31/03/17 16:10, Boris Ostrovsky wrote:
> On 03/31/2017 07:19 AM, Juergen Gross wrote:
>> Error handling in upload_pm_data() is rather questionable: possible
>> errno values from called functions are or'ed together resulting in
>> strange return values: -EINVAL and -ENOMEM would e.g. result in
>> returning -EXDEV.
> 
> And it doesn't matter anyway since noone checks return value. (not that
> OR-ing errors makes much sense)

Ha, you are right!

So I can just fold the patches into one and turn some more functions
into retuning void.


Juergen

> 
>>
>> Fix this by bailing out early after the first error.
> 
> I am not sure about this: why should we not try to load P states if C
> states failed to load?
> 
> -boris
> 
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  drivers/xen/xen-acpi-processor.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
>> index 23e391d..3659ed3 100644
>> --- a/drivers/xen/xen-acpi-processor.c
>> +++ b/drivers/xen/xen-acpi-processor.c
>> @@ -283,8 +283,8 @@ static int upload_pm_data(struct acpi_processor *_pr)
>>  	if (_pr->flags.power)
>>  		err = push_cxx_to_hypervisor(_pr);
>>  
>> -	if (_pr->performance && _pr->performance->states)
>> -		err |= push_pxx_to_hypervisor(_pr);
>> +	if (!err && _pr->performance && _pr->performance->states)
>> +		err = push_pxx_to_hypervisor(_pr);
>>  
>>  	mutex_unlock(&acpi_ids_mutex);
>>  	return err;
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel