| Message ID | 1465916848-8207-3-git-send-email-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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 --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; }
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(-)