[PATCH] Avoid wiping out pr->performance during preregistering

[PATCH] Avoid wiping out pr->performance during preregistering

[Stanislaw Gruszka]

When cpufreq driver call acpi_processor_preregister_performance() , function
will clean up pr->performance even if there is possibly already registered
other cpufreq driver. The patch fix this potential problem. It also remove 
double checks in P domain basic validity code and move these checks to function
where _PSD data is captured.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/acpi/processor_perflib.c |   46 +++++++++++++++++--------------------
 1 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
index 9cc769b..215f1bf 100644
--- a/drivers/acpi/processor_perflib.c
+++ b/drivers/acpi/processor_perflib.c
@@ -479,6 +479,13 @@ static int acpi_processor_get_psd(struct acpi_processor	*pr)
 		goto end;
 	}
 
+	if (pdomain->coord_type != DOMAIN_COORD_TYPE_SW_ALL &&
+	    pdomain->coord_type != DOMAIN_COORD_TYPE_SW_ANY &&
+	    pdomain->coord_type != DOMAIN_COORD_TYPE_HW_ALL) {
+		printk(KERN_ERR PREFIX "Invalid _PSD:coord_type\n");
+		result = -EFAULT;
+		goto end;
+	}
 end:
 	kfree(buffer.pointer);
 	return result;
@@ -501,9 +508,10 @@ int acpi_processor_preregister_performance(
 
 	mutex_lock(&performance_mutex);
 
-	retval = 0;
-
-	/* Call _PSD for all CPUs */
+	/*
+	 * Check if another driver has already registered, and abort before
+	 * changing pr->performance if it has. Check input data as well.
+	 */
 	for_each_possible_cpu(i) {
 		pr = per_cpu(processors, i);
 		if (!pr) {
@@ -513,13 +521,20 @@ int acpi_processor_preregister_performance(
 
 		if (pr->performance) {
 			retval = -EBUSY;
-			continue;
+			goto err_out;
 		}
 
 		if (!performance || !percpu_ptr(performance, i)) {
 			retval = -EINVAL;
-			continue;
+			goto err_out;
 		}
+	}
+
+	/* Call _PSD for all CPUs */
+	for_each_possible_cpu(i) {
+		pr = per_cpu(processors, i);
+		if (!pr)
+			continue;
 
 		pr->performance = percpu_ptr(performance, i);
 		cpumask_set_cpu(i, pr->performance->shared_cpu_map);
@@ -535,26 +550,6 @@ int acpi_processor_preregister_performance(
 	 * Now that we have _PSD data from all CPUs, lets setup P-state 
 	 * domain info.
 	 */
-	for_each_possible_cpu(i) {
-		pr = per_cpu(processors, i);
-		if (!pr)
-			continue;
-
-		/* Basic validity check for domain info */
-		pdomain = &(pr->performance->domain_info);
-		if ((pdomain->revision != ACPI_PSD_REV0_REVISION) ||
-		    (pdomain->num_entries != ACPI_PSD_REV0_ENTRIES)) {
-			retval = -EINVAL;
-			goto err_ret;
-		}
-		if (pdomain->coord_type != DOMAIN_COORD_TYPE_SW_ALL &&
-		    pdomain->coord_type != DOMAIN_COORD_TYPE_SW_ANY &&
-		    pdomain->coord_type != DOMAIN_COORD_TYPE_HW_ALL) {
-			retval = -EINVAL;
-			goto err_ret;
-		}
-	}
-
 	cpumask_clear(covered_cpus);
 	for_each_possible_cpu(i) {
 		pr = per_cpu(processors, i);
@@ -643,6 +638,7 @@ err_ret:
 		pr->performance = NULL; /* Will be set for real in register */
 	}
 
+err_out:
 	mutex_unlock(&performance_mutex);
 	free_cpumask_var(covered_cpus);
 	return retval;
-- 
1.6.0.6

Re: [PATCH] Avoid wiping out pr->performance during preregistering

[Len Brown]

On Tue, 24 Mar 2009, Stanislaw Gruszka wrote:

> When cpufreq driver call acpi_processor_preregister_performance() , function
> will clean up pr->performance even if there is possibly already registered
> other cpufreq driver. The patch fix this potential problem. It also remove 
> double checks in P domain basic validity code and move these checks to function
> where _PSD data is captured.
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  drivers/acpi/processor_perflib.c |   46 +++++++++++++++++--------------------
>  1 files changed, 21 insertions(+), 25 deletions(-)
> 

What failure symptom does the user see before this patch is applied?
Does it merit backporting to .stable?

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH] Avoid wiping out pr->performance during preregistering

[Stanislaw Gruszka]

On Fri, 27 Mar 2009 21:15:23 -0400 (EDT)
Len Brown <lenb@kernel.org> wrote:

> > When cpufreq driver call acpi_processor_preregister_performance() , function
> > will clean up pr->performance even if there is possibly already registered
> > other cpufreq driver. The patch fix this potential problem. It also remove 
> > double checks in P domain basic validity code and move these checks to function
> > where _PSD data is captured.
> > 
> > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > ---
> >  drivers/acpi/processor_perflib.c |   46 +++++++++++++++++--------------------
> >  1 files changed, 21 insertions(+), 25 deletions(-)
> > 
> 
> What failure symptom does the user see before this patch is applied?
> Does it merit backporting to .stable?

Nothing destructive, kernel ignore P-states limit requested by BIOS using
ACPI _PPC . I think this patch is not so good candidate for backporting.

Cheers
Stanislaw Gruszka