Re: [GIT PULL] sound updates for 5.9

[Takashi Iwai <tiwai@suse.de>]

On Sat, 08 Aug 2020 08:46:18 +0200,
Takashi Iwai wrote:
> 
> On Sat, 08 Aug 2020 02:23:24 +0200,
> John Stultz wrote:
> > 
> > On Thu, Aug 6, 2020 at 3:33 AM Takashi Iwai <tiwai@suse.de> wrote:
> > >
> > > Linus,
> > >
> > > please pull sound updates for v5.9 from:
> > >
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git tags/sound-5.9-rc1
> > >
> > > The topmost commit is c7fabbc51352f50cc58242a6dc3b9c1a3599849b
> > >
> > > ----------------------------------------------------------------
> > >
> > > sound updates for 5.9
> > >
> > > This became wide and scattered updates all over the sound tree as
> > > diffstat shows: lots of (still ongoing) refactoring works in ASoC,
> > > fixes and cleanups caught by static analysis, inclusive term
> > > conversions as well as lots of new drivers.  Below are highlights:
> > >
> > > ASoC core:
> > > * API cleanups and conversions to the unified mute_stream() call
> > > * Simplify I/O helper functions
> > > * Use helper macros to retrieve RTD from substreams
> > ...
> > > Kuninori Morimoto (90):
> > >       ASoC: soc-component: add soc_component_pin() and share code
> > >       ASoC: soc-component: move snd_soc_component_xxx_regmap() to soc-component
> > >       ASoC: soc-component: move snd_soc_component_initialize() to soc-component.c
> > >       ASoC: soc-component: add soc_component_err()
> > >       ASoC: soc-component: add snd_soc_pcm_component_prepare()
> > >       ASoC: soc-component: add snd_soc_pcm_component_hw_params()
> > >       ASoC: soc-component: add snd_soc_pcm_component_hw_free()
> > >       ASoC: soc-component: add snd_soc_pcm_component_trigger()
> > >       ASoC: soc-component: add snd_soc_component_init()
> > >       ASoC: soc-component: merge soc-io.c into soc-component.c
> > 
> > So oddly, today I bisected down the change "ASoC: soc-component: merge
> > soc-io.c into soc-component.c":
> >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=460b42d162e3cf634586999e6a84e74ca52e626d
> > 
> > as causing audio regressions on Dragonboard 845c running AOSP.
> > 
> > On boot I was seeing tons of:
> > 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
> > 
> > And when audio was supposed to play I'd see:
> > [  227.462986] qcom-q6afe aprsvc:apr-service:4:4: cmd = 0x100e5
> > returned error = 0x9
> > [  227.470720] qcom-q6afe aprsvc:apr-service:4:4: DSP returned error[9]
> > [  227.477168] qcom-q6afe aprsvc:apr-service:4:4: AFE enable for port
> > 0x4000 failed -22
> > [  227.485038] q6afe-dai
> > remoteproc-adsp:glink-edge:apr:apr-service@4:dais: fail to start AFE
> > port 2
> > [  227.494013] q6afe-dai
> > remoteproc-adsp:glink-edge:apr:apr-service@4:dais: ASoC: error at
> > snd_soc_pcm_dai_prepare on SLIMBUS_0_RX: -22
> > [  227.506034]  SLIM Playback: ASoC: DAI prepare error: -22
> > [  227.511415]  SLIM Playback: ASoC: backend prepare failed -22
> > 
> > Its strange, as the bisected patch is really just moving code around
> > and there's very little in the way of logic changes. After minimizing
> > the code movement and just focusing on what changed I forward ported a
> > revert to mainline and minimized it until things were working.
> > 
> > The resulting patch is a twoliner here:
> > https://git.linaro.org/people/john.stultz/android-dev.git/commit/?h=dev/db845c-mainline-WIP&id=a3527193f39b1224d59bf1519fce3ef8c57d0f5e
> > 
> > I'm a bit baffled as to why this patch works. Logically we are
> > returning the same value. I suspect when we hit the error, all the
> > extra error print messages on the console slow things down and end up
> > causing some timing related initialization failure?
> 
> Does the patch below fix the bug?  If so, it's rather a bug in the
> commit cf6e26c71bfd ("ASoC: soc-component: merge
> snd_soc_component_read() and snd_soc_component_read32()").

That said, the commit cf6e26c71bfd dropped the capability of returning
an error code from snd_soc_component_read() completely, while many
code still expect an error gets returned.  The assumption mentioned in
the patch (the error can be ignored) looks too naive.

Morimoto-san, Mark, could you address it?  IMO, we may still need two
variants in the end again: the former snd_soc_component_read32() that
returns the value directly and snd_soc_component_read() that returns 0
or an error.  Only once after we deal with the error handling in each
caller side, we can unify the read functions.


Takashi

> 
> thanks,
> 
> Takashi
> 
> --- 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;
>  }