Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 25 May 2021 22:38:45 +0100
From:   Mark Brown <broonie@kernel.org>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Phillip Potter <phil@philpotter.co.uk>
Cc:     linux-kernel@vger.kernel.org
Subject: Re: [PATCH 55/69] ASoC: rt5645: add error checking to rt5645_probe
 function
Message-ID: <YK1uZdtgffiCnUVQ@sirena.org.uk>
References: <20210503115736.2104747-1-gregkh@linuxfoundation.org>
 <20210503115736.2104747-56-gregkh@linuxfoundation.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="ub/BhbFoJ/f/pqD1"
Content-Disposition: inline
In-Reply-To: <20210503115736.2104747-56-gregkh@linuxfoundation.org>
Precedence: bulk


--ub/BhbFoJ/f/pqD1
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, May 03, 2021 at 01:57:22PM +0200, Greg Kroah-Hartman wrote:
> From: Phillip Potter <phil@philpotter.co.uk>
>=20
> Check for return value from various snd_soc_dapm_* calls, as many of
> them can return errors and this should be handled. Also, reintroduce
> the allocation failure check for rt5645->eq_param as well. Make all

Phillip, please follow the standard patch submission process,
this is documented in submitting-paches.rst in the kernel tree.
In particular please make sure that you copy the relevant
maintainers and mailing lists for the subsystem and any driver
specific maintainers on any patches that you are submitting to
the kernel so that they can be reviewed.

> +exit:
> +	/*
> +	 * If there was an error above, everything will be cleaned up by the
> +	 * caller if we return an error here.  This will be done with a later
> +	 * call to rt5645_remove().
> +	 */
> +	return ret;

This comment is not accurate, rt5645_remove() just resets the
hardware - it's not going to clean up anything to do with any of
the branches to error you've got above.  The core *will* clean up
any routes and widgets that are added, but it doesn't do it by
calling remove() and people shouldn't add code in their remove
functions which does so.

Also I'm guessing this was done purely through inspection rather
than the code having been tested?  If there was a problem seen at
runtime this isn't fixing it, TBH I'm more than a little dubious
about applying this untested - it's really random if things check
these errors since they're basically static checks that we're not
smart enough to do at compile time and the core is pretty loud
when they hit.  I occasionally wonder about just removing the
return codes, I think more callers don't have the checks than do
(certainly in the case of _force_enable() where I was surprised
to find any callers that do), but never got round to it.

--ub/BhbFoJ/f/pqD1
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmCtbl4ACgkQJNaLcl1U
h9DQWwf/TT6ek/EkuiVj1ZSYmXqZLo3Fb+SkoAfG/H/BZjtZdcAJNjY5nIxfVOrQ
h5Od36YMlk1JKsLIppyOjVPndqJPhijEAxSAc4PGpijOr6Dk7kj2pNMdNxd7fOYt
PeoD8F9126zalNOwAGA9ar2eZgjf2aKID3xHq2Xu3QHne3/wNdzRFuFJdHk5Ss8q
5DAZ8jkprER+kUhKPhIUvmznADnuShcoqAUVCDqp2QZ52kRctQSlQbvoAjppYfs1
DIyra0vSP3LK7jWPjlnlhymI8X8KGiXUKBx4Dmy7BX5iWF4nRLq3KB776iup6S9Z
9P5ClPa59xTOq12WfmgRF9FlrQkcoA==
=9ZiU
-----END PGP SIGNATURE-----

--ub/BhbFoJ/f/pqD1--