* [PATCH 0/2] aggregator device driver update @ 2009-12-14 1:42 Chen Gong 2009-12-14 1:42 ` [PATCH 1/2] fix some errors in aggregator device driver Chen Gong 2009-12-16 4:00 ` [PATCH 0/2] aggregator device driver update Len Brown 0 siblings, 2 replies; 6+ messages in thread From: Chen Gong @ 2009-12-14 1:42 UTC (permalink / raw) To: linux-acpi; +Cc: lenb 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] fix some errors in aggregator device driver 2009-12-14 1:42 [PATCH 0/2] aggregator device driver update Chen Gong @ 2009-12-14 1:42 ` Chen Gong 2009-12-14 1:42 ` [PATCH 2/2] update mutex usage in aggregator driver Chen Gong 2009-12-16 4:00 ` [PATCH 0/2] aggregator device driver update Len Brown 1 sibling, 1 reply; 6+ messages in thread From: Chen Gong @ 2009-12-14 1:42 UTC (permalink / raw) To: linux-acpi; +Cc: lenb, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] update mutex usage in aggregator driver 2009-12-14 1:42 ` [PATCH 1/2] fix some errors in aggregator device driver Chen Gong @ 2009-12-14 1:42 ` Chen Gong 2009-12-17 0:54 ` Shaohua Li 0 siblings, 1 reply; 6+ messages in thread From: Chen Gong @ 2009-12-14 1:42 UTC (permalink / raw) To: linux-acpi; +Cc: lenb, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] update mutex usage in aggregator driver 2009-12-14 1:42 ` [PATCH 2/2] update mutex usage in aggregator driver Chen Gong @ 2009-12-17 0:54 ` Shaohua Li 2009-12-17 4:52 ` chen gong 0 siblings, 1 reply; 6+ messages in thread From: Shaohua Li @ 2009-12-17 0:54 UTC (permalink / raw) To: Chen Gong; +Cc: linux-acpi@vger.kernel.org, lenb@kernel.org 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] update mutex usage in aggregator driver 2009-12-17 0:54 ` Shaohua Li @ 2009-12-17 4:52 ` chen gong 0 siblings, 0 replies; 6+ messages in thread From: chen gong @ 2009-12-17 4:52 UTC (permalink / raw) To: Shaohua Li; +Cc: linux-acpi@vger.kernel.org, lenb@kernel.org 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] aggregator device driver update 2009-12-14 1:42 [PATCH 0/2] aggregator device driver update Chen Gong 2009-12-14 1:42 ` [PATCH 1/2] fix some errors in aggregator device driver Chen Gong @ 2009-12-16 4:00 ` Len Brown 1 sibling, 0 replies; 6+ messages in thread From: Len Brown @ 2009-12-16 4:00 UTC (permalink / raw) To: Chen Gong; +Cc: linux-acpi, Shaohua Li applied to acpi-test, pending shaohua's ack thanks, Len Brown, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-12-17 4:52 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-12-14 1:42 [PATCH 0/2] aggregator device driver update Chen Gong 2009-12-14 1:42 ` [PATCH 1/2] fix some errors in aggregator device driver Chen Gong 2009-12-14 1:42 ` [PATCH 2/2] update mutex usage in aggregator driver Chen Gong 2009-12-17 0:54 ` Shaohua Li 2009-12-17 4:52 ` chen gong 2009-12-16 4:00 ` [PATCH 0/2] aggregator device driver update Len Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox