| Message ID | 1462268448-19954-2-git-send-email-akshay.adiga@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 03-05-16, 15:10, Akshay Adiga wrote: > Fixing a WARN_ON caused by smp_call_function_any() when irq is disabled, > because of changes made in the patch > ('cpufreq: powernv: Ramp-down global pstate slower than local-pstate') > https://patchwork.ozlabs.org/patch/612058/ > > WARNING: CPU: 0 PID: 4 at kernel/smp.c:291 > smp_call_function_single+0x170/0x180 > > Call Trace: > [c0000007f648f9f0] [c0000007f648fa90] 0xc0000007f648fa90 (unreliable) > [c0000007f648fa30] [c0000000001430e0] smp_call_function_any+0x170/0x1c0 > [c0000007f648fa90] [c0000000007b4b00] > powernv_cpufreq_target_index+0xe0/0x250 > [c0000007f648fb00] [c0000000007ac9dc] > __cpufreq_driver_target+0x20c/0x3d0 > [c0000007f648fbc0] [c0000000007b1b4c] od_dbs_timer+0xcc/0x260 > [c0000007f648fc10] [c0000000007b3024] dbs_work_handler+0x54/0xa0 > [c0000007f648fc50] [c0000000000c49a8] process_one_work+0x1d8/0x590 > [c0000007f648fce0] [c0000000000c4e08] worker_thread+0xa8/0x660 > [c0000007f648fd80] [c0000000000cca88] kthread+0x108/0x130 > [c0000007f648fe30] [c0000000000095e8] ret_from_kernel_thread+0x5c/0x74 > > Moving smp_call_function_any() out of the critical section and changing > irq safe spinlocks to normal spinlocks. > > Reported-by: Abdul Haleem <abdhalee@linux.vnet.linux.com> > Signed-off-by: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> > --- > Patch is based on Rafael's linux-next > drivers/cpufreq/powernv-cpufreq.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c > index 144c732..1f0e20c 100644 > --- a/drivers/cpufreq/powernv-cpufreq.c > +++ b/drivers/cpufreq/powernv-cpufreq.c > @@ -581,9 +581,10 @@ void gpstate_timer_handler(unsigned long data) > gpstates->last_gpstate = freq_data.gpstate_id; > gpstates->last_lpstate = freq_data.pstate_id; > > + spin_unlock(&gpstates->gpstate_lock); > + > /* Timer may get migrated to a different cpu on cpu hot unplug */ > smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1); > - spin_unlock(&gpstates->gpstate_lock); > } > > /* > @@ -596,7 +597,6 @@ static int powernv_cpufreq_target_index(struct cpufreq_policy *policy, > { > struct powernv_smp_call_data freq_data; > unsigned int cur_msec, gpstate_id; > - unsigned long flags; > struct global_pstate_info *gpstates = policy->driver_data; > > if (unlikely(rebooting) && new_index != get_nominal_index()) > @@ -607,7 +607,7 @@ static int powernv_cpufreq_target_index(struct cpufreq_policy *policy, > > cur_msec = jiffies_to_msecs(get_jiffies_64()); > > - spin_lock_irqsave(&gpstates->gpstate_lock, flags); > + spin_lock(&gpstates->gpstate_lock); You don't necessarily have to write 'what you are doing' in the commit log, but tell us why you are doing that. Please explain, why is this changed and why will things continue to work without this.
Hi Viresh, On 05/03/2016 05:19 PM, Viresh Kumar wrote: > On 03-05-16, 15:10, Akshay Adiga wrote: >> Fixing a WARN_ON caused by smp_call_function_any() when irq is disabled, >> because of changes made in the patch >> ('cpufreq: powernv: Ramp-down global pstate slower than local-pstate') >> https://patchwork.ozlabs.org/patch/612058/ >> >> WARNING: CPU: 0 PID: 4 at kernel/smp.c:291 >> smp_call_function_single+0x170/0x180 >> >> Call Trace: >> [c0000007f648f9f0] [c0000007f648fa90] 0xc0000007f648fa90 (unreliable) >> [c0000007f648fa30] [c0000000001430e0] smp_call_function_any+0x170/0x1c0 >> [c0000007f648fa90] [c0000000007b4b00] >> powernv_cpufreq_target_index+0xe0/0x250 >> [c0000007f648fb00] [c0000000007ac9dc] >> __cpufreq_driver_target+0x20c/0x3d0 >> [c0000007f648fbc0] [c0000000007b1b4c] od_dbs_timer+0xcc/0x260 >> [c0000007f648fc10] [c0000000007b3024] dbs_work_handler+0x54/0xa0 >> [c0000007f648fc50] [c0000000000c49a8] process_one_work+0x1d8/0x590 >> [c0000007f648fce0] [c0000000000c4e08] worker_thread+0xa8/0x660 >> [c0000007f648fd80] [c0000000000cca88] kthread+0x108/0x130 >> [c0000007f648fe30] [c0000000000095e8] ret_from_kernel_thread+0x5c/0x74 >> >> Moving smp_call_function_any() out of the critical section and changing >> irq safe spinlocks to normal spinlocks. >> >> Reported-by: Abdul Haleem <abdhalee@linux.vnet.linux.com> >> Signed-off-by: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> >> --- >> Patch is based on Rafael's linux-next >> drivers/cpufreq/powernv-cpufreq.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c >> index 144c732..1f0e20c 100644 >> --- a/drivers/cpufreq/powernv-cpufreq.c >> +++ b/drivers/cpufreq/powernv-cpufreq.c >> @@ -581,9 +581,10 @@ void gpstate_timer_handler(unsigned long data) >> gpstates->last_gpstate = freq_data.gpstate_id; >> gpstates->last_lpstate = freq_data.pstate_id; >> >> + spin_unlock(&gpstates->gpstate_lock); >> + >> /* Timer may get migrated to a different cpu on cpu hot unplug */ >> smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1); >> - spin_unlock(&gpstates->gpstate_lock); >> } >> >> /* >> @@ -596,7 +597,6 @@ static int powernv_cpufreq_target_index(struct cpufreq_policy *policy, >> { >> struct powernv_smp_call_data freq_data; >> unsigned int cur_msec, gpstate_id; >> - unsigned long flags; >> struct global_pstate_info *gpstates = policy->driver_data; >> >> if (unlikely(rebooting) && new_index != get_nominal_index()) >> @@ -607,7 +607,7 @@ static int powernv_cpufreq_target_index(struct cpufreq_policy *policy, >> >> cur_msec = jiffies_to_msecs(get_jiffies_64()); >> >> - spin_lock_irqsave(&gpstates->gpstate_lock, flags); >> + spin_lock(&gpstates->gpstate_lock); > You don't necessarily have to write 'what you are doing' in the commit log, but > tell us why you are doing that. > > Please explain, why is this changed and why will things continue to work > without this. > Thanks for reviewing. I have tried to convey that in the first line of commit message, "WARN_ON caused by smp_call_function_any() when irq is disabled, because of changes made in the patch" I see, i have not explained why i am changing irq safe spinlock to normal spinlock. will add some explanation. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c index 144c732..1f0e20c 100644 --- a/drivers/cpufreq/powernv-cpufreq.c +++ b/drivers/cpufreq/powernv-cpufreq.c @@ -581,9 +581,10 @@ void gpstate_timer_handler(unsigned long data) gpstates->last_gpstate = freq_data.gpstate_id; gpstates->last_lpstate = freq_data.pstate_id; + spin_unlock(&gpstates->gpstate_lock); + /* Timer may get migrated to a different cpu on cpu hot unplug */ smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1); - spin_unlock(&gpstates->gpstate_lock); } /* @@ -596,7 +597,6 @@ static int powernv_cpufreq_target_index(struct cpufreq_policy *policy, { struct powernv_smp_call_data freq_data; unsigned int cur_msec, gpstate_id; - unsigned long flags; struct global_pstate_info *gpstates = policy->driver_data; if (unlikely(rebooting) && new_index != get_nominal_index()) @@ -607,7 +607,7 @@ static int powernv_cpufreq_target_index(struct cpufreq_policy *policy, cur_msec = jiffies_to_msecs(get_jiffies_64()); - spin_lock_irqsave(&gpstates->gpstate_lock, flags); + spin_lock(&gpstates->gpstate_lock); freq_data.pstate_id = powernv_freqs[new_index].driver_data; if (!gpstates->last_sampled_time) { @@ -654,13 +654,14 @@ gpstates_done: gpstates->last_gpstate = freq_data.gpstate_id; gpstates->last_lpstate = freq_data.pstate_id; + spin_unlock(&gpstates->gpstate_lock); + /* * Use smp_call_function to send IPI and execute the * mtspr on target CPU. We could do that without IPI * if current CPU is within policy->cpus (core) */ smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1); - spin_unlock_irqrestore(&gpstates->gpstate_lock, flags); return 0; }
Fixing a WARN_ON caused by smp_call_function_any() when irq is disabled, because of changes made in the patch ('cpufreq: powernv: Ramp-down global pstate slower than local-pstate') https://patchwork.ozlabs.org/patch/612058/ WARNING: CPU: 0 PID: 4 at kernel/smp.c:291 smp_call_function_single+0x170/0x180 Call Trace: [c0000007f648f9f0] [c0000007f648fa90] 0xc0000007f648fa90 (unreliable) [c0000007f648fa30] [c0000000001430e0] smp_call_function_any+0x170/0x1c0 [c0000007f648fa90] [c0000000007b4b00] powernv_cpufreq_target_index+0xe0/0x250 [c0000007f648fb00] [c0000000007ac9dc] __cpufreq_driver_target+0x20c/0x3d0 [c0000007f648fbc0] [c0000000007b1b4c] od_dbs_timer+0xcc/0x260 [c0000007f648fc10] [c0000000007b3024] dbs_work_handler+0x54/0xa0 [c0000007f648fc50] [c0000000000c49a8] process_one_work+0x1d8/0x590 [c0000007f648fce0] [c0000000000c4e08] worker_thread+0xa8/0x660 [c0000007f648fd80] [c0000000000cca88] kthread+0x108/0x130 [c0000007f648fe30] [c0000000000095e8] ret_from_kernel_thread+0x5c/0x74 Moving smp_call_function_any() out of the critical section and changing irq safe spinlocks to normal spinlocks. Reported-by: Abdul Haleem <abdhalee@linux.vnet.linux.com> Signed-off-by: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> --- Patch is based on Rafael's linux-next drivers/cpufreq/powernv-cpufreq.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)