| Message ID | 20190725234032.21152-18-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: updates for 5.4 | expand |
Hi Pierre, I might be wrong but this doesn't seem right to me. (Supposedly) all RT-PM functions check for "enabled" internally. The only thing that can happen is that if RT-PM isn't enabled some of those functions will return an error. So, in those cases where the return value of RT-PM functions isn't checked, I don't think you need to do anything. Where it is checked maybe do + if (ret < 0 && pm_runtime_enabled(slave->bus->dev)) Thanks Guennadi On Thu, Jul 25, 2019 at 06:40:09PM -0500, Pierre-Louis Bossart wrote: > Not all platforms support runtime_pm for now, let's use runtime_pm > only when enabled. > > Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/bus.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c > index 5ad4109dc72f..0a45dc5713df 100644 > --- a/drivers/soundwire/bus.c > +++ b/drivers/soundwire/bus.c > @@ -332,12 +332,16 @@ 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); > + > + if (pm_runtime_enabled(slave->bus->dev)) > + pm_runtime_put(slave->bus->dev); > > return ret; > } > @@ -359,13 +363,16 @@ int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) > slave->dev_num, SDW_MSG_FLAG_WRITE, 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); > + > + if (pm_runtime_enabled(slave->bus->dev)) > + pm_runtime_put(slave->bus->dev); > > return ret; > } > -- > 2.20.1 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Hello, I while back I proposed a patch for this, but it went nowhere. https://patchwork.kernel.org/patch/10887405/ Maybe something similar can be implemented? Jan > On 26 Jul 2019, at 09:39, Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> wrote: > > EXTERNAL MAIL > > > Hi Pierre, > > I might be wrong but this doesn't seem right to me. (Supposedly) all RT-PM > functions check for "enabled" internally. The only thing that can happen is > that if RT-PM isn't enabled some of those functions will return an error. > So, in those cases where the return value of RT-PM functions isn't checked, > I don't think you need to do anything. Where it is checked maybe do > > + if (ret < 0 && pm_runtime_enabled(slave->bus->dev)) > > Thanks > Guennadi > > On Thu, Jul 25, 2019 at 06:40:09PM -0500, Pierre-Louis Bossart wrote: >> Not all platforms support runtime_pm for now, let's use runtime_pm >> only when enabled. >> >> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> --- >> drivers/soundwire/bus.c | 25 ++++++++++++++++--------- >> 1 file changed, 16 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c >> index 5ad4109dc72f..0a45dc5713df 100644 >> --- a/drivers/soundwire/bus.c >> +++ b/drivers/soundwire/bus.c >> @@ -332,12 +332,16 @@ 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); >> + >> + if (pm_runtime_enabled(slave->bus->dev)) >> + pm_runtime_put(slave->bus->dev); >> >> return ret; >> } >> @@ -359,13 +363,16 @@ int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) >> slave->dev_num, SDW_MSG_FLAG_WRITE, 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); >> + >> + if (pm_runtime_enabled(slave->bus->dev)) >> + pm_runtime_put(slave->bus->dev); >> >> return ret; >> } >> -- >> 2.20.1 >> >> _______________________________________________ >> Alsa-devel mailing list >> Alsa-devel@alsa-project.org >> https://urldefense.proofpoint.com/v2/url?u=https-3A__mailman.alsa-2Dproject.org_mailman_listinfo_alsa-2Ddevel&d=DwIBAg&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=g7GAQENVXx_RQdyXHInPMg&m=vETGQLSPeGb7K_ZsXv4Tl3VFfdXzyummTDga97ozJcg&s=LiW4SToh5U0zhnkox54oRhJ1u3vFNbBB9nmzRDuCDjI&e=
Hi Jan, On Fri, Jul 26, 2019 at 07:47:04AM +0000, Jan Kotas wrote: > Hello, > > I while back I proposed a patch for this, but it went nowhere. > > https://patchwork.kernel.org/patch/10887405/ > Maybe something similar can be implemented? Yes, I was thinking about checkint -EACCESS too, but then I noticed this code in rpm_resume(): else if (dev->power.disable_depth == 1 && dev->power.is_suspended && dev->power.runtime_status == RPM_ACTIVE) retval = 1; i.e. if RT-PM is disabled on the device (but only exactly once?..) and it's active and the device is suspended for a system suspend, the function will return 1. I don't understand the logic of this code, but it seems to me it could break the -EACCESS check? Thanks Guennadi > Jan > > > On 26 Jul 2019, at 09:39, Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> wrote: > > > > EXTERNAL MAIL > > > > > > Hi Pierre, > > > > I might be wrong but this doesn't seem right to me. (Supposedly) all RT-PM > > functions check for "enabled" internally. The only thing that can happen is > > that if RT-PM isn't enabled some of those functions will return an error. > > So, in those cases where the return value of RT-PM functions isn't checked, > > I don't think you need to do anything. Where it is checked maybe do > > > > + if (ret < 0 && pm_runtime_enabled(slave->bus->dev)) > > > > Thanks > > Guennadi > > > > On Thu, Jul 25, 2019 at 06:40:09PM -0500, Pierre-Louis Bossart wrote: > >> Not all platforms support runtime_pm for now, let's use runtime_pm > >> only when enabled. > >> > >> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > >> --- > >> drivers/soundwire/bus.c | 25 ++++++++++++++++--------- > >> 1 file changed, 16 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c > >> index 5ad4109dc72f..0a45dc5713df 100644 > >> --- a/drivers/soundwire/bus.c > >> +++ b/drivers/soundwire/bus.c > >> @@ -332,12 +332,16 @@ 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); > >> + > >> + if (pm_runtime_enabled(slave->bus->dev)) > >> + pm_runtime_put(slave->bus->dev); > >> > >> return ret; > >> } > >> @@ -359,13 +363,16 @@ int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) > >> slave->dev_num, SDW_MSG_FLAG_WRITE, 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); > >> + > >> + if (pm_runtime_enabled(slave->bus->dev)) > >> + pm_runtime_put(slave->bus->dev); > >> > >> return ret; > >> } > >> -- > >> 2.20.1 > >> > >> _______________________________________________ > >> Alsa-devel mailing list > >> Alsa-devel@alsa-project.org > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__mailman.alsa-2Dproject.org_mailman_listinfo_alsa-2Ddevel&d=DwIBAg&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=g7GAQENVXx_RQdyXHInPMg&m=vETGQLSPeGb7K_ZsXv4Tl3VFfdXzyummTDga97ozJcg&s=LiW4SToh5U0zhnkox54oRhJ1u3vFNbBB9nmzRDuCDjI&e= > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> On 26 Jul 2019, at 10:22, Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> wrote: > > EXTERNAL MAIL > > > Hi Jan, > > On Fri, Jul 26, 2019 at 07:47:04AM +0000, Jan Kotas wrote: >> Hello, >> >> I while back I proposed a patch for this, but it went nowhere. >> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_10887405_&d=DwIBAg&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=g7GAQENVXx_RQdyXHInPMg&m=i_0S359hFIVqNgv3fR5_MNzDOHP99trdXszZ-FMiQEE&s=ddktFZYlePh-bC7kXeoKWt4QomupzHATK4FLY4oSWKA&e= >> Maybe something similar can be implemented? > > Yes, I was thinking about checkint -EACCESS too, but then I noticed this code > in rpm_resume(): > > else if (dev->power.disable_depth == 1 && dev->power.is_suspended > && dev->power.runtime_status == RPM_ACTIVE) > retval = 1; > > i.e. if RT-PM is disabled on the device (but only exactly once?..) and it's > active and the device is suspended for a system suspend, the function will > return 1. I don't understand the logic of this code, but it seems to me it > could break the -EACCESS check? > Hi, In such case ret < 0 will not be true, which I think is fine, if I’m understanding you correctly. Regards, Jan > Thanks > Guennadi > >> Jan >> >>> On 26 Jul 2019, at 09:39, Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> wrote: >>> >>> EXTERNAL MAIL >>> >>> >>> Hi Pierre, >>> >>> I might be wrong but this doesn't seem right to me. (Supposedly) all RT-PM >>> functions check for "enabled" internally. The only thing that can happen is >>> that if RT-PM isn't enabled some of those functions will return an error. >>> So, in those cases where the return value of RT-PM functions isn't checked, >>> I don't think you need to do anything. Where it is checked maybe do >>> >>> + if (ret < 0 && pm_runtime_enabled(slave->bus->dev)) >>> >>> Thanks >>> Guennadi >>> >>> On Thu, Jul 25, 2019 at 06:40:09PM -0500, Pierre-Louis Bossart wrote: >>>> Not all platforms support runtime_pm for now, let's use runtime_pm >>>> only when enabled. >>>> >>>> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >>>> --- >>>> drivers/soundwire/bus.c | 25 ++++++++++++++++--------- >>>> 1 file changed, 16 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c >>>> index 5ad4109dc72f..0a45dc5713df 100644 >>>> --- a/drivers/soundwire/bus.c >>>> +++ b/drivers/soundwire/bus.c >>>> @@ -332,12 +332,16 @@ 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); >>>> + >>>> + if (pm_runtime_enabled(slave->bus->dev)) >>>> + pm_runtime_put(slave->bus->dev); >>>> >>>> return ret; >>>> } >>>> @@ -359,13 +363,16 @@ int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) >>>> slave->dev_num, SDW_MSG_FLAG_WRITE, 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); >>>> + >>>> + if (pm_runtime_enabled(slave->bus->dev)) >>>> + pm_runtime_put(slave->bus->dev); >>>> >>>> return ret; >>>> } >>>> -- >>>> 2.20.1 >>>> >>>> _______________________________________________ >>>> Alsa-devel mailing list >>>> Alsa-devel@alsa-project.org >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__mailman.alsa-2Dproject.org_mailman_listinfo_alsa-2Ddevel&d=DwIBAg&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=g7GAQENVXx_RQdyXHInPMg&m=vETGQLSPeGb7K_ZsXv4Tl3VFfdXzyummTDga97ozJcg&s=LiW4SToh5U0zhnkox54oRhJ1u3vFNbBB9nmzRDuCDjI&e= >> >> _______________________________________________ >> Alsa-devel mailing list >> Alsa-devel@alsa-project.org >> https://urldefense.proofpoint.com/v2/url?u=https-3A__mailman.alsa-2Dproject.org_mailman_listinfo_alsa-2Ddevel&d=DwIBAg&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=g7GAQENVXx_RQdyXHInPMg&m=i_0S359hFIVqNgv3fR5_MNzDOHP99trdXszZ-FMiQEE&s=RxPHxKfI3v6Fkh7qzKjq8sNi-5QMoY8XfyMDSquA38o&e=
On Fri, Jul 26, 2019 at 08:33:35AM +0000, Jan Kotas wrote: > > > > On 26 Jul 2019, at 10:22, Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> wrote: > > > > EXTERNAL MAIL > > > > > > Hi Jan, > > > > On Fri, Jul 26, 2019 at 07:47:04AM +0000, Jan Kotas wrote: > >> Hello, > >> > >> I while back I proposed a patch for this, but it went nowhere. > >> > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_10887405_&d=DwIBAg&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=g7GAQENVXx_RQdyXHInPMg&m=i_0S359hFIVqNgv3fR5_MNzDOHP99trdXszZ-FMiQEE&s=ddktFZYlePh-bC7kXeoKWt4QomupzHATK4FLY4oSWKA&e= > >> Maybe something similar can be implemented? > > > > Yes, I was thinking about checkint -EACCESS too, but then I noticed this code > > in rpm_resume(): > > > > else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > && dev->power.runtime_status == RPM_ACTIVE) > > retval = 1; > > > > i.e. if RT-PM is disabled on the device (but only exactly once?..) and it's > > active and the device is suspended for a system suspend, the function will > > return 1. I don't understand the logic of this code, but it seems to me it > > could break the -EACCESS check? > > > > Hi, > > In such case ret < 0 will not be true, which I think is fine, > if I’m understanding you correctly. Yes, if we just have to distinguish a single case "RT-PM is enabled and it failed." Which is indeed the case here, it seems. However if we want to check whether RT-PM is disabled after a call to, say, pm_runtime_get_sync(), then just checking -EACCESS isn't always enough - there can be cases when RT-PM is disabled and the return code is 1. But yes, just for checking for failures, like here, it should be fine. Thanks Guennadi > >> Jan > >> > >>> On 26 Jul 2019, at 09:39, Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> wrote: > >>> > >>> EXTERNAL MAIL > >>> > >>> > >>> Hi Pierre, > >>> > >>> I might be wrong but this doesn't seem right to me. (Supposedly) all RT-PM > >>> functions check for "enabled" internally. The only thing that can happen is > >>> that if RT-PM isn't enabled some of those functions will return an error. > >>> So, in those cases where the return value of RT-PM functions isn't checked, > >>> I don't think you need to do anything. Where it is checked maybe do > >>> > >>> + if (ret < 0 && pm_runtime_enabled(slave->bus->dev)) > >>> > >>> Thanks > >>> Guennadi > >>> > >>> On Thu, Jul 25, 2019 at 06:40:09PM -0500, Pierre-Louis Bossart wrote: > >>>> Not all platforms support runtime_pm for now, let's use runtime_pm > >>>> only when enabled. > >>>> > >>>> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > >>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > >>>> --- > >>>> drivers/soundwire/bus.c | 25 ++++++++++++++++--------- > >>>> 1 file changed, 16 insertions(+), 9 deletions(-) > >>>> > >>>> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c > >>>> index 5ad4109dc72f..0a45dc5713df 100644 > >>>> --- a/drivers/soundwire/bus.c > >>>> +++ b/drivers/soundwire/bus.c > >>>> @@ -332,12 +332,16 @@ 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); > >>>> + > >>>> + if (pm_runtime_enabled(slave->bus->dev)) > >>>> + pm_runtime_put(slave->bus->dev); > >>>> > >>>> return ret; > >>>> } > >>>> @@ -359,13 +363,16 @@ int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) > >>>> slave->dev_num, SDW_MSG_FLAG_WRITE, 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); > >>>> + > >>>> + if (pm_runtime_enabled(slave->bus->dev)) > >>>> + pm_runtime_put(slave->bus->dev); > >>>> > >>>> return ret; > >>>> } > >>>> -- > >>>> 2.20.1 > >>>> > >>>> _______________________________________________ > >>>> Alsa-devel mailing list > >>>> Alsa-devel@alsa-project.org > >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__mailman.alsa-2Dproject.org_mailman_listinfo_alsa-2Ddevel&d=DwIBAg&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=g7GAQENVXx_RQdyXHInPMg&m=vETGQLSPeGb7K_ZsXv4Tl3VFfdXzyummTDga97ozJcg&s=LiW4SToh5U0zhnkox54oRhJ1u3vFNbBB9nmzRDuCDjI&e= > >> > >> _______________________________________________ > >> Alsa-devel mailing list > >> Alsa-devel@alsa-project.org > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__mailman.alsa-2Dproject.org_mailman_listinfo_alsa-2Ddevel&d=DwIBAg&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=g7GAQENVXx_RQdyXHInPMg&m=i_0S359hFIVqNgv3fR5_MNzDOHP99trdXszZ-FMiQEE&s=RxPHxKfI3v6Fkh7qzKjq8sNi-5QMoY8XfyMDSquA38o&e= > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
This thread became unreadable with interleaved top-posting, allow me restate the options and ask PM folks what they think On 7/25/19 6:40 PM, Pierre-Louis Bossart wrote: > Not all platforms support runtime_pm for now, let's use runtime_pm > only when enabled. > > Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/bus.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c > index 5ad4109dc72f..0a45dc5713df 100644 > --- a/drivers/soundwire/bus.c > +++ b/drivers/soundwire/bus.c > @@ -332,12 +332,16 @@ 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); > + > + if (pm_runtime_enabled(slave->bus->dev)) > + pm_runtime_put(slave->bus->dev); This is option1: we explicitly test if pm_runtime is enabled before calling _get_sync() and _put() option2 (suggested by Jan Kotas): catch the -EACCESS error code ret = pm_runtime_get_sync(slave->bus->dev); - if (ret < 0) + if (ret < 0 && ret != -EACCES) return ret; option3: ignore the return value as done in quite a few drivers Are there any other options? I am personally surprised this is not handled in the pm_runtime core, not sure why users need to test for this? Thanks Pierre
On Fri, Jul 26, 2019 at 01:08:57PM -0500, Pierre-Louis Bossart wrote: > This thread became unreadable with interleaved top-posting, allow me restate > the options and ask PM folks what they think > > On 7/25/19 6:40 PM, Pierre-Louis Bossart wrote: > > Not all platforms support runtime_pm for now, let's use runtime_pm > > only when enabled. > > > > Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > --- > > drivers/soundwire/bus.c | 25 ++++++++++++++++--------- > > 1 file changed, 16 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c > > index 5ad4109dc72f..0a45dc5713df 100644 > > --- a/drivers/soundwire/bus.c > > +++ b/drivers/soundwire/bus.c > > @@ -332,12 +332,16 @@ 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); > > + > > + if (pm_runtime_enabled(slave->bus->dev)) > > + pm_runtime_put(slave->bus->dev); > > This is option1: we explicitly test if pm_runtime is enabled before calling > _get_sync() and _put() > > option2 (suggested by Jan Kotas): catch the -EACCESS error code > > ret = pm_runtime_get_sync(slave->bus->dev); > - if (ret < 0) > + if (ret < 0 && ret != -EACCES) > return ret; > > option3: ignore the return value as done in quite a few drivers > > Are there any other options? I am personally surprised this is not handled > in the pm_runtime core, not sure why users need to test for this? option 4: fix this in runtime PM :-) This seems like the best option to me, but probably not the easiest one. Otherwise I'd go with (2), I think, since that's also the official purpose of the -EACCESS return code: https://lists.linuxfoundation.org/pipermail/linux-pm/2011-June/031930.html Thanks Guennadi
On Fri, Jul 26, 2019 at 01:08:57PM -0500, Pierre-Louis Bossart wrote: > This thread became unreadable with interleaved top-posting, allow me restate > the options and ask PM folks what they think > > On 7/25/19 6:40 PM, Pierre-Louis Bossart wrote: > > Not all platforms support runtime_pm for now, let's use runtime_pm > > only when enabled. Just a side note below... > > - ret = pm_runtime_get_sync(slave->bus->dev); > > - if (ret < 0) Here... > > - return ret; > > + if (pm_runtime_enabled(slave->bus->dev)) { > > + ret = pm_runtime_get_sync(slave->bus->dev); > > + if (ret < 0) ...and thus here... > > + return ret; > > + } > > ret = sdw_transfer(slave->bus, &msg); > > - pm_runtime_put(slave->bus->dev); > > + > > + if (pm_runtime_enabled(slave->bus->dev)) > > + pm_runtime_put(slave->bus->dev); > > This is option1: we explicitly test if pm_runtime is enabled before calling > _get_sync() and _put() > > option2 (suggested by Jan Kotas): catch the -EACCESS error code > > ret = pm_runtime_get_sync(slave->bus->dev); > - if (ret < 0) > + if (ret < 0 && ret != -EACCES) ...and here, the pm_runtime_put_noidle() call is missed. > return ret; > > option3: ignore the return value as done in quite a few drivers > > Are there any other options? I am personally surprised this is not handled > in the pm_runtime core, not sure why users need to test for this?
On Fri, Jul 26, 2019 at 08:25:35PM +0200, Guennadi Liakhovetski wrote: > On Fri, Jul 26, 2019 at 01:08:57PM -0500, Pierre-Louis Bossart wrote: > > On 7/25/19 6:40 PM, Pierre-Louis Bossart wrote: > > > Not all platforms support runtime_pm for now, let's use runtime_pm > > > only when enabled. > > option2 (suggested by Jan Kotas): catch the -EACCESS error code > > > > ret = pm_runtime_get_sync(slave->bus->dev); > > - if (ret < 0) > > + if (ret < 0 && ret != -EACCES) > > return ret; > Otherwise I'd go with (2), I think, since > that's also the official purpose of the -EACCESS return code: > > https://lists.linuxfoundation.org/pipermail/linux-pm/2011-June/031930.html And at least we have examples in the kernel drivers/gpu/drm/radeon/radeon_fb.c:57: if (ret < 0 && ret != -EACCES) {
On 7/26/19 2:08 PM, Andy Shevchenko wrote: > On Fri, Jul 26, 2019 at 01:08:57PM -0500, Pierre-Louis Bossart wrote: >> This thread became unreadable with interleaved top-posting, allow me restate >> the options and ask PM folks what they think >> >> On 7/25/19 6:40 PM, Pierre-Louis Bossart wrote: >>> Not all platforms support runtime_pm for now, let's use runtime_pm >>> only when enabled. > > Just a side note below... > >>> - ret = pm_runtime_get_sync(slave->bus->dev); >>> - if (ret < 0) > > Here... > >>> - return ret; >>> + if (pm_runtime_enabled(slave->bus->dev)) { >>> + ret = pm_runtime_get_sync(slave->bus->dev); >>> + if (ret < 0) > > ...and thus here... > >>> + return ret; >>> + } >>> ret = sdw_transfer(slave->bus, &msg); >>> - pm_runtime_put(slave->bus->dev); >>> + >>> + if (pm_runtime_enabled(slave->bus->dev)) >>> + pm_runtime_put(slave->bus->dev); >> >> This is option1: we explicitly test if pm_runtime is enabled before calling >> _get_sync() and _put() >> >> option2 (suggested by Jan Kotas): catch the -EACCESS error code >> >> ret = pm_runtime_get_sync(slave->bus->dev); >> - if (ret < 0) >> + if (ret < 0 && ret != -EACCES) > > ...and here, the pm_runtime_put_noidle() call is missed. yes but in the example you provided, they actually do more work than just decrement the device usage counter: static int radeonfb_open(struct fb_info *info, int user) { struct radeon_fbdev *rfbdev = info->par; struct radeon_device *rdev = rfbdev->rdev; int ret = pm_runtime_get_sync(rdev->ddev->dev); if (ret < 0 && ret != -EACCES) { pm_runtime_mark_last_busy(rdev->ddev->dev); pm_runtime_put_autosuspend(rdev->ddev->dev); return ret; } return 0; } unless I am missing something pm_runtime_put_noidle() and _put_autosuspend() are not equivalent, are they?
On Mon, Jul 29, 2019 at 05:07:39PM -0500, Pierre-Louis Bossart wrote: > On 7/26/19 2:08 PM, Andy Shevchenko wrote: > > On Fri, Jul 26, 2019 at 01:08:57PM -0500, Pierre-Louis Bossart wrote: > > > - if (ret < 0) > > > + if (ret < 0 && ret != -EACCES) > > > > ...and here, the pm_runtime_put_noidle() call is missed. > > yes but in the example you provided, they actually do more work than just > decrement the device usage counter: In their case they would like to do that. You decide what is appropriate call in your case. My point is, that reference counter in case of error handling should be returned back to its value.
On 7/30/19 6:21 AM, Andy Shevchenko wrote: > On Mon, Jul 29, 2019 at 05:07:39PM -0500, Pierre-Louis Bossart wrote: >> On 7/26/19 2:08 PM, Andy Shevchenko wrote: >>> On Fri, Jul 26, 2019 at 01:08:57PM -0500, Pierre-Louis Bossart wrote: > >>>> - if (ret < 0) >>>> + if (ret < 0 && ret != -EACCES) >>> >>> ...and here, the pm_runtime_put_noidle() call is missed. >> >> yes but in the example you provided, they actually do more work than just >> decrement the device usage counter: > > In their case they would like to do that. You decide what is appropriate call > in your case. > > My point is, that reference counter in case of error handling should be > returned back to its value. Agree on the reference count. I am however not clear on the next step and 'what is appropriate'. If pm_runtime_get_sync() failed, then technically the device was not resumed so marking it as last_busy+autosuspend, or using a plain vanilla put() will not result in any action. I must be missing something here.
On Tue, Jul 30, 2019 at 07:57:46AM -0500, Pierre-Louis Bossart wrote: > On 7/30/19 6:21 AM, Andy Shevchenko wrote: > > On Mon, Jul 29, 2019 at 05:07:39PM -0500, Pierre-Louis Bossart wrote: > > > On 7/26/19 2:08 PM, Andy Shevchenko wrote: > > > > On Fri, Jul 26, 2019 at 01:08:57PM -0500, Pierre-Louis Bossart wrote: > > > > > > > - if (ret < 0) > > > > > + if (ret < 0 && ret != -EACCES) > > > > > > > > ...and here, the pm_runtime_put_noidle() call is missed. > > > > > > yes but in the example you provided, they actually do more work than just > > > decrement the device usage counter: > > > > In their case they would like to do that. You decide what is appropriate call > > in your case. > > > > My point is, that reference counter in case of error handling should be > > returned back to its value. > > Agree on the reference count. > I am however not clear on the next step and 'what is appropriate'. > > If pm_runtime_get_sync() failed, then technically the device was not resumed Not so straight. It depends on reference count. It might be true (most cases I think), or not true, if device had been resumed previously by other call. > so marking it as last_busy+autosuspend, or using a plain vanilla put() will > not result in any action. I must be missing something here. put_noidle(). Because if it failed on the first call and was resumed, put() will try to shut it down (since reference count goes to no-user base).
On Tue, Jul 30, 2019 at 06:58:09PM +0300, Andy Shevchenko wrote: > On Tue, Jul 30, 2019 at 07:57:46AM -0500, Pierre-Louis Bossart wrote: > > On 7/30/19 6:21 AM, Andy Shevchenko wrote: > > > On Mon, Jul 29, 2019 at 05:07:39PM -0500, Pierre-Louis Bossart wrote: > > > > On 7/26/19 2:08 PM, Andy Shevchenko wrote: > > so marking it as last_busy+autosuspend, or using a plain vanilla put() will > > not result in any action. I must be missing something here. > > put_noidle(). Because if it failed on the first call and was resumed, put() > will try to shut it down (since reference count goes to no-user base). Sorry, this has to be read as (was -> wasn't) > put_noidle(). Because if it failed on the first call and wasn't resumed, put() > will try to shut it down (since reference count goes to no-user base).
On 25-07-19, 18:40, Pierre-Louis Bossart wrote: > Not all platforms support runtime_pm for now, let's use runtime_pm > only when enabled. We discussed this with Ulf sometime back and it was a consensus the core should handle it, but that may take a while. So that led me to explore what others do notably ASoC, based on this I feel we should not check the error code. We handle the non streaming case here but streaming is handled in ASoC which doesnt check the return Pierre, can you verify the below patch and let me know if that is fine for Intel platforms --- >8 --- From: Vinod Koul <vkoul@kernel.org> Date: Fri, 2 Aug 2019 22:15:11 +0530 Subject: [PATCH] soundwire: dont check return of pm_runtime_get_sync() Soundwire core checks pm_runtime_get_sync() return. But in case the driver has not enabled runtime pm we get an error. To fix this, dont check the return. We handle the non streaming case in framework but streaming case has similar handling in ASoC so make it same across use cases Signed-off-by: Vinod Koul <vkoul@kernel.org> --- drivers/soundwire/bus.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index fe745830a261..9cdf7e9e0852 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -326,9 +326,7 @@ 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; + pm_runtime_get_sync(slave->bus->dev); ret = sdw_transfer(slave->bus, &msg); pm_runtime_put(slave->bus->dev); @@ -354,9 +352,7 @@ 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; + pm_runtime_get_sync(slave->bus->dev); ret = sdw_transfer(slave->bus, &msg); pm_runtime_put(slave->bus->dev);
On 8/2/19 11:58 AM, Vinod Koul wrote: > On 25-07-19, 18:40, Pierre-Louis Bossart wrote: >> Not all platforms support runtime_pm for now, let's use runtime_pm >> only when enabled. > > We discussed this with Ulf sometime back and it was a consensus the core > should handle it, but that may take a while. > > So that led me to explore what others do notably ASoC, based on this I > feel we should not check the error code. We handle the non streaming > case here but streaming is handled in ASoC which doesnt check the return > > Pierre, can you verify the below patch and let me know if that is fine > for Intel platforms So if for some reason we cannot resume, then we'd still initiate a transaction and have even more issues to sort out. Fail big and fail early would really be my preference. Also the user of this function is largely the Slave driver, which typically doesn't do any streaming operation but controls the imp-def registers. The bus driver will only use this routine for standard registers and that's a very small part of the total traffic. > > --- >8 --- > > From: Vinod Koul <vkoul@kernel.org> > Date: Fri, 2 Aug 2019 22:15:11 +0530 > Subject: [PATCH] soundwire: dont check return of pm_runtime_get_sync() > > Soundwire core checks pm_runtime_get_sync() return. But in case the > driver has not enabled runtime pm we get an error. > > To fix this, dont check the return. We handle the non streaming case in > framework but streaming case has similar handling in ASoC so make it > same across use cases > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > --- > drivers/soundwire/bus.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c > index fe745830a261..9cdf7e9e0852 100644 > --- a/drivers/soundwire/bus.c > +++ b/drivers/soundwire/bus.c > @@ -326,9 +326,7 @@ 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; > + pm_runtime_get_sync(slave->bus->dev); > > ret = sdw_transfer(slave->bus, &msg); > pm_runtime_put(slave->bus->dev); > @@ -354,9 +352,7 @@ 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; > + pm_runtime_get_sync(slave->bus->dev); > > ret = sdw_transfer(slave->bus, &msg); > pm_runtime_put(slave->bus->dev); >
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 5ad4109dc72f..0a45dc5713df 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -332,12 +332,16 @@ 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); + + if (pm_runtime_enabled(slave->bus->dev)) + pm_runtime_put(slave->bus->dev); return ret; } @@ -359,13 +363,16 @@ int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) slave->dev_num, SDW_MSG_FLAG_WRITE, 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); + + if (pm_runtime_enabled(slave->bus->dev)) + pm_runtime_put(slave->bus->dev); return ret; }
Not all platforms support runtime_pm for now, let's use runtime_pm only when enabled. Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- drivers/soundwire/bus.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)