[3/4] soundwire: bus: check if pm runtime is enabled before calling

Message ID 20190328134134.22479-4-srinivas.kandagatla@linaro.org
State New
Headers show
Series
  • soundwire: few trival fixes and cleanups.
Related show

Commit Message

Srinivas Kandagatla March 28, 2019, 1:41 p.m.
Check the device has pm runtime enabled before returning error.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

---
 drivers/soundwire/bus.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

-- 
2.21.0

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Comments

Pierre-Louis Bossart March 28, 2019, 1:55 p.m. | #1
On 3/28/19 9:41 AM, Srinivas Kandagatla wrote:
> Check the device has pm runtime enabled before returning error.

> 

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> ---

>   drivers/soundwire/bus.c | 16 ++++++++++------

>   1 file changed, 10 insertions(+), 6 deletions(-)

> 

> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c

> index 1cbfedfc20ef..101562a6fb0d 100644

> --- a/drivers/soundwire/bus.c

> +++ b/drivers/soundwire/bus.c

> @@ -327,9 +327,11 @@ int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)

>   	if (ret < 0)

>   		return ret;

>   

> -	ret = pm_runtime_get_sync(slave->bus->dev);

> -	if (ret < 0)

> -		return ret;

> +	if (pm_runtime_enabled(slave->bus->dev)) {

> +		ret = pm_runtime_get_sync(slave->bus->dev);


Is this an recommended/accepted sequence in kernel circles? I did a 
quick git grep and don't see anyone using this sort of tests.


> +		if (ret < 0)

> +			return ret;

> +	}

>   

>   	ret = sdw_transfer(slave->bus, &msg);

>   	pm_runtime_put(slave->bus->dev);


and the weird thing is that you don't test for the put() case?

> @@ -355,9 +357,11 @@ int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)

>   	if (ret < 0)

>   		return ret;

>   

> -	ret = pm_runtime_get_sync(slave->bus->dev);

> -	if (ret < 0)

> -		return ret;

> +	if (pm_runtime_enabled(slave->bus->dev)) {

> +		ret = pm_runtime_get_sync(slave->bus->dev);

> +		if (ret < 0)

> +			return ret;

> +	}

>   

>   	ret = sdw_transfer(slave->bus, &msg);

>   	pm_runtime_put(slave->bus->dev);

> 


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Srinivas Kandagatla March 28, 2019, 2:37 p.m. | #2
On 28/03/2019 13:55, Pierre-Louis Bossart wrote:
> On 3/28/19 9:41 AM, Srinivas Kandagatla wrote:
>> Check the device has pm runtime enabled before returning error.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/soundwire/bus.c | 16 ++++++++++------
>>   1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
>> index 1cbfedfc20ef..101562a6fb0d 100644
>> --- a/drivers/soundwire/bus.c
>> +++ b/drivers/soundwire/bus.c
>> @@ -327,9 +327,11 @@ int sdw_nread(struct sdw_slave *slave, u32 addr, 
>> size_t count, u8 *val)
>>       if (ret < 0)
>>           return ret;
>> -    ret = pm_runtime_get_sync(slave->bus->dev);
>> -    if (ret < 0)
>> -        return ret;
>> +    if (pm_runtime_enabled(slave->bus->dev)) {
>> +        ret = pm_runtime_get_sync(slave->bus->dev);
> 
> Is this an recommended/accepted sequence in kernel circles? I did a 
> quick git grep and don't see anyone using this sort of tests.

There are lots of users in kernel with such sequences, ex:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/arm-smmu.c?h=v5.1-rc2#n279

> 
> 
>> +        if (ret < 0)
>> +            return ret;
>> +    }
>>       ret = sdw_transfer(slave->bus, &msg);
>>       pm_runtime_put(slave->bus->dev);
> 
> and the weird thing is that you don't test for the put() case?
> 
>> @@ -355,9 +357,11 @@ int sdw_nwrite(struct sdw_slave *slave, u32 addr, 
>> size_t count, u8 *val)
>>       if (ret < 0)
>>           return ret;
>> -    ret = pm_runtime_get_sync(slave->bus->dev);
>> -    if (ret < 0)
>> -        return ret;
>> +    if (pm_runtime_enabled(slave->bus->dev)) {
>> +        ret = pm_runtime_get_sync(slave->bus->dev);
>> +        if (ret < 0)
>> +            return ret;
>> +    }
>>       ret = sdw_transfer(slave->bus, &msg);
>>       pm_runtime_put(slave->bus->dev);

I should add a check in this path as well.

Thanks,
srini

>>
>
Vinod Koul March 29, 2019, 5:58 a.m. | #3
On 28-03-19, 14:37, Srinivas Kandagatla wrote:
> 

> 

> On 28/03/2019 13:55, Pierre-Louis Bossart wrote:

> > On 3/28/19 9:41 AM, Srinivas Kandagatla wrote:

> > > Check the device has pm runtime enabled before returning error.

> > > 

> > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> > > ---

> > >   drivers/soundwire/bus.c | 16 ++++++++++------

> > >   1 file changed, 10 insertions(+), 6 deletions(-)

> > > 

> > > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c

> > > index 1cbfedfc20ef..101562a6fb0d 100644

> > > --- a/drivers/soundwire/bus.c

> > > +++ b/drivers/soundwire/bus.c

> > > @@ -327,9 +327,11 @@ int sdw_nread(struct sdw_slave *slave, u32

> > > addr, size_t count, u8 *val)

> > >       if (ret < 0)

> > >           return ret;

> > > -    ret = pm_runtime_get_sync(slave->bus->dev);

> > > -    if (ret < 0)

> > > -        return ret;

> > > +    if (pm_runtime_enabled(slave->bus->dev)) {

> > > +        ret = pm_runtime_get_sync(slave->bus->dev);

> > 

> > Is this an recommended/accepted sequence in kernel circles? I did a

> > quick git grep and don't see anyone using this sort of tests.

> 

> There are lots of users in kernel with such sequences, ex:

> 

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/arm-smmu.c?h=v5.1-rc2#n279


Sorry not a fan of this :) I think PM core should do this :D

I guess you are trying to solve the case where PM can be disabled for a
device and pm_runtime_get_sync() returns error right?

That would be quite a common sequence in most frameworks..

Thanks


> 

> > 

> > 

> > > +        if (ret < 0)

> > > +            return ret;

> > > +    }

> > >       ret = sdw_transfer(slave->bus, &msg);

> > >       pm_runtime_put(slave->bus->dev);

> > 

> > and the weird thing is that you don't test for the put() case?

> > 

> > > @@ -355,9 +357,11 @@ int sdw_nwrite(struct sdw_slave *slave, u32

> > > addr, size_t count, u8 *val)

> > >       if (ret < 0)

> > >           return ret;

> > > -    ret = pm_runtime_get_sync(slave->bus->dev);

> > > -    if (ret < 0)

> > > -        return ret;

> > > +    if (pm_runtime_enabled(slave->bus->dev)) {

> > > +        ret = pm_runtime_get_sync(slave->bus->dev);

> > > +        if (ret < 0)

> > > +            return ret;

> > > +    }

> > >       ret = sdw_transfer(slave->bus, &msg);

> > >       pm_runtime_put(slave->bus->dev);

> 

> I should add a check in this path as well.

> 

> Thanks,

> srini

> 

> > > 

> > 


-- 
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Srinivas Kandagatla March 29, 2019, 10:02 a.m. | #4
On 29/03/2019 05:58, Vinod Koul wrote:
>> There are lots of users in kernel with such sequences, ex:

>>

>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/arm-smmu.c?h=v5.1-rc2#n279

> Sorry not a fan of this:)  I think PM core should do this :D

> 

Yes, I would prefer that too, There might be a reason why its not done 
in the pm core, I will catchup with Ulf next week and see if we can add 
checks to return -ENOSUPPORT  or something more sensible error code to 
users.


Thanks,
srini
> I guess you are trying to solve the case where PM can be disabled for a

> device and pm_runtime_get_sync() returns error right?

> 

> That would be quite a common sequence in most frameworks..

> 

> Thanks

> 

> 

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Ranjani Sridharan April 5, 2019, 3:26 p.m. | #5
On Thu, 2019-03-28 at 09:55 -0400, Pierre-Louis Bossart wrote:
> On 3/28/19 9:41 AM, Srinivas Kandagatla wrote:

> > Check the device has pm runtime enabled before returning error.

> > 

> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> > ---

> >   drivers/soundwire/bus.c | 16 ++++++++++------

> >   1 file changed, 10 insertions(+), 6 deletions(-)

> > 

> > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c

> > index 1cbfedfc20ef..101562a6fb0d 100644

> > --- a/drivers/soundwire/bus.c

> > +++ b/drivers/soundwire/bus.c

> > @@ -327,9 +327,11 @@ int sdw_nread(struct sdw_slave *slave, u32

> > addr, size_t count, u8 *val)

> >   	if (ret < 0)

> >   		return ret;

> >   

> > -	ret = pm_runtime_get_sync(slave->bus->dev);

> > -	if (ret < 0)

> > -		return ret;

> > +	if (pm_runtime_enabled(slave->bus->dev)) {

> > +		ret = pm_runtime_get_sync(slave->bus->dev);

> 

> Is this an recommended/accepted sequence in kernel circles? I did a 

> quick git grep and don't see anyone using this sort of tests.

Hi Srinivas/Pierre,

Sorry for the delayed reply.

The only instance where I have seen the pm_runtime_get_sync() fail is
not because pm_runtime was disabled. But it is because the power is
powered off when trying to do a pm_runtime_get_sync().

I'm not very familiar with the code in soundwire yet, but is it
possible that the pm_domain supplier has powered off the soundwire
device and would cause a failure in pm_runtime_get_sync()?

Thanks,
Ranjani

> 

> 

> > +		if (ret < 0)

> > +			return ret;

> > +	}

> >   

> >   	ret = sdw_transfer(slave->bus, &msg);

> >   	pm_runtime_put(slave->bus->dev);

> 

> and the weird thing is that you don't test for the put() case?

> 

> > @@ -355,9 +357,11 @@ int sdw_nwrite(struct sdw_slave *slave, u32

> > addr, size_t count, u8 *val)

> >   	if (ret < 0)

> >   		return ret;

> >   

> > -	ret = pm_runtime_get_sync(slave->bus->dev);

> > -	if (ret < 0)

> > -		return ret;

> > +	if (pm_runtime_enabled(slave->bus->dev)) {

> > +		ret = pm_runtime_get_sync(slave->bus->dev);

> > +		if (ret < 0)

> > +			return ret;

> > +	}

> >   

> >   	ret = sdw_transfer(slave->bus, &msg);

> >   	pm_runtime_put(slave->bus->dev);

> > 

> 

> _______________________________________________

> Alsa-devel mailing list

> Alsa-devel@alsa-project.org

> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Patch

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 1cbfedfc20ef..101562a6fb0d 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -327,9 +327,11 @@  int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
 	if (ret < 0)
 		return ret;
 
-	ret = pm_runtime_get_sync(slave->bus->dev);
-	if (ret < 0)
-		return ret;
+	if (pm_runtime_enabled(slave->bus->dev)) {
+		ret = pm_runtime_get_sync(slave->bus->dev);
+		if (ret < 0)
+			return ret;
+	}
 
 	ret = sdw_transfer(slave->bus, &msg);
 	pm_runtime_put(slave->bus->dev);
@@ -355,9 +357,11 @@  int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
 	if (ret < 0)
 		return ret;
 
-	ret = pm_runtime_get_sync(slave->bus->dev);
-	if (ret < 0)
-		return ret;
+	if (pm_runtime_enabled(slave->bus->dev)) {
+		ret = pm_runtime_get_sync(slave->bus->dev);
+		if (ret < 0)
+			return ret;
+	}
 
 	ret = sdw_transfer(slave->bus, &msg);
 	pm_runtime_put(slave->bus->dev);