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/driv...
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