Re: [GIT PULL] sound updates for 5.9

From: Mark Brown <broonie@kernel.org>
To: Takashi Iwai <tiwai@suse.de>
Cc: John Stultz <john.stultz@linaro.org>,
	Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
	Srini Kandagatla <srinivas.kandagatla@linaro.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Amit Pundir <amit.pundir@linaro.org>,
	Vinod Koul <vkoul@kernel.org>
Subject: Re: [GIT PULL] sound updates for 5.9
Date: Mon, 10 Aug 2020 13:22:54 +0100	[thread overview]
Message-ID: <20200810122254.GA6438@sirena.org.uk> (raw)
In-Reply-To: <s5ho8nl7e7r.wl-tiwai@suse.de>

[-- Attachment #1: Type: text/plain, Size: 1828 bytes --]

On Sat, Aug 08, 2020 at 10:07:36AM +0200, Takashi Iwai wrote:
> Takashi Iwai wrote:

> > 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.

I did an audit of the users when the series was posted and wasn't able
to turn up any code doing anything constructive with the return values,
but then once you're past probe error handling often makes things worse
if you try.  This is the first one which actually seems to have had an
impact.

> 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.

I'm not sure if that specifically is what we need but yeah we should do
something, if it fixes things your change is certainly good for the
immediate problem so could you send it with a signoff please?  

With the new code we do now have the core code printing an error message
if the I/O fails, before they were just being ignored more often than
not.  This did turn up a couple of cases where drivers were relying on
being able to do things like silently read from registers that just
don't exist or aren't currently accessible without any diagnostics which
is it's own problem :/ (especially the volatile cases).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]