[PATCH] Thermal: Fix bug on generic thermal framework.

[PATCH] Thermal: Fix bug on generic thermal framework.

[Jonghwa Lee]

When system fails to bind cooling devices to thermal zone device during
registering thermal zone device, it leaves registering without canceling
delayed work. It probably makes panic if polling rate is not enough to release
that work from workqueue. So it is better to ignore initialization of polling
work to prevent that unexpected state.

Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
---
 drivers/thermal/thermal_sys.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 2ab31e4..744e69d 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -1351,12 +1351,14 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
 		}
 	mutex_unlock(&thermal_list_lock);
 
+	if (result)
+		goto unregister;
+
 	INIT_DELAYED_WORK(&(tz->poll_queue), thermal_zone_device_check);
 
 	thermal_zone_device_update(tz);
 
-	if (!result)
-		return tz;
+	return tz;
 
 unregister:
 	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
-- 
1.7.4.1

RE: [PATCH] Thermal: Fix bug on generic thermal framework.

[R, Durgadoss]

Hi,

Patch is fine, but I think you have to re-base on top of
Rui's -next branch here:
git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux.git

Also, adding Rui to this mail, not sure whether he is in LKML/pm.

Thanks,
Durga

> -----Original Message-----
> From: Jonghwa Lee [mailto:jonghwa3.lee@samsung.com]
> Sent: Monday, September 24, 2012 7:36 AM
> To: linux-pml@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; Brown, Len; Rafael J. Wysocki; Andrew
> Morton; Amit Kachhap; R, Durgadoss; Jonghwa Lee
> Subject: [PATCH] Thermal: Fix bug on generic thermal framework.
> 
> When system fails to bind cooling devices to thermal zone device during
> registering thermal zone device, it leaves registering without canceling
> delayed work. It probably makes panic if polling rate is not enough to release
> that work from workqueue. So it is better to ignore initialization of polling
> work to prevent that unexpected state.
> 
> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
> ---
>  drivers/thermal/thermal_sys.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index 2ab31e4..744e69d 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -1351,12 +1351,14 @@ struct thermal_zone_device
> *thermal_zone_device_register(const char *type,
>  		}
>  	mutex_unlock(&thermal_list_lock);
> 
> +	if (result)
> +		goto unregister;
> +
>  	INIT_DELAYED_WORK(&(tz->poll_queue),
> thermal_zone_device_check);
> 
>  	thermal_zone_device_update(tz);
> 
> -	if (!result)
> -		return tz;
> +	return tz;
> 
>  unregister:
>  	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
> --
> 1.7.4.1

Re: [PATCH] Thermal: Fix bug on generic thermal framework.

[jonghwa3.lee]

On 2012년 09월 24일 17:08, R, Durgadoss wrote:
> Hi,
>
> Patch is fine, but I think you have to re-base on top of
> Rui's -next branch here:
> git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux.git
>
> Also, adding Rui to this mail, not sure whether he is in LKML/pm.
>
> Thanks,
> Durga

I'll re-base and re-post it.
Thanks

>> -----Original Message-----
>> From: Jonghwa Lee [mailto:jonghwa3.lee@samsung.com]
>> Sent: Monday, September 24, 2012 7:36 AM
>> To: linux-pml@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org; Brown, Len; Rafael J. Wysocki; Andrew
>> Morton; Amit Kachhap; R, Durgadoss; Jonghwa Lee
>> Subject: [PATCH] Thermal: Fix bug on generic thermal framework.
>>
>> When system fails to bind cooling devices to thermal zone device during
>> registering thermal zone device, it leaves registering without canceling
>> delayed work. It probably makes panic if polling rate is not enough to release
>> that work from workqueue. So it is better to ignore initialization of polling
>> work to prevent that unexpected state.
>>
>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
>> ---
>>  drivers/thermal/thermal_sys.c |    6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
>> index 2ab31e4..744e69d 100644
>> --- a/drivers/thermal/thermal_sys.c
>> +++ b/drivers/thermal/thermal_sys.c
>> @@ -1351,12 +1351,14 @@ struct thermal_zone_device
>> *thermal_zone_device_register(const char *type,
>>  		}
>>  	mutex_unlock(&thermal_list_lock);
>>
>> +	if (result)
>> +		goto unregister;
>> +
>>  	INIT_DELAYED_WORK(&(tz->poll_queue),
>> thermal_zone_device_check);
>>
>>  	thermal_zone_device_update(tz);
>>
>> -	if (!result)
>> -		return tz;
>> +	return tz;
>>
>>  unregister:
>>  	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
>> --
>> 1.7.4.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

RE: [PATCH] Thermal: Fix bug on generic thermal framework.

[Zhang Rui]

On 一, 2012-09-24 at 02:08 -0600, R, Durgadoss wrote:
> Hi,
> 
> Patch is fine, but I think you have to re-base on top of
> Rui's -next branch here:
> git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux.git
> 
> Also, adding Rui to this mail, not sure whether he is in LKML/pm.
> 
> Thanks,
> Durga
> 
> > -----Original Message-----
> > From: Jonghwa Lee [mailto:jonghwa3.lee@samsung.com]
> > Sent: Monday, September 24, 2012 7:36 AM
> > To: linux-pml@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org; Brown, Len; Rafael J. Wysocki; Andrew
> > Morton; Amit Kachhap; R, Durgadoss; Jonghwa Lee
> > Subject: [PATCH] Thermal: Fix bug on generic thermal framework.
> > 
> > When system fails to bind cooling devices to thermal zone device during
> > registering thermal zone device, it leaves registering without canceling
> > delayed work. It probably makes panic if polling rate is not enough to release
> > that work from workqueue. So it is better to ignore initialization of polling
> > work to prevent that unexpected state.
> > 
Hi, Jonghwa,

I still do not understand what the problem is.
Say if a cooling device fails to bind, the thermal zone device would
still work properly, just like the failure cooling device is not
referenced in this thermal zone.

thanks,
rui
> > Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
> > ---
> >  drivers/thermal/thermal_sys.c |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> > index 2ab31e4..744e69d 100644
> > --- a/drivers/thermal/thermal_sys.c
> > +++ b/drivers/thermal/thermal_sys.c
> > @@ -1351,12 +1351,14 @@ struct thermal_zone_device
> > *thermal_zone_device_register(const char *type,
> >  		}
> >  	mutex_unlock(&thermal_list_lock);
> > 
> > +	if (result)
> > +		goto unregister;
> > +
> >  	INIT_DELAYED_WORK(&(tz->poll_queue),
> > thermal_zone_device_check);
> > 
> >  	thermal_zone_device_update(tz);
> > 
> > -	if (!result)
> > -		return tz;
> > +	return tz;
> > 
> >  unregister:
> >  	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
> > --
> > 1.7.4.1
> 

Re: [PATCH] Thermal: Fix bug on generic thermal framework.

[jonghwa3.lee]

On 2012년 09월 24일 17:57, Zhang Rui wrote:
> On 一, 2012-09-24 at 02:08 -0600, R, Durgadoss wrote:
>> Hi,
>>
>> Patch is fine, but I think you have to re-base on top of
>> Rui's -next branch here:
>> git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux.git
>>
>> Also, adding Rui to this mail, not sure whether he is in LKML/pm.
>>
>> Thanks,
>> Durga
>>
>>> -----Original Message-----
>>> From: Jonghwa Lee [mailto:jonghwa3.lee@samsung.com]
>>> Sent: Monday, September 24, 2012 7:36 AM
>>> To: linux-pml@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org; Brown, Len; Rafael J. Wysocki; Andrew
>>> Morton; Amit Kachhap; R, Durgadoss; Jonghwa Lee
>>> Subject: [PATCH] Thermal: Fix bug on generic thermal framework.
>>>
>>> When system fails to bind cooling devices to thermal zone device during
>>> registering thermal zone device, it leaves registering without canceling
>>> delayed work. It probably makes panic if polling rate is not enough to release
>>> that work from workqueue. So it is better to ignore initialization of polling
>>> work to prevent that unexpected state.
>>>
> Hi, Jonghwa,
>
> I still do not understand what the problem is.
> Say if a cooling device fails to bind, the thermal zone device would
> still work properly, just like the failure cooling device is not
> referenced in this thermal zone.
>
> thanks,
> rui
Hi rui,
No, it doesn't work properly. If it fails to bind some cool dev to
thermal zone device, it frees thermal zone
device without canceling delayed work. After freeing thermal zone
device, system may call work function
pointed NULL as the timer expired. Thus it requires skipping the
initialization of polling work or canceling before
the unregister.

Thanks,
Jonghwa

>>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
>>> ---
>>>  drivers/thermal/thermal_sys.c |    6 ++++--
>>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
>>> index 2ab31e4..744e69d 100644
>>> --- a/drivers/thermal/thermal_sys.c
>>> +++ b/drivers/thermal/thermal_sys.c
>>> @@ -1351,12 +1351,14 @@ struct thermal_zone_device
>>> *thermal_zone_device_register(const char *type,
>>>  		}
>>>  	mutex_unlock(&thermal_list_lock);
>>>
>>> +	if (result)
>>> +		goto unregister;
>>> +
>>>  	INIT_DELAYED_WORK(&(tz->poll_queue),
>>> thermal_zone_device_check);
>>>
>>>  	thermal_zone_device_update(tz);
>>>
>>> -	if (!result)
>>> -		return tz;
>>> +	return tz;
>>>
>>>  unregister:
>>>  	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
>>> --
>>> 1.7.4.1
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH] Thermal: Fix bug on generic thermal framework.

[Zhang Rui]

On 二, 2012-09-25 at 10:12 +0900, jonghwa3.lee@samsung.com wrote:
> On 2012년 09월 24일 17:57, Zhang Rui wrote:
> > On 一, 2012-09-24 at 02:08 -0600, R, Durgadoss wrote:
> >> Hi,
> >>
> >> Patch is fine, but I think you have to re-base on top of
> >> Rui's -next branch here:
> >> git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux.git
> >>
> >> Also, adding Rui to this mail, not sure whether he is in LKML/pm.
> >>
> >> Thanks,
> >> Durga
> >>
> >>> -----Original Message-----
> >>> From: Jonghwa Lee [mailto:jonghwa3.lee@samsung.com]
> >>> Sent: Monday, September 24, 2012 7:36 AM
> >>> To: linux-pml@vger.kernel.org
> >>> Cc: linux-kernel@vger.kernel.org; Brown, Len; Rafael J. Wysocki; Andrew
> >>> Morton; Amit Kachhap; R, Durgadoss; Jonghwa Lee
> >>> Subject: [PATCH] Thermal: Fix bug on generic thermal framework.
> >>>
> >>> When system fails to bind cooling devices to thermal zone device during
> >>> registering thermal zone device, it leaves registering without canceling
> >>> delayed work. It probably makes panic if polling rate is not enough to release
> >>> that work from workqueue. So it is better to ignore initialization of polling
> >>> work to prevent that unexpected state.
> >>>
> > Hi, Jonghwa,
> >
> > I still do not understand what the problem is.
> > Say if a cooling device fails to bind, the thermal zone device would
> > still work properly, just like the failure cooling device is not
> > referenced in this thermal zone.
> >
> > thanks,
> > rui
> Hi rui,
> No, it doesn't work properly. If it fails to bind some cool dev to
> thermal zone device, it frees thermal zone
> device without canceling delayed work. After freeing thermal zone
> device, system may call work function
> pointed NULL as the timer expired. Thus it requires skipping the
> initialization of polling work or canceling before
> the unregister.


hah, I see what the problem is.
ideally, if we fail to bind one cooling device, we should just ignore it
and continue to bind other, what do you think?

does the patch below fix your problem?
If yes, I'll try to rebase it on top of my next tree.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/thermal/thermal_sys.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 2ab31e4..c5e2c28 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -1343,20 +1343,17 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
 
 	mutex_lock(&thermal_list_lock);
 	list_add_tail(&tz->node, &thermal_tz_list);
-	if (ops->bind)
-		list_for_each_entry(pos, &thermal_cdev_list, node) {
-		result = ops->bind(tz, pos);
-		if (result)
-			break;
-		}
+	if (ops->bind) {
+		list_for_each_entry(pos, &thermal_cdev_list, node)
+			ops->bind(tz, pos);
+	}
 	mutex_unlock(&thermal_list_lock);
 
 	INIT_DELAYED_WORK(&(tz->poll_queue), thermal_zone_device_check);
 
 	thermal_zone_device_update(tz);
 
-	if (!result)
-		return tz;
+	return tz;
 
 unregister:
 	release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
-- 
1.7.7.6

RE: [PATCH] Thermal: Fix bug on generic thermal framework.

[R, Durgadoss]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1319 bytes --]

Hi Rui,

> > > Hi, Jonghwa,
> > >
> > > I still do not understand what the problem is.
> > > Say if a cooling device fails to bind, the thermal zone device would
> > > still work properly, just like the failure cooling device is not
> > > referenced in this thermal zone.
> > >
> > > thanks,
> > > rui
> > Hi rui,
> > No, it doesn't work properly. If it fails to bind some cool dev to
> > thermal zone device, it frees thermal zone
> > device without canceling delayed work. After freeing thermal zone
> > device, system may call work function
> > pointed NULL as the timer expired. Thus it requires skipping the
> > initialization of polling work or canceling before
> > the unregister.
> 
> 
> hah, I see what the problem is.
> ideally, if we fail to bind one cooling device, we should just ignore it
> and continue to bind other, what do you think?

Yes, this is what we should do.

> 
> does the patch below fix your problem?
> If yes, I'll try to rebase it on top of my next tree.

This is already fixed in your -next tree, since you applied the
fair share patches 10/15. The function bind_tz(tz) does the
exact same thing, and continues.

Thanks,
Durga
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] Thermal: Fix bug on generic thermal framework.

[jonghwa3.lee]

On 2012년 09월 25일 12:04, R, Durgadoss wrote:
> Hi Rui,
>
>>>> Hi, Jonghwa,
>>>>
>>>> I still do not understand what the problem is.
>>>> Say if a cooling device fails to bind, the thermal zone device would
>>>> still work properly, just like the failure cooling device is not
>>>> referenced in this thermal zone.
>>>>
>>>> thanks,
>>>> rui
>>> Hi rui,
>>> No, it doesn't work properly. If it fails to bind some cool dev to
>>> thermal zone device, it frees thermal zone
>>> device without canceling delayed work. After freeing thermal zone
>>> device, system may call work function
>>> pointed NULL as the timer expired. Thus it requires skipping the
>>> initialization of polling work or canceling before
>>> the unregister.
>>
>> hah, I see what the problem is.
>> ideally, if we fail to bind one cooling device, we should just ignore it
>> and continue to bind other, what do you think?
> Yes, this is what we should do.
>
>> does the patch below fix your problem?
>> If yes, I'll try to rebase it on top of my next tree.
> This is already fixed in your -next tree, since you applied the
> fair share patches 10/15. The function bind_tz(tz) does the
> exact same thing, and continues.
>
> Thanks,
> Durga
I checked that it had been applied on -next branch, I don't commit this
patch any more.

Thanks,
Jonghwa

> N�����r��y���b�X��ǧv�^�)޺{.n�+����{��h��\x17��ܨ}���Ơz�&j:+v���\a����zZ+��+zf���h���~����i���z�\x1e�w���?����&�)ߢ^[fl===