* [PATCH] Thermal: change spin lock
@ 2013-09-12 22:05 Srinivas Pandruvada
2013-09-24 16:32 ` Srinivas Pandruvada
2013-09-24 17:25 ` Eduardo Valentin
0 siblings, 2 replies; 5+ messages in thread
From: Srinivas Pandruvada @ 2013-09-12 22:05 UTC (permalink / raw)
To: linux-pm; +Cc: rui.zhang, Srinivas Pandruvada
Change spin_lock to spin_lock_irqsave and spin_unlock to
spin_unlock_irqrestore as the data it is trying to protect can also
be modified in a notification call called from interrupt handler.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
drivers/thermal/x86_pkg_temp_thermal.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/thermal/x86_pkg_temp_thermal.c b/drivers/thermal/x86_pkg_temp_thermal.c
index f36950e..7722cb9 100644
--- a/drivers/thermal/x86_pkg_temp_thermal.c
+++ b/drivers/thermal/x86_pkg_temp_thermal.c
@@ -316,18 +316,19 @@ static void pkg_temp_thermal_threshold_work_fn(struct work_struct *work)
int phy_id = topology_physical_package_id(cpu);
struct phy_dev_entry *phdev = pkg_temp_thermal_get_phy_entry(cpu);
bool notify = false;
+ unsigned long flags;
if (!phdev)
return;
- spin_lock(&pkg_work_lock);
+ spin_lock_irqsave(&pkg_work_lock, flags);
++pkg_work_cnt;
if (unlikely(phy_id > max_phy_id)) {
- spin_unlock(&pkg_work_lock);
+ spin_unlock_irqrestore(&pkg_work_lock, flags);
return;
}
pkg_work_scheduled[phy_id] = 0;
- spin_unlock(&pkg_work_lock);
+ spin_unlock_irqrestore(&pkg_work_lock, flags);
enable_pkg_thres_interrupt();
rdmsrl(MSR_IA32_PACKAGE_THERM_STATUS, msr_val);
@@ -397,6 +398,7 @@ static int pkg_temp_thermal_device_add(unsigned int cpu)
int thres_count;
u32 eax, ebx, ecx, edx;
u8 *temp;
+ unsigned long flags;
cpuid(6, &eax, &ebx, &ecx, &edx);
thres_count = ebx & 0x07;
@@ -420,19 +422,19 @@ static int pkg_temp_thermal_device_add(unsigned int cpu)
goto err_ret_unlock;
}
- spin_lock(&pkg_work_lock);
+ spin_lock_irqsave(&pkg_work_lock, flags);
if (topology_physical_package_id(cpu) > max_phy_id)
max_phy_id = topology_physical_package_id(cpu);
temp = krealloc(pkg_work_scheduled,
(max_phy_id+1) * sizeof(u8), GFP_ATOMIC);
if (!temp) {
- spin_unlock(&pkg_work_lock);
+ spin_unlock_irqrestore(&pkg_work_lock, flags);
err = -ENOMEM;
goto err_ret_free;
}
pkg_work_scheduled = temp;
pkg_work_scheduled[topology_physical_package_id(cpu)] = 0;
- spin_unlock(&pkg_work_lock);
+ spin_unlock_irqrestore(&pkg_work_lock, flags);
phy_dev_entry->phys_proc_id = topology_physical_package_id(cpu);
phy_dev_entry->first_cpu = cpu;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] Thermal: change spin lock
2013-09-12 22:05 [PATCH] Thermal: change spin lock Srinivas Pandruvada
@ 2013-09-24 16:32 ` Srinivas Pandruvada
2013-09-24 17:14 ` Eduardo Valentin
2013-09-24 17:25 ` Eduardo Valentin
1 sibling, 1 reply; 5+ messages in thread
From: Srinivas Pandruvada @ 2013-09-24 16:32 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: linux-pm, rui.zhang, Eduardo Valentin
How long it take for thermal maintainers to look at a patch and comment?
Already 12 days.
Thanks,
Srinivas
On 09/12/2013 03:05 PM, Srinivas Pandruvada wrote:
> Change spin_lock to spin_lock_irqsave and spin_unlock to
> spin_unlock_irqrestore as the data it is trying to protect can also
> be modified in a notification call called from interrupt handler.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> drivers/thermal/x86_pkg_temp_thermal.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/thermal/x86_pkg_temp_thermal.c b/drivers/thermal/x86_pkg_temp_thermal.c
> index f36950e..7722cb9 100644
> --- a/drivers/thermal/x86_pkg_temp_thermal.c
> +++ b/drivers/thermal/x86_pkg_temp_thermal.c
> @@ -316,18 +316,19 @@ static void pkg_temp_thermal_threshold_work_fn(struct work_struct *work)
> int phy_id = topology_physical_package_id(cpu);
> struct phy_dev_entry *phdev = pkg_temp_thermal_get_phy_entry(cpu);
> bool notify = false;
> + unsigned long flags;
>
> if (!phdev)
> return;
>
> - spin_lock(&pkg_work_lock);
> + spin_lock_irqsave(&pkg_work_lock, flags);
> ++pkg_work_cnt;
> if (unlikely(phy_id > max_phy_id)) {
> - spin_unlock(&pkg_work_lock);
> + spin_unlock_irqrestore(&pkg_work_lock, flags);
> return;
> }
> pkg_work_scheduled[phy_id] = 0;
> - spin_unlock(&pkg_work_lock);
> + spin_unlock_irqrestore(&pkg_work_lock, flags);
>
> enable_pkg_thres_interrupt();
> rdmsrl(MSR_IA32_PACKAGE_THERM_STATUS, msr_val);
> @@ -397,6 +398,7 @@ static int pkg_temp_thermal_device_add(unsigned int cpu)
> int thres_count;
> u32 eax, ebx, ecx, edx;
> u8 *temp;
> + unsigned long flags;
>
> cpuid(6, &eax, &ebx, &ecx, &edx);
> thres_count = ebx & 0x07;
> @@ -420,19 +422,19 @@ static int pkg_temp_thermal_device_add(unsigned int cpu)
> goto err_ret_unlock;
> }
>
> - spin_lock(&pkg_work_lock);
> + spin_lock_irqsave(&pkg_work_lock, flags);
> if (topology_physical_package_id(cpu) > max_phy_id)
> max_phy_id = topology_physical_package_id(cpu);
> temp = krealloc(pkg_work_scheduled,
> (max_phy_id+1) * sizeof(u8), GFP_ATOMIC);
> if (!temp) {
> - spin_unlock(&pkg_work_lock);
> + spin_unlock_irqrestore(&pkg_work_lock, flags);
> err = -ENOMEM;
> goto err_ret_free;
> }
> pkg_work_scheduled = temp;
> pkg_work_scheduled[topology_physical_package_id(cpu)] = 0;
> - spin_unlock(&pkg_work_lock);
> + spin_unlock_irqrestore(&pkg_work_lock, flags);
>
> phy_dev_entry->phys_proc_id = topology_physical_package_id(cpu);
> phy_dev_entry->first_cpu = cpu;
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Thermal: change spin lock
2013-09-24 16:32 ` Srinivas Pandruvada
@ 2013-09-24 17:14 ` Eduardo Valentin
2013-09-24 18:34 ` Srinivas Pandruvada
0 siblings, 1 reply; 5+ messages in thread
From: Eduardo Valentin @ 2013-09-24 17:14 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: linux-pm, rui.zhang, Eduardo Valentin
[-- Attachment #1: Type: text/plain, Size: 3643 bytes --]
On 24-09-2013 12:32, Srinivas Pandruvada wrote:
> How long it take for thermal maintainers to look at a patch and comment?
> Already 12 days.
Well, I dont think there is a hard deadline for this. This specific
period is still within 2 weeks time frame, which is pretty reasonable time.
Also, adding maintainers in your To: list call their attention to your
patch.
Besides, I don't think throwing such kind of comment would help to speed
the process.
>
> Thanks,
> Srinivas
>
> On 09/12/2013 03:05 PM, Srinivas Pandruvada wrote:
>> Change spin_lock to spin_lock_irqsave and spin_unlock to
>> spin_unlock_irqrestore as the data it is trying to protect can also
>> be modified in a notification call called from interrupt handler.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>> ---
>> drivers/thermal/x86_pkg_temp_thermal.c | 14 ++++++++------
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/thermal/x86_pkg_temp_thermal.c
>> b/drivers/thermal/x86_pkg_temp_thermal.c
>> index f36950e..7722cb9 100644
>> --- a/drivers/thermal/x86_pkg_temp_thermal.c
>> +++ b/drivers/thermal/x86_pkg_temp_thermal.c
>> @@ -316,18 +316,19 @@ static void
>> pkg_temp_thermal_threshold_work_fn(struct work_struct *work)
>> int phy_id = topology_physical_package_id(cpu);
>> struct phy_dev_entry *phdev = pkg_temp_thermal_get_phy_entry(cpu);
>> bool notify = false;
>> + unsigned long flags;
>> if (!phdev)
>> return;
>> - spin_lock(&pkg_work_lock);
>> + spin_lock_irqsave(&pkg_work_lock, flags);
>> ++pkg_work_cnt;
>> if (unlikely(phy_id > max_phy_id)) {
>> - spin_unlock(&pkg_work_lock);
>> + spin_unlock_irqrestore(&pkg_work_lock, flags);
>> return;
>> }
>> pkg_work_scheduled[phy_id] = 0;
>> - spin_unlock(&pkg_work_lock);
>> + spin_unlock_irqrestore(&pkg_work_lock, flags);
>> enable_pkg_thres_interrupt();
>> rdmsrl(MSR_IA32_PACKAGE_THERM_STATUS, msr_val);
>> @@ -397,6 +398,7 @@ static int pkg_temp_thermal_device_add(unsigned
>> int cpu)
>> int thres_count;
>> u32 eax, ebx, ecx, edx;
>> u8 *temp;
>> + unsigned long flags;
>> cpuid(6, &eax, &ebx, &ecx, &edx);
>> thres_count = ebx & 0x07;
>> @@ -420,19 +422,19 @@ static int pkg_temp_thermal_device_add(unsigned
>> int cpu)
>> goto err_ret_unlock;
>> }
>> - spin_lock(&pkg_work_lock);
>> + spin_lock_irqsave(&pkg_work_lock, flags);
>> if (topology_physical_package_id(cpu) > max_phy_id)
>> max_phy_id = topology_physical_package_id(cpu);
>> temp = krealloc(pkg_work_scheduled,
>> (max_phy_id+1) * sizeof(u8), GFP_ATOMIC);
>> if (!temp) {
>> - spin_unlock(&pkg_work_lock);
>> + spin_unlock_irqrestore(&pkg_work_lock, flags);
>> err = -ENOMEM;
>> goto err_ret_free;
>> }
>> pkg_work_scheduled = temp;
>> pkg_work_scheduled[topology_physical_package_id(cpu)] = 0;
>> - spin_unlock(&pkg_work_lock);
>> + spin_unlock_irqrestore(&pkg_work_lock, flags);
>> phy_dev_entry->phys_proc_id = topology_physical_package_id(cpu);
>> phy_dev_entry->first_cpu = cpu;
>
> --
> 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
>
>
--
You have got to be excited about what you are doing. (L. Lamport)
Eduardo Valentin
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Thermal: change spin lock
2013-09-24 17:14 ` Eduardo Valentin
@ 2013-09-24 18:34 ` Srinivas Pandruvada
0 siblings, 0 replies; 5+ messages in thread
From: Srinivas Pandruvada @ 2013-09-24 18:34 UTC (permalink / raw)
To: Eduardo Valentin; +Cc: linux-pm, rui.zhang
On 09/24/2013 10:14 AM, Eduardo Valentin wrote:
> On 24-09-2013 12:32, Srinivas Pandruvada wrote:
>> How long it take for thermal maintainers to look at a patch and comment?
>> Already 12 days.
> Well, I dont think there is a hard deadline for this. This specific
> period is still within 2 weeks time frame, which is pretty reasonable time.
>
> Also, adding maintainers in your To: list call their attention to your
> patch.
>
> Besides, I don't think throwing such kind of comment would help to speed
> the process.
Thanks for the review. Updated and sent again.
>> Thanks,
>> Srinivas
>>
>> On 09/12/2013 03:05 PM, Srinivas Pandruvada wrote:
>>> Change spin_lock to spin_lock_irqsave and spin_unlock to
>>> spin_unlock_irqrestore as the data it is trying to protect can also
>>> be modified in a notification call called from interrupt handler.
>>>
>>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>>> ---
>>> drivers/thermal/x86_pkg_temp_thermal.c | 14 ++++++++------
>>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/thermal/x86_pkg_temp_thermal.c
>>> b/drivers/thermal/x86_pkg_temp_thermal.c
>>> index f36950e..7722cb9 100644
>>> --- a/drivers/thermal/x86_pkg_temp_thermal.c
>>> +++ b/drivers/thermal/x86_pkg_temp_thermal.c
>>> @@ -316,18 +316,19 @@ static void
>>> pkg_temp_thermal_threshold_work_fn(struct work_struct *work)
>>> int phy_id = topology_physical_package_id(cpu);
>>> struct phy_dev_entry *phdev = pkg_temp_thermal_get_phy_entry(cpu);
>>> bool notify = false;
>>> + unsigned long flags;
>>> if (!phdev)
>>> return;
>>> - spin_lock(&pkg_work_lock);
>>> + spin_lock_irqsave(&pkg_work_lock, flags);
>>> ++pkg_work_cnt;
>>> if (unlikely(phy_id > max_phy_id)) {
>>> - spin_unlock(&pkg_work_lock);
>>> + spin_unlock_irqrestore(&pkg_work_lock, flags);
>>> return;
>>> }
>>> pkg_work_scheduled[phy_id] = 0;
>>> - spin_unlock(&pkg_work_lock);
>>> + spin_unlock_irqrestore(&pkg_work_lock, flags);
>>> enable_pkg_thres_interrupt();
>>> rdmsrl(MSR_IA32_PACKAGE_THERM_STATUS, msr_val);
>>> @@ -397,6 +398,7 @@ static int pkg_temp_thermal_device_add(unsigned
>>> int cpu)
>>> int thres_count;
>>> u32 eax, ebx, ecx, edx;
>>> u8 *temp;
>>> + unsigned long flags;
>>> cpuid(6, &eax, &ebx, &ecx, &edx);
>>> thres_count = ebx & 0x07;
>>> @@ -420,19 +422,19 @@ static int pkg_temp_thermal_device_add(unsigned
>>> int cpu)
>>> goto err_ret_unlock;
>>> }
>>> - spin_lock(&pkg_work_lock);
>>> + spin_lock_irqsave(&pkg_work_lock, flags);
>>> if (topology_physical_package_id(cpu) > max_phy_id)
>>> max_phy_id = topology_physical_package_id(cpu);
>>> temp = krealloc(pkg_work_scheduled,
>>> (max_phy_id+1) * sizeof(u8), GFP_ATOMIC);
>>> if (!temp) {
>>> - spin_unlock(&pkg_work_lock);
>>> + spin_unlock_irqrestore(&pkg_work_lock, flags);
>>> err = -ENOMEM;
>>> goto err_ret_free;
>>> }
>>> pkg_work_scheduled = temp;
>>> pkg_work_scheduled[topology_physical_package_id(cpu)] = 0;
>>> - spin_unlock(&pkg_work_lock);
>>> + spin_unlock_irqrestore(&pkg_work_lock, flags);
>>> phy_dev_entry->phys_proc_id = topology_physical_package_id(cpu);
>>> phy_dev_entry->first_cpu = cpu;
>> --
>> 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
>>
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Thermal: change spin lock
2013-09-12 22:05 [PATCH] Thermal: change spin lock Srinivas Pandruvada
2013-09-24 16:32 ` Srinivas Pandruvada
@ 2013-09-24 17:25 ` Eduardo Valentin
1 sibling, 0 replies; 5+ messages in thread
From: Eduardo Valentin @ 2013-09-24 17:25 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: linux-pm, rui.zhang, eduardo.valentin
[-- Attachment #1: Type: text/plain, Size: 3249 bytes --]
Please change your patch subject. It suggests you are changing the
locking mechanism in the entire thermal subsystem, which is not true.
On 12-09-2013 18:05, Srinivas Pandruvada wrote:
> Change spin_lock to spin_lock_irqsave and spin_unlock to
> spin_unlock_irqrestore as the data it is trying to protect can also
> be modified in a notification call called from interrupt handler.
ditto for your patch description.
While you are there, looks like your lock needs a better documentation.
I would suggest you to add a description of what data it protects and
not leaving only documentation of where it it used.
Also, in your code it is not clear what is interrupt context and what is
not interrupt context. Now that you have found that you have the need to
use this lock in both context, please document it.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> drivers/thermal/x86_pkg_temp_thermal.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/thermal/x86_pkg_temp_thermal.c b/drivers/thermal/x86_pkg_temp_thermal.c
> index f36950e..7722cb9 100644
> --- a/drivers/thermal/x86_pkg_temp_thermal.c
> +++ b/drivers/thermal/x86_pkg_temp_thermal.c
> @@ -316,18 +316,19 @@ static void pkg_temp_thermal_threshold_work_fn(struct work_struct *work)
> int phy_id = topology_physical_package_id(cpu);
> struct phy_dev_entry *phdev = pkg_temp_thermal_get_phy_entry(cpu);
> bool notify = false;
> + unsigned long flags;
>
> if (!phdev)
> return;
>
> - spin_lock(&pkg_work_lock);
> + spin_lock_irqsave(&pkg_work_lock, flags);
> ++pkg_work_cnt;
> if (unlikely(phy_id > max_phy_id)) {
> - spin_unlock(&pkg_work_lock);
> + spin_unlock_irqrestore(&pkg_work_lock, flags);
> return;
> }
> pkg_work_scheduled[phy_id] = 0;
> - spin_unlock(&pkg_work_lock);
> + spin_unlock_irqrestore(&pkg_work_lock, flags);
>
> enable_pkg_thres_interrupt();
> rdmsrl(MSR_IA32_PACKAGE_THERM_STATUS, msr_val);
> @@ -397,6 +398,7 @@ static int pkg_temp_thermal_device_add(unsigned int cpu)
> int thres_count;
> u32 eax, ebx, ecx, edx;
> u8 *temp;
> + unsigned long flags;
>
> cpuid(6, &eax, &ebx, &ecx, &edx);
> thres_count = ebx & 0x07;
> @@ -420,19 +422,19 @@ static int pkg_temp_thermal_device_add(unsigned int cpu)
> goto err_ret_unlock;
> }
>
> - spin_lock(&pkg_work_lock);
> + spin_lock_irqsave(&pkg_work_lock, flags);
> if (topology_physical_package_id(cpu) > max_phy_id)
> max_phy_id = topology_physical_package_id(cpu);
> temp = krealloc(pkg_work_scheduled,
> (max_phy_id+1) * sizeof(u8), GFP_ATOMIC);
> if (!temp) {
> - spin_unlock(&pkg_work_lock);
> + spin_unlock_irqrestore(&pkg_work_lock, flags);
> err = -ENOMEM;
> goto err_ret_free;
> }
> pkg_work_scheduled = temp;
> pkg_work_scheduled[topology_physical_package_id(cpu)] = 0;
> - spin_unlock(&pkg_work_lock);
> + spin_unlock_irqrestore(&pkg_work_lock, flags);
>
> phy_dev_entry->phys_proc_id = topology_physical_package_id(cpu);
> phy_dev_entry->first_cpu = cpu;
>
--
You have got to be excited about what you are doing. (L. Lamport)
Eduardo Valentin
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-09-24 18:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-12 22:05 [PATCH] Thermal: change spin lock Srinivas Pandruvada
2013-09-24 16:32 ` Srinivas Pandruvada
2013-09-24 17:14 ` Eduardo Valentin
2013-09-24 18:34 ` Srinivas Pandruvada
2013-09-24 17:25 ` Eduardo Valentin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).