| Message ID | 20190328134134.22479-4-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: few trival fixes and cleanups. | expand |
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); >
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 >> >
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 > > > > > >
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 > >
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
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);
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(-)