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

[Phillip Potter <phil@philpotter.co.uk>]

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