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

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

[Christophe JAILLET]

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@wanadoo.fr>
---
 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;
 	} else {
 		ret = regulator_enable(dvb_spi->vcc_supply);
 		if (ret)
-			goto fail_adapter;
+			goto fail_regulator;
 	}
 
 	dvb_spi->spi = spi;
@@ -618,6 +618,9 @@ cxd2880_spi_probe(struct spi_device *spi)
 fail_attach:
 	dvb_unregister_adapter(&dvb_spi->adapter);
 fail_adapter:
+	if (!IS_ERR(dvb_spi->vcc_supply))
+		regulator_disable(dvb_spi->vcc_supply);
+fail_regulator:
 	kfree(dvb_spi);
 	return ret;
 }
-- 
2.30.2

[PATCH 2/2] media: cxd2880-spi: Fix some error messages

[Christophe JAILLET]

Fix some erroneous function names in some error messages.
Remove some spurious or useless trailing and leading character in some
messages.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/media/spi/cxd2880-spi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/spi/cxd2880-spi.c b/drivers/media/spi/cxd2880-spi.c
index df1335e7061c..88aef9333ad8 100644
--- a/drivers/media/spi/cxd2880-spi.c
+++ b/drivers/media/spi/cxd2880-spi.c
@@ -147,7 +147,7 @@ static int cxd2880_spi_read_ts(struct spi_device *spi,
 
 	ret = spi_sync(spi, &message);
 	if (ret)
-		pr_err("spi_write_then_read failed\n");
+		pr_err("spi_sync failed\n");
 
 	return ret;
 }
@@ -401,7 +401,7 @@ static int cxd2880_start_feed(struct dvb_demux_feed *feed)
 							      dvb_spi,
 							      "cxd2880_ts_read");
 		if (IS_ERR(dvb_spi->cxd2880_ts_read_thread)) {
-			pr_err("kthread_run failed/\n");
+			pr_err("kthread_run failed\n");
 			kfree(dvb_spi->ts_buf);
 			dvb_spi->ts_buf = NULL;
 			memset(&dvb_spi->filter_config, 0,
@@ -448,7 +448,7 @@ static int cxd2880_stop_feed(struct dvb_demux_feed *feed)
 		 * in dvb_spi->all_pid_feed_count.
 		 */
 		if (dvb_spi->all_pid_feed_count <= 0) {
-			pr_err("PID %d not found.\n", feed->pid);
+			pr_err("PID %d not found\n", feed->pid);
 			return -EINVAL;
 		}
 		dvb_spi->all_pid_feed_count--;
@@ -485,7 +485,7 @@ static int cxd2880_stop_feed(struct dvb_demux_feed *feed)
 
 		ret_stop = kthread_stop(dvb_spi->cxd2880_ts_read_thread);
 		if (ret_stop) {
-			pr_err("'kthread_stop failed. (%d)\n", ret_stop);
+			pr_err("kthread_stop failed. (%d)\n", ret_stop);
 			ret = ret_stop;
 		}
 		kfree(dvb_spi->ts_buf);
@@ -512,7 +512,7 @@ cxd2880_spi_probe(struct spi_device *spi)
 	struct cxd2880_config config;
 
 	if (!spi) {
-		pr_err("invalid arg.\n");
+		pr_err("invalid arg\n");
 		return -EINVAL;
 	}
 
@@ -596,7 +596,7 @@ cxd2880_spi_probe(struct spi_device *spi)
 	ret = dvb_spi->demux.dmx.connect_frontend(&dvb_spi->demux.dmx,
 						  &dvb_spi->dmx_fe);
 	if (ret < 0) {
-		pr_err("dvb_register_frontend() failed\n");
+		pr_err("connect_frontend() failed\n");
 		goto fail_fe_conn;
 	}
 
-- 
2.30.2

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

[Sean Young]

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@wanadoo.fr>
> ---
>  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.

>  	} else {
>  		ret = regulator_enable(dvb_spi->vcc_supply);
>  		if (ret)
> -			goto fail_adapter;
> +			goto fail_regulator;
>  	}
>  
>  	dvb_spi->spi = spi;
> @@ -618,6 +618,9 @@ cxd2880_spi_probe(struct spi_device *spi)
>  fail_attach:
>  	dvb_unregister_adapter(&dvb_spi->adapter);
>  fail_adapter:
> +	if (!IS_ERR(dvb_spi->vcc_supply))
> +		regulator_disable(dvb_spi->vcc_supply);

IS_ERR(NULL) -> false
regulator_disable will dereference a null pointer.


Sean

> +fail_regulator:
>  	kfree(dvb_spi);
>  	return ret;
>  }
> -- 
> 2.30.2

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@wanadoo.fr>
> > ---
> >  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