Re: [PATCH 1/2] media: cxd2880-spi: Fix an error handling path

[Dan Carpenter]

On Mon, May 31, 2021 at 11:17:25AM +0100, Sean Young wrote:
> On Fri, May 21, 2021 at 02:18:14PM +0200, Christophe JAILLET wrote:
> > If an error occurs after a successful 'regulator_enable()' call,
> > 'regulator_disable()' must be called.
> > 
> > Fix the error handling path of the probe accordingly.
> > 
> > Fixes: cb496cd472af ("media: cxd2880-spi: Add optional vcc regulator")
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
> > ---
> >  drivers/media/spi/cxd2880-spi.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/spi/cxd2880-spi.c b/drivers/media/spi/cxd2880-spi.c
> > index 931ec0727cd3..df1335e7061c 100644
> > --- a/drivers/media/spi/cxd2880-spi.c
> > +++ b/drivers/media/spi/cxd2880-spi.c
> > @@ -524,13 +524,13 @@ cxd2880_spi_probe(struct spi_device *spi)
> >  	if (IS_ERR(dvb_spi->vcc_supply)) {
> >  		if (PTR_ERR(dvb_spi->vcc_supply) == -EPROBE_DEFER) {
> >  			ret = -EPROBE_DEFER;
> > -			goto fail_adapter;
> > +			goto fail_regulator;
> >  		}
> >  		dvb_spi->vcc_supply = NULL;
> 
> vcc_supply is set to null in this path.
> 

The regulator_enable()/disable() functions should be modified to handle
NULL pointers.

Btw, the regulator_get_optional() is a very annoying function...
Normally when there is an optional feature then it returns NULL if the
option was to have the feature disabled and error pointers if there
is an error.  Then the surrounding code is expected to check for and
work with NULL pointers.  The error handling is very simple:

	p = get_feature();
	if (IS_ERR(p))
		return PTR_ERR(p);

Or if the function doesn't make sense when the feature is disabled we
do:

	p = get_feature();
	if (IS_ERR_OR_NULL(p))
		return PTR_ERR(p);  // <-- this will return success if
				    // get_feature() returns NULL

Users want to see errors and be able to fix them.  We shouldn't just
disable stuff.

It sort of seems like the idea with regulator_get_optional() was to
return -ENODEV if the user deliberately has the feature off.  That's
how I would read the code but it doesn't really work because other
errors can also lead to -ENODEV.

I have examined seven call sites and there are four which treat
everthing except -ENODEV as a failure and three which only treat
-EPROBE_DEFER as a failure.

I don't know how to fix it at this point, without introducing a new
wrapper:

struct regulator *devm_regulator_get_optional_v2(struct device *dev,
						 const char *id)
{
	struct regulator *r;

	r = _devm_regulator_get(dev, id, OPTIONAL_GET);
	if (IS_ERR(r) == -ENODEV)
		return NULL;
	return r;
}

regards,
dan carpenter