| Message ID | 1446127050-5957-1-git-send-email-balbi@ti.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e6b5140b706689a38aaeabd9de8fb3e1531cf9cb |
| Headers | show |
On 10/29/2015 03:57 PM, Felipe Balbi wrote: > there's no need to call pm_runtime_get_sync() > followed by pm_runtime_put(). We should, instead, > just call pm_runtime_put_sync() and pm_runtime_disable(). Sry, but why do we need to call pm_runtime_put[_sync]() here? My be just pm_runtime_disable() will be ok? > > Signed-off-by: Felipe Balbi <balbi@ti.com> > --- > drivers/spi/spi-ti-qspi.c | 11 +---------- > 1 file changed, 1 insertion(+), 10 deletions(-) > > diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c > index 69c1a95b0615..64318fcfacf2 100644 > --- a/drivers/spi/spi-ti-qspi.c > +++ b/drivers/spi/spi-ti-qspi.c > @@ -554,16 +554,7 @@ free_master: > > static int ti_qspi_remove(struct platform_device *pdev) > { > - struct ti_qspi *qspi = platform_get_drvdata(pdev); > - int ret; > - > - ret = pm_runtime_get_sync(qspi->dev); > - if (ret < 0) { > - dev_err(qspi->dev, "pm_runtime_get_sync() failed\n"); > - return ret; > - } > - > - pm_runtime_put(qspi->dev); > + pm_runtime_put_sync(&pdev->dev); > pm_runtime_disable(&pdev->dev); > > return 0; >
Grygorii Strashko <grygorii.strashko@ti.com> writes: > On 10/29/2015 03:57 PM, Felipe Balbi wrote: >> there's no need to call pm_runtime_get_sync() >> followed by pm_runtime_put(). We should, instead, >> just call pm_runtime_put_sync() and pm_runtime_disable(). > > Sry, but why do we need to call pm_runtime_put[_sync]() here? > > My be just pm_runtime_disable() will be ok? and disable with unbalanced pm_runtime_get() ?
On 11/02/2015 05:20 PM, Felipe Balbi wrote: > Grygorii Strashko <grygorii.strashko@ti.com> writes: > >> On 10/29/2015 03:57 PM, Felipe Balbi wrote: >>> there's no need to call pm_runtime_get_sync() >>> followed by pm_runtime_put(). We should, instead, >>> just call pm_runtime_put_sync() and pm_runtime_disable(). >> >> Sry, but why do we need to call pm_runtime_put[_sync]() here? >> >> My be just pm_runtime_disable() will be ok? > > and disable with unbalanced pm_runtime_get() ? > Which one is unbalanced pm_runtime_get()? There are no pm_runtime_get() in probe, so there you are going to introduce unbalanced pm_runtime_put_sync() actually :(
hi, Grygorii Strashko <grygorii.strashko@ti.com> writes: > On 11/02/2015 05:20 PM, Felipe Balbi wrote: >> Grygorii Strashko <grygorii.strashko@ti.com> writes: >> >>> On 10/29/2015 03:57 PM, Felipe Balbi wrote: >>>> there's no need to call pm_runtime_get_sync() >>>> followed by pm_runtime_put(). We should, instead, >>>> just call pm_runtime_put_sync() and pm_runtime_disable(). >>> >>> Sry, but why do we need to call pm_runtime_put[_sync]() here? >>> >>> My be just pm_runtime_disable() will be ok? >> >> and disable with unbalanced pm_runtime_get() ? >> > > Which one is unbalanced pm_runtime_get()? > There are no pm_runtime_get() in probe, so there you are > going to introduce unbalanced pm_runtime_put_sync() actually :( look at ti_qspi_setup(). I _do_ see, however, that it calls pm_runtime_put_autosuspend() in the same function; what happens if driver is removed after ti_qspi_setup() runs but before put_autosuspend() has time to actually run ?
On 11/02/2015 06:06 PM, Felipe Balbi wrote: > > hi, > > Grygorii Strashko <grygorii.strashko@ti.com> writes: >> On 11/02/2015 05:20 PM, Felipe Balbi wrote: >>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>> >>>> On 10/29/2015 03:57 PM, Felipe Balbi wrote: >>>>> there's no need to call pm_runtime_get_sync() >>>>> followed by pm_runtime_put(). We should, instead, >>>>> just call pm_runtime_put_sync() and pm_runtime_disable(). >>>> >>>> Sry, but why do we need to call pm_runtime_put[_sync]() here? >>>> >>>> My be just pm_runtime_disable() will be ok? >>> >>> and disable with unbalanced pm_runtime_get() ? >>> >> >> Which one is unbalanced pm_runtime_get()? >> There are no pm_runtime_get() in probe, so there you are >> going to introduce unbalanced pm_runtime_put_sync() actually :( > > look at ti_qspi_setup(). I _do_ see, however, that it calls > pm_runtime_put_autosuspend() in the same function; what happens if > driver is removed after ti_qspi_setup() runs but before > put_autosuspend() has time to actually run ? > Seems nothing :) If I understand code in __device_release_driver() right: the .remove() callback will be called after pm_runtime_put_sync() and device should be disabled at this moment. Also, note that ti_qspi_setup() will be called for each SPI device attached to SPI master and further PM management of SPI master is performed by SPI core from __spi_pump_messages().
Hi, Grygorii Strashko <grygorii.strashko@ti.com> writes: > On 11/02/2015 06:06 PM, Felipe Balbi wrote: >> >> hi, >> >> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>> On 11/02/2015 05:20 PM, Felipe Balbi wrote: >>>> Grygorii Strashko <grygorii.strashko@ti.com> writes: >>>> >>>>> On 10/29/2015 03:57 PM, Felipe Balbi wrote: >>>>>> there's no need to call pm_runtime_get_sync() >>>>>> followed by pm_runtime_put(). We should, instead, >>>>>> just call pm_runtime_put_sync() and pm_runtime_disable(). >>>>> >>>>> Sry, but why do we need to call pm_runtime_put[_sync]() here? >>>>> >>>>> My be just pm_runtime_disable() will be ok? >>>> >>>> and disable with unbalanced pm_runtime_get() ? >>>> >>> >>> Which one is unbalanced pm_runtime_get()? >>> There are no pm_runtime_get() in probe, so there you are >>> going to introduce unbalanced pm_runtime_put_sync() actually :( >> >> look at ti_qspi_setup(). I _do_ see, however, that it calls >> pm_runtime_put_autosuspend() in the same function; what happens if >> driver is removed after ti_qspi_setup() runs but before >> put_autosuspend() has time to actually run ? >> > > Seems nothing :) If I understand code in __device_release_driver() > right: the .remove() callback will be called after > pm_runtime_put_sync() and device should be disabled at this moment. > > Also, note that ti_qspi_setup() will be called for each SPI device > attached to SPI master and further PM management of SPI master is > performed by SPI core from __spi_pump_messages(). all right, do you want to send a patch fixing it ?
diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c index 69c1a95b0615..64318fcfacf2 100644 --- a/drivers/spi/spi-ti-qspi.c +++ b/drivers/spi/spi-ti-qspi.c @@ -554,16 +554,7 @@ free_master: static int ti_qspi_remove(struct platform_device *pdev) { - struct ti_qspi *qspi = platform_get_drvdata(pdev); - int ret; - - ret = pm_runtime_get_sync(qspi->dev); - if (ret < 0) { - dev_err(qspi->dev, "pm_runtime_get_sync() failed\n"); - return ret; - } - - pm_runtime_put(qspi->dev); + pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev); return 0;
there's no need to call pm_runtime_get_sync() followed by pm_runtime_put(). We should, instead, just call pm_runtime_put_sync() and pm_runtime_disable(). Signed-off-by: Felipe Balbi <balbi@ti.com> --- drivers/spi/spi-ti-qspi.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)