* [PATCH v2] xen: make functions in xen-acpi-processor return void
@ 2017-03-31 14:40 Juergen Gross
2017-03-31 15:13 ` Boris Ostrovsky
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Juergen Gross @ 2017-03-31 14:40 UTC (permalink / raw)
To: linux-kernel, xen-devel; +Cc: boris.ostrovsky, konrad.wilk, Juergen Gross
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] xen: make functions in xen-acpi-processor return void
2017-03-31 14:40 [PATCH v2] xen: make functions in xen-acpi-processor return void Juergen Gross
2017-03-31 15:13 ` Boris Ostrovsky
@ 2017-03-31 15:13 ` Boris Ostrovsky
2017-03-31 15:29 ` Juergen Gross
2017-03-31 15:29 ` Juergen Gross
2017-03-31 15:15 ` Konrad Rzeszutek Wilk
2017-03-31 15:15 ` Konrad Rzeszutek Wilk
3 siblings, 2 replies; 10+ messages in thread
From: Boris Ostrovsky @ 2017-03-31 15:13 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel; +Cc: konrad.wilk
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] xen: make functions in xen-acpi-processor return void
2017-03-31 14:40 [PATCH v2] xen: make functions in xen-acpi-processor return void Juergen Gross
@ 2017-03-31 15:13 ` Boris Ostrovsky
2017-03-31 15:13 ` Boris Ostrovsky
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Boris Ostrovsky @ 2017-03-31 15:13 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, xen-devel
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>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] xen: make functions in xen-acpi-processor return void
2017-03-31 14:40 [PATCH v2] xen: make functions in xen-acpi-processor return void Juergen Gross
` (2 preceding siblings ...)
2017-03-31 15:15 ` Konrad Rzeszutek Wilk
@ 2017-03-31 15:15 ` Konrad Rzeszutek Wilk
2017-03-31 15:30 ` Juergen Gross
2017-03-31 15:30 ` Juergen Gross
3 siblings, 2 replies; 10+ messages in thread
From: Konrad Rzeszutek Wilk @ 2017-03-31 15:15 UTC (permalink / raw)
To: Juergen Gross; +Cc: linux-kernel, xen-devel, boris.ostrovsky
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] xen: make functions in xen-acpi-processor return void
2017-03-31 14:40 [PATCH v2] xen: make functions in xen-acpi-processor return void Juergen Gross
2017-03-31 15:13 ` Boris Ostrovsky
2017-03-31 15:13 ` Boris Ostrovsky
@ 2017-03-31 15:15 ` Konrad Rzeszutek Wilk
2017-03-31 15:15 ` Konrad Rzeszutek Wilk
3 siblings, 0 replies; 10+ messages in thread
From: Konrad Rzeszutek Wilk @ 2017-03-31 15:15 UTC (permalink / raw)
To: Juergen Gross; +Cc: xen-devel, boris.ostrovsky, linux-kernel
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
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] xen: make functions in xen-acpi-processor return void
2017-03-31 15:13 ` Boris Ostrovsky
@ 2017-03-31 15:29 ` Juergen Gross
2017-03-31 15:29 ` Juergen Gross
1 sibling, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2017-03-31 15:29 UTC (permalink / raw)
To: Boris Ostrovsky, linux-kernel, xen-devel; +Cc: konrad.wilk
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>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] xen: make functions in xen-acpi-processor return void
2017-03-31 15:13 ` Boris Ostrovsky
2017-03-31 15:29 ` Juergen Gross
@ 2017-03-31 15:29 ` Juergen Gross
1 sibling, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2017-03-31 15:29 UTC (permalink / raw)
To: Boris Ostrovsky, linux-kernel, xen-devel
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>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] xen: make functions in xen-acpi-processor return void
2017-03-31 15:15 ` Konrad Rzeszutek Wilk
@ 2017-03-31 15:30 ` Juergen Gross
2017-03-31 15:30 ` Juergen Gross
1 sibling, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2017-03-31 15:30 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: linux-kernel, xen-devel, boris.ostrovsky
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] xen: make functions in xen-acpi-processor return void
2017-03-31 15:15 ` Konrad Rzeszutek Wilk
2017-03-31 15:30 ` Juergen Gross
@ 2017-03-31 15:30 ` Juergen Gross
1 sibling, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2017-03-31 15:30 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: xen-devel, boris.ostrovsky, linux-kernel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] xen: make functions in xen-acpi-processor return void
@ 2017-03-31 14:40 Juergen Gross
0 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2017-03-31 14:40 UTC (permalink / raw)
To: linux-kernel, xen-devel; +Cc: Juergen Gross, boris.ostrovsky
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-03-31 15:30 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-31 14:40 [PATCH v2] xen: make functions in xen-acpi-processor return void Juergen Gross
2017-03-31 15:13 ` Boris Ostrovsky
2017-03-31 15:13 ` Boris Ostrovsky
2017-03-31 15:29 ` Juergen Gross
2017-03-31 15:29 ` Juergen Gross
2017-03-31 15:15 ` Konrad Rzeszutek Wilk
2017-03-31 15:15 ` Konrad Rzeszutek Wilk
2017-03-31 15:30 ` Juergen Gross
2017-03-31 15:30 ` Juergen Gross
2017-03-31 14:40 Juergen Gross
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.