diff mbox

thermal: exynos: Disable the regulator on probe failure

Message ID 1433727349-23330-1-git-send-email-k.kozlowski@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Krzysztof Kozlowski June 8, 2015, 1:35 a.m. UTC
During probe the regulator (if present) was enabled but not disabled in
case of failure. So an unsuccessful probe lead to enabling the
regulator which was actually not needed because the device was not
enabled.

Additionally each deferred probe lead to increase of regulator enable
count so it would not be effectively disabled during removal of the
device.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Fixes: 498d22f616f6 ("thermal: exynos: Support for TMU regulator defined at device tree")
Cc: <stable@vger.kernel.org>

---

I am not entirely convinced that this should go to stable. Leaving a
regulator enabled in case of probe failure (no exynos TMU device) or
after deferred probe (regulator won't be disabled during device removal)
is not a critical issue, just leaks power.
---
 drivers/thermal/samsung/exynos_tmu.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Javier Martinez Canillas June 8, 2015, 6:54 a.m. UTC | #1
Hello Krzysztof,

On Mon, Jun 8, 2015 at 3:35 AM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> During probe the regulator (if present) was enabled but not disabled in
> case of failure. So an unsuccessful probe lead to enabling the
> regulator which was actually not needed because the device was not
> enabled.
>
> Additionally each deferred probe lead to increase of regulator enable
> count so it would not be effectively disabled during removal of the
> device.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Fixes: 498d22f616f6 ("thermal: exynos: Support for TMU regulator defined at device tree")
> Cc: <stable@vger.kernel.org>
>

Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

> ---
>
> I am not entirely convinced that this should go to stable. Leaving a
> regulator enabled in case of probe failure (no exynos TMU device) or
> after deferred probe (regulator won't be disabled during device removal)
> is not a critical issue, just leaks power.


Yes, as you said leaving the regulator enabled is not critical but
OTOH is a very small patch and is fixing a very evident bug so I think
it's OK.

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukasz Majewski June 8, 2015, 4:14 p.m. UTC | #2
Hi Krzysztof,

> During probe the regulator (if present) was enabled but not disabled
> in case of failure. So an unsuccessful probe lead to enabling the
> regulator which was actually not needed because the device was not
> enabled.
> 
> Additionally each deferred probe lead to increase of regulator enable
> count so it would not be effectively disabled during removal of the
> device.

Thanks for catching this.

> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Fixes: 498d22f616f6 ("thermal: exynos: Support for TMU regulator
> defined at device tree") Cc: <stable@vger.kernel.org>
> 
> ---
> 
> I am not entirely convinced that this should go to stable. Leaving a
> regulator enabled in case of probe failure (no exynos TMU device) or
> after deferred probe (regulator won't be disabled during device
> removal) is not a critical issue, just leaks power.
> ---
>  drivers/thermal/samsung/exynos_tmu.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/thermal/samsung/exynos_tmu.c
> b/drivers/thermal/samsung/exynos_tmu.c index
> 531f4b179871..13c3aceed19d 100644 ---
> a/drivers/thermal/samsung/exynos_tmu.c +++
> b/drivers/thermal/samsung/exynos_tmu.c @@ -1392,6 +1392,8 @@
> err_clk_sec: if (!IS_ERR(data->clk_sec))
>  		clk_unprepare(data->clk_sec);
>  err_sensor:
> +	if (!IS_ERR_OR_NULL(data->regulator))
> +		regulator_disable(data->regulator);
>  	thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd);
>  
>  	return ret;

Acked-by: Lukasz Majewski <l.majewski@samsung.com>

I will test it and afterwards add to samsung-thermal tree.
Krzysztof Kozlowski July 6, 2015, 4 a.m. UTC | #3
2015-06-09 1:14 GMT+09:00 Lukasz Majewski <l.majewski@samsung.com>:
> Hi Krzysztof,
>
>> During probe the regulator (if present) was enabled but not disabled
>> in case of failure. So an unsuccessful probe lead to enabling the
>> regulator which was actually not needed because the device was not
>> enabled.
>>
>> Additionally each deferred probe lead to increase of regulator enable
>> count so it would not be effectively disabled during removal of the
>> device.
>
> Thanks for catching this.
>
>>
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> Fixes: 498d22f616f6 ("thermal: exynos: Support for TMU regulator
>> defined at device tree") Cc: <stable@vger.kernel.org>
>>
>> ---
>>
>> I am not entirely convinced that this should go to stable. Leaving a
>> regulator enabled in case of probe failure (no exynos TMU device) or
>> after deferred probe (regulator won't be disabled during device
>> removal) is not a critical issue, just leaks power.
>> ---
>>  drivers/thermal/samsung/exynos_tmu.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/thermal/samsung/exynos_tmu.c
>> b/drivers/thermal/samsung/exynos_tmu.c index
>> 531f4b179871..13c3aceed19d 100644 ---
>> a/drivers/thermal/samsung/exynos_tmu.c +++
>> b/drivers/thermal/samsung/exynos_tmu.c @@ -1392,6 +1392,8 @@
>> err_clk_sec: if (!IS_ERR(data->clk_sec))
>>               clk_unprepare(data->clk_sec);
>>  err_sensor:
>> +     if (!IS_ERR_OR_NULL(data->regulator))
>> +             regulator_disable(data->regulator);
>>       thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd);
>>
>>       return ret;
>
> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
>
> I will test it and afterwards add to samsung-thermal tree.

Hi ?ukasz,

I can't find this patch in v4.2-rc1 or your tree. What happened?

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukasz Majewski July 6, 2015, 7:01 a.m. UTC | #4
Hi Krzysztof,

> 2015-06-09 1:14 GMT+09:00 Lukasz Majewski <l.majewski@samsung.com>:
> > Hi Krzysztof,
> >
> >> During probe the regulator (if present) was enabled but not
> >> disabled in case of failure. So an unsuccessful probe lead to
> >> enabling the regulator which was actually not needed because the
> >> device was not enabled.
> >>
> >> Additionally each deferred probe lead to increase of regulator
> >> enable count so it would not be effectively disabled during
> >> removal of the device.
> >
> > Thanks for catching this.
> >
> >>
> >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> >> Fixes: 498d22f616f6 ("thermal: exynos: Support for TMU regulator
> >> defined at device tree") Cc: <stable@vger.kernel.org>
> >>
> >> ---
> >>
> >> I am not entirely convinced that this should go to stable. Leaving
> >> a regulator enabled in case of probe failure (no exynos TMU
> >> device) or after deferred probe (regulator won't be disabled
> >> during device removal) is not a critical issue, just leaks power.
> >> ---
> >>  drivers/thermal/samsung/exynos_tmu.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/thermal/samsung/exynos_tmu.c
> >> b/drivers/thermal/samsung/exynos_tmu.c index
> >> 531f4b179871..13c3aceed19d 100644 ---
> >> a/drivers/thermal/samsung/exynos_tmu.c +++
> >> b/drivers/thermal/samsung/exynos_tmu.c @@ -1392,6 +1392,8 @@
> >> err_clk_sec: if (!IS_ERR(data->clk_sec))
> >>               clk_unprepare(data->clk_sec);
> >>  err_sensor:
> >> +     if (!IS_ERR_OR_NULL(data->regulator))
> >> +             regulator_disable(data->regulator);
> >>       thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd);
> >>
> >>       return ret;
> >
> > Acked-by: Lukasz Majewski <l.majewski@samsung.com>
> >
> > I will test it and afterwards add to samsung-thermal tree.
> 
> Hi ?ukasz,
> 
> I can't find this patch in v4.2-rc1 or your tree. What happened?

I will got together with Chanowoo patches. I will send PR today to
Eduardo.

> 
> Best regards,
> Krzysztof
Krzysztof Kozlowski July 6, 2015, 7:02 a.m. UTC | #5
On 06.07.2015 16:01, Lukasz Majewski wrote:
> Hi Krzysztof,
> 
>> 2015-06-09 1:14 GMT+09:00 Lukasz Majewski <l.majewski@samsung.com>:
>>> Hi Krzysztof,
>>>
>>>> During probe the regulator (if present) was enabled but not
>>>> disabled in case of failure. So an unsuccessful probe lead to
>>>> enabling the regulator which was actually not needed because the
>>>> device was not enabled.
>>>>
>>>> Additionally each deferred probe lead to increase of regulator
>>>> enable count so it would not be effectively disabled during
>>>> removal of the device.
>>>
>>> Thanks for catching this.
>>>
>>>>
>>>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>>> Fixes: 498d22f616f6 ("thermal: exynos: Support for TMU regulator
>>>> defined at device tree") Cc: <stable@vger.kernel.org>
>>>>
>>>> ---
>>>>
>>>> I am not entirely convinced that this should go to stable. Leaving
>>>> a regulator enabled in case of probe failure (no exynos TMU
>>>> device) or after deferred probe (regulator won't be disabled
>>>> during device removal) is not a critical issue, just leaks power.
>>>> ---
>>>>  drivers/thermal/samsung/exynos_tmu.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c
>>>> b/drivers/thermal/samsung/exynos_tmu.c index
>>>> 531f4b179871..13c3aceed19d 100644 ---
>>>> a/drivers/thermal/samsung/exynos_tmu.c +++
>>>> b/drivers/thermal/samsung/exynos_tmu.c @@ -1392,6 +1392,8 @@
>>>> err_clk_sec: if (!IS_ERR(data->clk_sec))
>>>>               clk_unprepare(data->clk_sec);
>>>>  err_sensor:
>>>> +     if (!IS_ERR_OR_NULL(data->regulator))
>>>> +             regulator_disable(data->regulator);
>>>>       thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd);
>>>>
>>>>       return ret;
>>>
>>> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
>>>
>>> I will test it and afterwards add to samsung-thermal tree.
>>
>> Hi ?ukasz,
>>
>> I can't find this patch in v4.2-rc1 or your tree. What happened?
> 
> I will got together with Chanowoo patches. I will send PR today to
> Eduardo.

Thanks!

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukasz Majewski July 6, 2015, 3:05 p.m. UTC | #6
Hi Krzysztof,

> 2015-06-09 1:14 GMT+09:00 Lukasz Majewski <l.majewski@samsung.com>:
> > Hi Krzysztof,
> >
> >> During probe the regulator (if present) was enabled but not
> >> disabled in case of failure. So an unsuccessful probe lead to
> >> enabling the regulator which was actually not needed because the
> >> device was not enabled.
> >>
> >> Additionally each deferred probe lead to increase of regulator
> >> enable count so it would not be effectively disabled during
> >> removal of the device.
> >
> > Thanks for catching this.
> >
> >>
> >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> >> Fixes: 498d22f616f6 ("thermal: exynos: Support for TMU regulator
> >> defined at device tree") Cc: <stable@vger.kernel.org>
> >>
> >> ---
> >>
> >> I am not entirely convinced that this should go to stable. Leaving
> >> a regulator enabled in case of probe failure (no exynos TMU
> >> device) or after deferred probe (regulator won't be disabled
> >> during device removal) is not a critical issue, just leaks power.
> >> ---
> >>  drivers/thermal/samsung/exynos_tmu.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/thermal/samsung/exynos_tmu.c
> >> b/drivers/thermal/samsung/exynos_tmu.c index
> >> 531f4b179871..13c3aceed19d 100644 ---
> >> a/drivers/thermal/samsung/exynos_tmu.c +++
> >> b/drivers/thermal/samsung/exynos_tmu.c @@ -1392,6 +1392,8 @@
> >> err_clk_sec: if (!IS_ERR(data->clk_sec))
> >>               clk_unprepare(data->clk_sec);
> >>  err_sensor:
> >> +     if (!IS_ERR_OR_NULL(data->regulator))
> >> +             regulator_disable(data->regulator);
> >>       thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd);
> >>
> >>       return ret;
> >
> > Acked-by: Lukasz Majewski <l.majewski@samsung.com>
> >
> > I will test it and afterwards add to samsung-thermal tree.
> 
> Hi ?ukasz,
> 
> I can't find this patch in v4.2-rc1 or your tree. What happened?
> 
> Best regards,
> Krzysztof

Applied to linux-samsung-thermal.git

Thanks for fix and sorry for the delay.
diff mbox

Patch

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 531f4b179871..13c3aceed19d 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -1392,6 +1392,8 @@  err_clk_sec:
 	if (!IS_ERR(data->clk_sec))
 		clk_unprepare(data->clk_sec);
 err_sensor:
+	if (!IS_ERR_OR_NULL(data->regulator))
+		regulator_disable(data->regulator);
 	thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd);
 
 	return ret;