diff mbox

[02/10] i2c: designware-platdrv: Gate clk in error path in ->probe()

Message ID 1465916848-8207-3-git-send-email-ulf.hansson@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Ulf Hansson June 14, 2016, 3:07 p.m. UTC
The call to i2c_dw_probe() may fail in ->probe() in which case the clock
remains ungated. Fix the error path by gating the clock before the error
code is returned.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Andy Shevchenko June 14, 2016, 3:22 p.m. UTC | #1
On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
> The call to i2c_dw_probe() may fail in ->probe() in which case the
> clock
> remains ungated. Fix the error path by gating the clock before the
> error
> code is returned.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index e39962b..19174e7 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
>  	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
>  	adap->dev.of_node = pdev->dev.of_node;
> 

 
> +	pm_runtime_get_noresume(&pdev->dev);

> +	pm_runtime_put(&pdev->dev);

I don't see an explanation of these add-ons.
Ulf Hansson June 15, 2016, 7:16 a.m. UTC | #2
On 14 June 2016 at 17:22, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
>> The call to i2c_dw_probe() may fail in ->probe() in which case the
>> clock
>> remains ungated. Fix the error path by gating the clock before the
>> error
>> code is returned.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index e39962b..19174e7 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct
>> platform_device *pdev)
>>       ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
>>       adap->dev.of_node = pdev->dev.of_node;
>>
>
>
>> +     pm_runtime_get_noresume(&pdev->dev);
>
>> +     pm_runtime_put(&pdev->dev);
>
> I don't see an explanation of these add-ons.

As we explicitly do clk_disable_unprepare() in the error path, we must
prevent the ->runtime_suspend() callback to be called during this
period.

This is a common pattern used by many drivers deploying runtime PM
support, which is probably why I left out the explanation.

I can update the change log with this information, if you like.

Kind regards
Uffe
Jarkko Nikula June 15, 2016, 12:07 p.m. UTC | #3
On 06/15/2016 10:16 AM, Ulf Hansson wrote:
> On 14 June 2016 at 17:22, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
>>> The call to i2c_dw_probe() may fail in ->probe() in which case the
>>> clock
>>> remains ungated. Fix the error path by gating the clock before the
>>> error
>>> code is returned.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> index e39962b..19174e7 100644
>>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> @@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct
>>> platform_device *pdev)
>>>       ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
>>>       adap->dev.of_node = pdev->dev.of_node;
>>>
>>
>>
>>> +     pm_runtime_get_noresume(&pdev->dev);
>>
>>> +     pm_runtime_put(&pdev->dev);
>>
>> I don't see an explanation of these add-ons.
>
> As we explicitly do clk_disable_unprepare() in the error path, we must
> prevent the ->runtime_suspend() callback to be called during this
> period.
>
> This is a common pattern used by many drivers deploying runtime PM
> support, which is probably why I left out the explanation.
>
What would cause here runtime suspend during probe? Also pairing 
pm_runtime_get_noresume() with pm_runtime_put() instead of 
pm_runtime_put_noidle() looks suspicious to me.
Ulf Hansson June 20, 2016, 4:41 a.m. UTC | #4
On 15 June 2016 at 14:07, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> On 06/15/2016 10:16 AM, Ulf Hansson wrote:
>>
>> On 14 June 2016 at 17:22, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>>>
>>> On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
>>>>
>>>> The call to i2c_dw_probe() may fail in ->probe() in which case the
>>>> clock
>>>> remains ungated. Fix the error path by gating the clock before the
>>>> error
>>>> code is returned.
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>> ---
>>>>  drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++--
>>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>>>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>>>> index e39962b..19174e7 100644
>>>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>>>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>>>> @@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct
>>>> platform_device *pdev)
>>>>       ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
>>>>       adap->dev.of_node = pdev->dev.of_node;
>>>>
>>>
>>>
>>>> +     pm_runtime_get_noresume(&pdev->dev);
>>>
>>>
>>>> +     pm_runtime_put(&pdev->dev);
>>>
>>>
>>> I don't see an explanation of these add-ons.
>>
>>
>> As we explicitly do clk_disable_unprepare() in the error path, we must
>> prevent the ->runtime_suspend() callback to be called during this
>> period.
>>
>> This is a common pattern used by many drivers deploying runtime PM
>> support, which is probably why I left out the explanation.
>>
> What would cause here runtime suspend during probe? Also pairing

In practice this might not happen, although in theory it could...

The struct device is accessible by others than the driver (userspace
through sysfs for example). So, if "someone" would invoke
pm_runtime_suspend() (or a similar runtime PM API) for the device,
that would cause the ->runtime_suspend() callback to be invoked within
this period.

I suggest we just make sure to protect from this to happen.

> pm_runtime_get_noresume() with pm_runtime_put() instead of
> pm_runtime_put_noidle() looks suspicious to me.
>

It's not a problem. There's two cases here:

*) Runtime PM is enabled.
In this case the pm_runtime_put() will decrease the runtime PM usage
count and likely cause the device to be runtime suspended. Earlier,
the similar would happen as the driver core do a pm_request_idle()
when the ->probe() has completed.

**) Runtime PM is disabled.
In this case, the pm_runtime_put() has the same effect as a
pm_runtime_put_noidle(). It simply decrease the runtime PM usage
count, that's it.

Following this reasoning, one could decide to replace the
pm_runtime_put() with pm_runtime_put_noidle(), the behaviour will be
very similar.

If you would like to change to pm_runtime_put_noidle(), that's
perfectly fine by me.

Kind regards
Uffe
diff mbox

Patch

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index e39962b..19174e7 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -235,6 +235,7 @@  static int dw_i2c_plat_probe(struct platform_device *pdev)
 	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->dev.of_node = pdev->dev.of_node;
 
+	pm_runtime_get_noresume(&pdev->dev);
 	if (dev->pm_runtime_disabled) {
 		pm_runtime_forbid(&pdev->dev);
 	} else {
@@ -245,8 +246,13 @@  static int dw_i2c_plat_probe(struct platform_device *pdev)
 	}
 
 	r = i2c_dw_probe(dev);
-	if (r && !dev->pm_runtime_disabled)
-		pm_runtime_disable(&pdev->dev);
+	if (r) {
+		if (!IS_ERR(dev->clk))
+			clk_disable_unprepare(dev->clk);
+		if (!dev->pm_runtime_disabled)
+			pm_runtime_disable(&pdev->dev);
+	}
+	pm_runtime_put(&pdev->dev);
 
 	return r;
 }