| Message ID | 20200810134631.19742-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | efc913c8fb88728626759735e1b09370a6813180 |
| Headers | show |
| Series | ASoC: Make soc_component_read() returning an error code again | expand |
On Mon, Aug 10, 2020 at 6:53 AM Takashi Iwai <tiwai@suse.de> wrote: > > Along with the recent unification of snd_soc_component_read*() > functions, the behavior of snd_soc_component_read() was changed > slightly; namely it returns the register read value directly, and even > if an error happens, it returns zero (but it prints an error > message). That said, the caller side can't know whether it's an error > or not any longer. > > Ideally this shouldn't matter much, but in practice this seems causing > a regression, as John reported. And, grepping the tree revealed that > there are still plenty of callers that do check the error code, so > we'll need to deal with them in anyway. > > As a quick band-aid over the regression, this patch changes the return > value of snd_soc_component_read() again to the negative error code. > It can't work, obviously, for 32bit register values, but it should be > enough for the known regressions, so far. > > Fixes: cf6e26c71bfd ("ASoC: soc-component: merge snd_soc_component_read() and snd_soc_component_read32()") > Reported-by: John Stultz <john.stultz@linaro.org> > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > sound/soc/soc-component.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c > index f0b4f4bc44a4..5504b92946e3 100644 > --- a/sound/soc/soc-component.c > +++ b/sound/soc/soc-component.c > @@ -406,7 +406,7 @@ static unsigned int soc_component_read_no_lock( > ret = -EIO; > > if (ret < 0) > - soc_component_ret(component, ret); > + return soc_component_ret(component, ret); So oddly, using this instead of just "return ret;", doesn't seem to fully resolve the issue for me. It's baffling! My only guess is at boot up I get a *ton* of error messages: q6routing remoteproc-adsp:glink-edge:apr:apr-service@8:routing: ASoC: error at soc_component_read_no_lock on remoteproc-adsp:glink-edge:apr:5 which I suspect is causing other failures due to timing issues. Srini sent me a patch to provide dummy read functions for the routing and dai components that were missing it, and that seems to properly avoid the issue. thanks -john
On Mon, Aug 10, 2020 at 09:45:14PM -0700, John Stultz wrote: > On Mon, Aug 10, 2020 at 6:53 AM Takashi Iwai <tiwai@suse.de> wrote: > > if (ret < 0) > > - soc_component_ret(component, ret); > > + return soc_component_ret(component, ret); > So oddly, using this instead of just "return ret;", doesn't seem to > fully resolve the issue for me. It's baffling! > My only guess is at boot up I get a *ton* of error messages: > q6routing remoteproc-adsp:glink-edge:apr:apr-service@8:routing: > ASoC: error at soc_component_read_no_lock on > remoteproc-adsp:glink-edge:apr:5 > which I suspect is causing other failures due to timing issues. Seems likely. > Srini sent me a patch to provide dummy read functions for the routing > and dai components that were missing it, and that seems to properly > avoid the issue. Hopefully Srini will post that soon then.
On Mon, 10 Aug 2020 15:46:31 +0200, Takashi Iwai wrote: > Along with the recent unification of snd_soc_component_read*() > functions, the behavior of snd_soc_component_read() was changed > slightly; namely it returns the register read value directly, and even > if an error happens, it returns zero (but it prints an error > message). That said, the caller side can't know whether it's an error > or not any longer. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: Make soc_component_read() returning an error code again commit: efc913c8fb88728626759735e1b09370a6813180 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
diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c index f0b4f4bc44a4..5504b92946e3 100644 --- a/sound/soc/soc-component.c +++ b/sound/soc/soc-component.c @@ -406,7 +406,7 @@ static unsigned int soc_component_read_no_lock( ret = -EIO; if (ret < 0) - soc_component_ret(component, ret); + return soc_component_ret(component, ret); return val; }
Along with the recent unification of snd_soc_component_read*() functions, the behavior of snd_soc_component_read() was changed slightly; namely it returns the register read value directly, and even if an error happens, it returns zero (but it prints an error message). That said, the caller side can't know whether it's an error or not any longer. Ideally this shouldn't matter much, but in practice this seems causing a regression, as John reported. And, grepping the tree revealed that there are still plenty of callers that do check the error code, so we'll need to deal with them in anyway. As a quick band-aid over the regression, this patch changes the return value of snd_soc_component_read() again to the negative error code. It can't work, obviously, for 32bit register values, but it should be enough for the known regressions, so far. Fixes: cf6e26c71bfd ("ASoC: soc-component: merge snd_soc_component_read() and snd_soc_component_read32()") Reported-by: John Stultz <john.stultz@linaro.org> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/soc/soc-component.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)