Messages in this thread | | | Date | Tue, 25 May 2021 23:02:16 +0100 | From | Phillip Potter <> | Subject | Re: [PATCH 55/69] ASoC: rt5645: add error checking to rt5645_probe function |
| |
On Tue, May 25, 2021 at 10:38:45PM +0100, Mark Brown wrote: > 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. >
Dear Mark,
This patch was submitted to a closed mentoring group as part of the University of Minnesota reversion/checking process. I was not responsible for the final send out to the public mailing lists etc. as the patches were collated first and sent out together en masse.
> > +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.
My comment was adjusted after submission for brevity's sake. This was what I originally wrote: /* * All of the above is cleaned up when we return an error here, as * the caller will notice the error and invoke rt5645_remove and * other cleanup routines, as it does for the snd_soc_dapm_* calls * above as well. */ Happy to resubmit/rewrite as needed? Based on what you've written though it may be better to drop the patch?
> > 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.
Yes, that's correct - I did not test this directly other than making sure it builds, as I don't have this hardware to test with.
Regards, Phil
| |