| Message ID | 20201109135543.7862-1-yung-chuan.liao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] ASoC: max98373: don't access volatile registers in bias level off | expand |
Hi, On Mon, 9 Nov 2020, Bard Liao wrote: > Besides, it doesn't make sense to read feedback data when codec is not > active. To avoid the regmap_read error, this patch try to return a fake > value when kcontrol _get is called in suspend. > However, the question is what is the "correct" behavior when we try to > read a volatile register when cache only is set. > 1. return an error code to signal codec is not available as what we have > now. > 2. return a fake value like what this patch do. > 3. wake-up the codec and always return a valid value. any thoughts on this anyone? This seems like a generic concern w.r.t. ASoC codec drivers and behavior of volatile registers exposed as kcontrols, and what happens when codec is in suspend. Currently regmap read will return -EBUSY in this case (case 1 above). I personally think this is still the best option. It's a bit ugly as there could be other reasons for -EBUSY as well, but at least user-space won't silently read invalid values. Waking up the codec (3) could work as well, but should this be done in all codec drivers that have volatile regs in regmap? Again, user-space would get consistent results, with the expense of extra/additional codec wakeups. Br, Kai
On Mon, Nov 09, 2020 at 09:55:43PM +0800, Bard Liao wrote: > We will set regcache_cache_only true in suspend. As a result, regmap_read > will return error when we try to read volatile registers in suspend. > Besides, it doesn't make sense to read feedback data when codec is not > active. To avoid the regmap_read error, this patch try to return a fake > value when kcontrol _get is called in suspend. > However, the question is what is the "correct" behavior when we try to > read a volatile register when cache only is set. > 1. return an error code to signal codec is not available as what we have > now. > 2. return a fake value like what this patch do. > 3. wake-up the codec and always return a valid value. This depends a bit on what the value is, if a value obtained by waking the device up is likely to be useful and what userspace is likely to do if it gets an error. > -SOC_SINGLE("ADC PVDD", MAX98373_R2054_MEAS_ADC_PVDD_CH_READBACK, 0, 0xFF, 0), > -SOC_SINGLE("ADC TEMP", MAX98373_R2055_MEAS_ADC_THERM_CH_READBACK, 0, 0xFF, 0), For things like voltage and temperature it seems like if we return zero that's not much different from a userspace point of view than returning an error, they're both clearly bad values so if userspace is doing any kind of error checking based on looking at the values it's likely to get upset by unexpected values - a clear indication that it was the read that failed might be better than bogus data, stops someone wondering why there's no power or the device is suddenly freezing. Or if it's important that we get a value for monitoring purposes then waking the device up and reading a value might make more sense though it'd burn power if done by some random ALSA UI on a regular basis which wouldn't be desirable either, it'd be relatively slow too. Another option might be for the driver to cache the last value it got when powering down, that way it can return something "sensible" even if it's not up to date. TBH I'd consider moving these to hwmon, it's possibly a bit more idiomatic to have these there than in the ALSA API where things do stuff like go through and read all the controls I think? Not sure that it'd be that much happier with values that are only intermittently readable though so the underlying problem remains.
On 19-11-20, 16:58, Mark Brown wrote: > On Mon, Nov 09, 2020 at 09:55:43PM +0800, Bard Liao wrote: > > We will set regcache_cache_only true in suspend. As a result, regmap_read > > will return error when we try to read volatile registers in suspend. > > Besides, it doesn't make sense to read feedback data when codec is not > > active. To avoid the regmap_read error, this patch try to return a fake > > value when kcontrol _get is called in suspend. > > > However, the question is what is the "correct" behavior when we try to > > read a volatile register when cache only is set. > > 1. return an error code to signal codec is not available as what we have > > now. > > 2. return a fake value like what this patch do. > > 3. wake-up the codec and always return a valid value. > > This depends a bit on what the value is, if a value obtained by waking > the device up is likely to be useful and what userspace is likely to > do if it gets an error. > > > -SOC_SINGLE("ADC PVDD", MAX98373_R2054_MEAS_ADC_PVDD_CH_READBACK, 0, 0xFF, 0), > > -SOC_SINGLE("ADC TEMP", MAX98373_R2055_MEAS_ADC_THERM_CH_READBACK, 0, 0xFF, 0), > > For things like voltage and temperature it seems like if we return zero > that's not much different from a userspace point of view than returning > an error, they're both clearly bad values so if userspace is doing any > kind of error checking based on looking at the values it's likely to get > upset by unexpected values - a clear indication that it was the read > that failed might be better than bogus data, stops someone wondering why > there's no power or the device is suddenly freezing. Or if it's > important that we get a value for monitoring purposes then waking the > device up and reading a value might make more sense though it'd burn > power if done by some random ALSA UI on a regular basis which wouldn't > be desirable either, it'd be relatively slow too. > > Another option might be for the driver to cache the last value it got > when powering down, that way it can return something "sensible" even if > it's not up to date. That would be better IMO. Also, do consider the nature of data, the monitoring data should be useful only when audio is active, not sure if anyone would care to look at this data when codec is suspended... > TBH I'd consider moving these to hwmon, it's possibly a bit more > idiomatic to have these there than in the ALSA API where things do stuff > like go through and read all the controls I think? Not sure that it'd > be that much happier with values that are only intermittently readable > though so the underlying problem remains.
> -----Original Message----- > From: Vinod Koul <vkoul@kernel.org> > Sent: Friday, November 20, 2020 1:59 PM > To: Mark Brown <broonie@kernel.org> > Cc: Bard Liao <yung-chuan.liao@linux.intel.com>; tiwai@suse.de; alsa- > devel@alsa-project.org; pierre-louis.bossart@linux.intel.com; > ryans.lee@maximintegrated.com; kai.vehmanen@linux.intel.com; Liao, Bard > <bard.liao@intel.com> > Subject: Re: [RFC] ASoC: max98373: don't access volatile registers in bias level > off > > On 19-11-20, 16:58, Mark Brown wrote: > > On Mon, Nov 09, 2020 at 09:55:43PM +0800, Bard Liao wrote: > > > We will set regcache_cache_only true in suspend. As a result, > > > regmap_read will return error when we try to read volatile registers in > suspend. > > > Besides, it doesn't make sense to read feedback data when codec is > > > not active. To avoid the regmap_read error, this patch try to return > > > a fake value when kcontrol _get is called in suspend. > > > > > However, the question is what is the "correct" behavior when we try > > > to read a volatile register when cache only is set. > > > 1. return an error code to signal codec is not available as what we > > > have now. > > > 2. return a fake value like what this patch do. > > > 3. wake-up the codec and always return a valid value. > > > > This depends a bit on what the value is, if a value obtained by waking > > the device up is likely to be useful and what userspace is likely to > > do if it gets an error. > > > > > -SOC_SINGLE("ADC PVDD", > MAX98373_R2054_MEAS_ADC_PVDD_CH_READBACK, 0, > > > 0xFF, 0), -SOC_SINGLE("ADC TEMP", > > > MAX98373_R2055_MEAS_ADC_THERM_CH_READBACK, 0, 0xFF, 0), > > > > For things like voltage and temperature it seems like if we return > > zero that's not much different from a userspace point of view than > > returning an error, they're both clearly bad values so if userspace is > > doing any kind of error checking based on looking at the values it's > > likely to get upset by unexpected values - a clear indication that it > > was the read that failed might be better than bogus data, stops > > someone wondering why there's no power or the device is suddenly > > freezing. Or if it's important that we get a value for monitoring > > purposes then waking the device up and reading a value might make more > > sense though it'd burn power if done by some random ALSA UI on a > > regular basis which wouldn't be desirable either, it'd be relatively slow too. > > > > Another option might be for the driver to cache the last value it got > > when powering down, that way it can return something "sensible" even > > if it's not up to date. > > That would be better IMO. Also, do consider the nature of data, the > monitoring data should be useful only when audio is active, not sure if > anyone would care to look at this data when codec is suspended... Thanks Mark and Vinod Obviously, return the last value is better then zero. > > > TBH I'd consider moving these to hwmon, it's possibly a bit more > > idiomatic to have these there than in the ALSA API where things do > > stuff like go through and read all the controls I think? Not sure > > that it'd be that much happier with values that are only > > intermittently readable though so the underlying problem remains. Indeed, the issue was reported when someone went through and read all the controls. People are not happy to see errors. Moving it to hwmon sounds reasonable to me, too. But it depends on how userspace works. I don't know how userspace uses those controls or if userspace needs those controls. @Ryan what is your idea? > > > > -- > ~Vinod
On Mon, 9 Nov 2020 21:55:43 +0800, Bard Liao wrote: > We will set regcache_cache_only true in suspend. As a result, regmap_read > will return error when we try to read volatile registers in suspend. > Besides, it doesn't make sense to read feedback data when codec is not > active. To avoid the regmap_read error, this patch try to return a fake > value when kcontrol _get is called in suspend. > However, the question is what is the "correct" behavior when we try to > read a volatile register when cache only is set. > 1. return an error code to signal codec is not available as what we have > now. > 2. return a fake value like what this patch do. > 3. wake-up the codec and always return a valid value. Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: max98373: don't access volatile registers in bias level off (no commit info) All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
> -----Original Message----- > From: Mark Brown <broonie@kernel.org> > Sent: Tuesday, December 1, 2020 9:58 PM > To: Bard Liao <yung-chuan.liao@linux.intel.com>; tiwai@suse.de > Cc: pierre-louis.bossart@linux.intel.com; vkoul@kernel.org; Liao, Bard > <bard.liao@intel.com>; kai.vehmanen@linux.intel.com; > ryans.lee@maximintegrated.com; alsa-devel@alsa-project.org > Subject: Re: [RFC] ASoC: max98373: don't access volatile registers in bias level > off > > On Mon, 9 Nov 2020 21:55:43 +0800, Bard Liao wrote: > > We will set regcache_cache_only true in suspend. As a result, > > regmap_read will return error when we try to read volatile registers in > suspend. > > Besides, it doesn't make sense to read feedback data when codec is not > > active. To avoid the regmap_read error, this patch try to return a > > fake value when kcontrol _get is called in suspend. > > However, the question is what is the "correct" behavior when we try to > > read a volatile register when cache only is set. > > 1. return an error code to signal codec is not available as what we > > have now. > > 2. return a fake value like what this patch do. > > 3. wake-up the codec and always return a valid value. > > Applied to > > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next > > Thanks! > > [1/1] ASoC: max98373: don't access volatile registers in bias level off > (no commit info) > Hi Mark, Sorry but I don't find the patch on your tree. Is it applied? Does "no commit info" mean the commit doesn't apply? Thanks, Bard
On Thu, Dec 10, 2020 at 06:18:45AM +0000, Liao, Bard wrote: > > [1/1] ASoC: max98373: don't access volatile registers in bias level off > > (no commit info) > Sorry but I don't find the patch on your tree. Is it applied? > Does "no commit info" mean the commit doesn't apply? Yes, this was a b4 bug and it wasn't applied.
diff --git a/sound/soc/codecs/max98373.c b/sound/soc/codecs/max98373.c index 929bb1798c43..07494e40c296 100644 --- a/sound/soc/codecs/max98373.c +++ b/sound/soc/codecs/max98373.c @@ -168,6 +168,23 @@ static SOC_ENUM_SINGLE_DECL(max98373_adc_samplerate_enum, MAX98373_R2051_MEAS_ADC_SAMPLING_RATE, 0, max98373_ADC_samplerate_text); +static int max98373_feedback_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_component *component = snd_kcontrol_chip(kcontrol); + + if (snd_soc_component_get_bias_level(component) == SND_SOC_BIAS_OFF) { + /* + * We can't get the feedback data when codec is off, set + * value = 0 and return 0 to avoid error. + */ + ucontrol->value.integer.value[0] = 0; + return 0; + } + + return snd_soc_put_volsw(kcontrol, ucontrol); +} + static const struct snd_kcontrol_new max98373_snd_controls[] = { SOC_SINGLE("Digital Vol Sel Switch", MAX98373_R203F_AMP_DSP_CFG, MAX98373_AMP_VOL_SEL_SHIFT, 1, 0), @@ -209,8 +226,10 @@ SOC_SINGLE("ADC PVDD FLT Switch", MAX98373_R2052_MEAS_ADC_PVDD_FLT_CFG, MAX98373_FLT_EN_SHIFT, 1, 0), SOC_SINGLE("ADC TEMP FLT Switch", MAX98373_R2053_MEAS_ADC_THERM_FLT_CFG, MAX98373_FLT_EN_SHIFT, 1, 0), -SOC_SINGLE("ADC PVDD", MAX98373_R2054_MEAS_ADC_PVDD_CH_READBACK, 0, 0xFF, 0), -SOC_SINGLE("ADC TEMP", MAX98373_R2055_MEAS_ADC_THERM_CH_READBACK, 0, 0xFF, 0), +SOC_SINGLE_EXT("ADC PVDD", MAX98373_R2054_MEAS_ADC_PVDD_CH_READBACK, 0, 0xFF, 0, + max98373_feedback_get, NULL), +SOC_SINGLE_EXT("ADC TEMP", MAX98373_R2055_MEAS_ADC_THERM_CH_READBACK, 0, 0xFF, 0, + max98373_feedback_get, NULL), SOC_SINGLE("ADC PVDD FLT Coeff", MAX98373_R2052_MEAS_ADC_PVDD_FLT_CFG, 0, 0x3, 0), SOC_SINGLE("ADC TEMP FLT Coeff", MAX98373_R2053_MEAS_ADC_THERM_FLT_CFG, @@ -226,7 +245,8 @@ SOC_SINGLE("BDE LVL1 Thresh", MAX98373_R2097_BDE_L1_THRESH, 0, 0xFF, 0), SOC_SINGLE("BDE LVL2 Thresh", MAX98373_R2098_BDE_L2_THRESH, 0, 0xFF, 0), SOC_SINGLE("BDE LVL3 Thresh", MAX98373_R2099_BDE_L3_THRESH, 0, 0xFF, 0), SOC_SINGLE("BDE LVL4 Thresh", MAX98373_R209A_BDE_L4_THRESH, 0, 0xFF, 0), -SOC_SINGLE("BDE Active Level", MAX98373_R20B6_BDE_CUR_STATE_READBACK, 0, 8, 0), +SOC_SINGLE_EXT("BDE Active Level", MAX98373_R20B6_BDE_CUR_STATE_READBACK, 0, 8, 0, + max98373_feedback_get, NULL), SOC_SINGLE("BDE Clip Mode Switch", MAX98373_R2092_BDE_CLIPPER_MODE, 0, 1, 0), SOC_SINGLE("BDE Thresh Hysteresis", MAX98373_R209B_BDE_THRESH_HYST, 0, 0xFF, 0), SOC_SINGLE("BDE Hold Time", MAX98373_R2090_BDE_LVL_HOLD, 0, 0xFF, 0),
We will set regcache_cache_only true in suspend. As a result, regmap_read will return error when we try to read volatile registers in suspend. Besides, it doesn't make sense to read feedback data when codec is not active. To avoid the regmap_read error, this patch try to return a fake value when kcontrol _get is called in suspend. However, the question is what is the "correct" behavior when we try to read a volatile register when cache only is set. 1. return an error code to signal codec is not available as what we have now. 2. return a fake value like what this patch do. 3. wake-up the codec and always return a valid value. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> --- sound/soc/codecs/max98373.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)