Re: [PATCH 55/69] ASoC: rt5645: add error checking to rt5645_probe function

[Mark Brown]

On Mon, May 03, 2021 at 01:57:22PM +0200, Greg Kroah-Hartman wrote:
> From: Phillip Potter <phil@xxxxxxxxxxxxxxxx>
> 
> 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.

Attachment: signature.asc
Description: PGP signature