[PATCH] spi: ti-qspi: improve ->remove() callback

[PATCH] spi: ti-qspi: improve ->remove() callback

[Felipe Balbi]

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-l0cyMroinI0@public.gmane.org>
---
 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;
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: ti-qspi: improve ->remove() callback

[Grygorii Strashko]

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-l0cyMroinI0@public.gmane.org>
> ---
>   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;
>


-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: ti-qspi: improve ->remove() callback

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> 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() ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] spi: ti-qspi: improve ->remove() callback

[Grygorii Strashko]

On 11/02/2015 05:20 PM, Felipe Balbi wrote:
> Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> 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 :(

-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: ti-qspi: improve ->remove() callback

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1048 bytes --]


hi,

Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> writes:
> On 11/02/2015 05:20 PM, Felipe Balbi wrote:
>> Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> 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 ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] spi: ti-qspi: improve ->remove() callback

[Grygorii Strashko]

On 11/02/2015 06:06 PM, Felipe Balbi wrote:
> 
> hi,
> 
> Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> writes:
>> On 11/02/2015 05:20 PM, Felipe Balbi wrote:
>>> Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> 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().

-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: ti-qspi: improve ->remove() callback

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]


Hi,

Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> writes:
> On 11/02/2015 06:06 PM, Felipe Balbi wrote:
>> 
>> hi,
>> 
>> Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> writes:
>>> On 11/02/2015 05:20 PM, Felipe Balbi wrote:
>>>> Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org> 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 ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]