[PATCH 0/2] aggregator device driver update

[PATCH 0/2] aggregator device driver update

[Chen Gong]

I'v made some update for this driver. The 1st one is for error fix and
enhancement. And the other is to update mutex usage.

[PATCH 1/2] fix some errors in aggregator device driver

[Chen Gong]

There are some fixes listed below:
1. When met a bogus BIOS, the return value of cpu number maybe is
   a negative value so that acpi_pad_pur get an unexpected result.
2. the return value of function acpi_pad_idle_cpus is useless.
3. enhance the process of create_power_saving_task/destroy_power_saving_task
4. Add more error checks when evaluating _PUR object.
5. one typo fix

Signed-off-by: Chen Gong <gong.chen@linux.intel.com>
---
 drivers/acpi/acpi_pad.c |   37 ++++++++++++++++++++-----------------
 1 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/acpi_pad.c b/drivers/acpi/acpi_pad.c
index 0d2cdb8..a7bd49f 100644
--- a/drivers/acpi/acpi_pad.c
+++ b/drivers/acpi/acpi_pad.c
@@ -207,7 +207,7 @@ static int power_saving_thread(void *data)
 		 * the mechanism only works when all CPUs have RT task running,
 		 * as if one CPU hasn't RT task, RT task from other CPUs will
 		 * borrow CPU time from this CPU and cause RT task use > 95%
-		 * CPU time. To make 'avoid staration' work, takes a nap here.
+		 * CPU time. To make 'avoid starvation' work, takes a nap here.
 		 */
 		if (do_sleep)
 			schedule_timeout_killable(HZ * idle_pct / 100);
@@ -221,14 +221,18 @@ static struct task_struct *ps_tsks[NR_CPUS];
 static unsigned int ps_tsk_num;
 static int create_power_saving_task(void)
 {
+	int rc = -ENOMEM;
+
 	ps_tsks[ps_tsk_num] = kthread_run(power_saving_thread,
 		(void *)(unsigned long)ps_tsk_num,
 		"power_saving/%d", ps_tsk_num);
-	if (ps_tsks[ps_tsk_num]) {
+	rc = IS_ERR(ps_tsks[ps_tsk_num]) ? PTR_ERR(ps_tsks[ps_tsk_num]) : 0;
+	if (!rc)
 		ps_tsk_num++;
-		return 0;
-	}
-	return -EINVAL;
+	else
+		ps_tsks[ps_tsk_num] = NULL;
+
+	return rc;
 }
 
 static void destroy_power_saving_task(void)
@@ -236,6 +240,7 @@ static void destroy_power_saving_task(void)
 	if (ps_tsk_num > 0) {
 		ps_tsk_num--;
 		kthread_stop(ps_tsks[ps_tsk_num]);
+		ps_tsks[ps_tsk_num] = NULL;
 	}
 }
 
@@ -252,7 +257,7 @@ static void set_power_saving_task_num(unsigned int num)
 	}
 }
 
-static int acpi_pad_idle_cpus(unsigned int num_cpus)
+static void acpi_pad_idle_cpus(unsigned int num_cpus)
 {
 	get_online_cpus();
 
@@ -260,7 +265,6 @@ static int acpi_pad_idle_cpus(unsigned int num_cpus)
 	set_power_saving_task_num(num_cpus);
 
 	put_online_cpus();
-	return 0;
 }
 
 static uint32_t acpi_pad_idle_cpus_num(void)
@@ -368,19 +372,21 @@ static void acpi_pad_remove_sysfs(struct acpi_device *device)
 static int acpi_pad_pur(acpi_handle handle, int *num_cpus)
 {
 	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
-	acpi_status status;
 	union acpi_object *package;
 	int rev, num, ret = -EINVAL;
 
-	status = acpi_evaluate_object(handle, "_PUR", NULL, &buffer);
-	if (ACPI_FAILURE(status))
+	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_PUR", NULL, &buffer)))
+		return -EINVAL;
+
+	if (!buffer.length || !buffer.pointer)
 		return -EINVAL;
+
 	package = buffer.pointer;
 	if (package->type != ACPI_TYPE_PACKAGE || package->package.count != 2)
 		goto out;
 	rev = package->package.elements[0].integer.value;
 	num = package->package.elements[1].integer.value;
-	if (rev != 1)
+	if (rev != 1 || num < 0)
 		goto out;
 	*num_cpus = num;
 	ret = 0;
@@ -409,7 +415,7 @@ static void acpi_pad_ost(acpi_handle handle, int stat,
 
 static void acpi_pad_handle_notify(acpi_handle handle)
 {
-	int num_cpus, ret;
+	int num_cpus;
 	uint32_t idle_cpus;
 
 	mutex_lock(&isolated_cpus_lock);
@@ -417,12 +423,9 @@ static void acpi_pad_handle_notify(acpi_handle handle)
 		mutex_unlock(&isolated_cpus_lock);
 		return;
 	}
-	ret = acpi_pad_idle_cpus(num_cpus);
+	acpi_pad_idle_cpus(num_cpus);
 	idle_cpus = acpi_pad_idle_cpus_num();
-	if (!ret)
-		acpi_pad_ost(handle, 0, idle_cpus);
-	else
-		acpi_pad_ost(handle, 1, 0);
+	acpi_pad_ost(handle, 0, idle_cpus);
 	mutex_unlock(&isolated_cpus_lock);
 }
 
-- 
1.6.5.2.143.g8cc62

[PATCH 2/2] update mutex usage in aggregator driver

[Chen Gong]

I have 2 issues.
 1. I consider thread creation and RR thread execution don't share the
global data, so the serialization execution is not necessary. It is
enough to use another different mutex to lock thread exectuion itself
when these threads running concurrently.
 2. part of sysfs interfaces doesn't need to own mutex with thread createion.
Though some data maybe are not identical, it is not a big deal.

Signed-off-by: Chen Gong <gong.chen@linux.intel.com>
---
 drivers/acpi/acpi_pad.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/acpi_pad.c b/drivers/acpi/acpi_pad.c
index a7bd49f..ceb8270 100644
--- a/drivers/acpi/acpi_pad.c
+++ b/drivers/acpi/acpi_pad.c
@@ -34,6 +34,7 @@
 #define ACPI_PROCESSOR_AGGREGATOR_DEVICE_NAME "Processor Aggregator"
 #define ACPI_PROCESSOR_AGGREGATOR_NOTIFY 0x80
 static DEFINE_MUTEX(isolated_cpus_lock);
+static DEFINE_MUTEX(isolated_thread_lock);
 
 #define MWAIT_SUBSTATE_MASK	(0xf)
 #define MWAIT_CSTATE_MASK	(0xf)
@@ -105,7 +106,6 @@ static void round_robin_cpu(unsigned int tsk_index)
 	if (!alloc_cpumask_var(&tmp, GFP_KERNEL))
 		return;
 
-	mutex_lock(&isolated_cpus_lock);
 	cpumask_clear(tmp);
 	for_each_cpu(cpu, pad_busy_cpus)
 		cpumask_or(tmp, tmp, topology_thread_cpumask(cpu));
@@ -113,10 +113,10 @@ static void round_robin_cpu(unsigned int tsk_index)
 	/* avoid HT sibilings if possible */
 	if (cpumask_empty(tmp))
 		cpumask_andnot(tmp, cpu_online_mask, pad_busy_cpus);
-	if (cpumask_empty(tmp)) {
-		mutex_unlock(&isolated_cpus_lock);
+	if (cpumask_empty(tmp))
 		return;
-	}
+
+	mutex_lock(&isolated_thread_lock);
 	for_each_cpu(cpu, tmp) {
 		if (cpu_weight[cpu] < min_weight) {
 			min_weight = cpu_weight[cpu];
@@ -129,7 +129,7 @@ static void round_robin_cpu(unsigned int tsk_index)
 	tsk_in_cpu[tsk_index] = preferred_cpu;
 	cpumask_set_cpu(preferred_cpu, pad_busy_cpus);
 	cpu_weight[preferred_cpu]++;
-	mutex_unlock(&isolated_cpus_lock);
+	mutex_unlock(&isolated_thread_lock);
 
 	set_cpus_allowed_ptr(current, cpumask_of(preferred_cpu));
 }
@@ -280,9 +280,7 @@ static ssize_t acpi_pad_rrtime_store(struct device *dev,
 		return -EINVAL;
 	if (num < 1 || num >= 100)
 		return -EINVAL;
-	mutex_lock(&isolated_cpus_lock);
 	round_robin_time = num;
-	mutex_unlock(&isolated_cpus_lock);
 	return count;
 }
 
@@ -303,9 +301,7 @@ static ssize_t acpi_pad_idlepct_store(struct device *dev,
 		return -EINVAL;
 	if (num < 1 || num >= 100)
 		return -EINVAL;
-	mutex_lock(&isolated_cpus_lock);
 	idle_pct = num;
-	mutex_unlock(&isolated_cpus_lock);
 	return count;
 }
 
-- 
1.6.5.2.143.g8cc62

Re: [PATCH 0/2] aggregator device driver update

[Len Brown]

applied to acpi-test, pending shaohua's ack

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 2/2] update mutex usage in aggregator driver

[Shaohua Li]

On Mon, Dec 14, 2009 at 09:42:29AM +0800, Chen Gong wrote:
> I have 2 issues.
>  1. I consider thread creation and RR thread execution don't share the
> global data, so the serialization execution is not necessary. It is
> enough to use another different mutex to lock thread exectuion itself
> when these threads running concurrently.
>  2. part of sysfs interfaces doesn't need to own mutex with thread createion.
> Though some data maybe are not identical, it is not a big deal.
ACK the first patch.
NACK this one. I don't see any reason we need two locks here. This isn't hot
code path, fine-grained lock just makes thing complex.

Thanks,
Shaohua 

Re: [PATCH 2/2] update mutex usage in aggregator driver

[chen gong]

On 2009-12-17 8:54, Shaohua Li wrote:
> On Mon, Dec 14, 2009 at 09:42:29AM +0800, Chen Gong wrote:
>> I have 2 issues.
>>   1. I consider thread creation and RR thread execution don't share the
>> global data, so the serialization execution is not necessary. It is
>> enough to use another different mutex to lock thread exectuion itself
>> when these threads running concurrently.
>>   2. part of sysfs interfaces doesn't need to own mutex with thread createion.
>> Though some data maybe are not identical, it is not a big deal.
> ACK the first patch.
> NACK this one. I don't see any reason we need two locks here. This isn't hot
> code path, fine-grained lock just makes thing complex.
>
> Thanks,
> Shaohua
>
I don't think so. Though it is not critical code path but it is kernel
code. It should be made more consideration.