| Date | Tue, 25 May 2021 22:38:45 +0100 | From | Mark Brown <> | Subject | Re: [PATCH 55/69] ASoC: rt5645: add error checking to rt5645_probe function |
| |
On Mon, May 03, 2021 at 01:57:22PM +0200, Greg Kroah-Hartman wrote: > From: Phillip Potter <phil@philpotter.co.uk> > > 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. [unhandled content-type:application/pgp-signature] |