All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] media: cxd2880-spi: Fix an error handling path
@ 2021-05-21 12:18 Christophe JAILLET
  2021-05-21 12:18 ` [PATCH 2/2] media: cxd2880-spi: Fix some error messages Christophe JAILLET
  2021-05-31 10:17 ` [PATCH 1/2] media: cxd2880-spi: Fix an error handling path Sean Young
  0 siblings, 2 replies; 4+ messages in thread
From: Christophe JAILLET @ 2021-05-21 12:18 UTC (permalink / raw)
  To: Yasunari.Takiguchi, mchehab, narmstrong, sean
  Cc: linux-media, linux-kernel, kernel-janitors, 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 2/2] media: cxd2880-spi: Fix some error messages
  2021-05-21 12:18 [PATCH 1/2] media: cxd2880-spi: Fix an error handling path Christophe JAILLET
@ 2021-05-21 12:18 ` Christophe JAILLET
  2021-05-31 10:17 ` [PATCH 1/2] media: cxd2880-spi: Fix an error handling path Sean Young
  1 sibling, 0 replies; 4+ messages in thread
From: Christophe JAILLET @ 2021-05-21 12:18 UTC (permalink / raw)
  To: Yasunari.Takiguchi, mchehab, narmstrong, sean
  Cc: linux-media, linux-kernel, kernel-janitors, 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/2] media: cxd2880-spi: Fix an error handling path
  2021-05-21 12:18 [PATCH 1/2] media: cxd2880-spi: Fix an error handling path Christophe JAILLET
  2021-05-21 12:18 ` [PATCH 2/2] media: cxd2880-spi: Fix some error messages Christophe JAILLET
@ 2021-05-31 10:17 ` Sean Young
  2021-05-31 11:57   ` Dan Carpenter
  1 sibling, 1 reply; 4+ messages in thread
From: Sean Young @ 2021-05-31 10:17 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: Yasunari.Takiguchi, mchehab, narmstrong, linux-media,
	linux-kernel, kernel-janitors

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/2] media: cxd2880-spi: Fix an error handling path
  2021-05-31 10:17 ` [PATCH 1/2] media: cxd2880-spi: Fix an error handling path Sean Young
@ 2021-05-31 11:57   ` Dan Carpenter
  0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2021-05-31 11:57 UTC (permalink / raw)
  To: Sean Young
  Cc: Christophe JAILLET, Yasunari.Takiguchi, mchehab, narmstrong,
	linux-media, linux-kernel, kernel-janitors

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-05-31 11:57 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-21 12:18 [PATCH 1/2] media: cxd2880-spi: Fix an error handling path Christophe JAILLET
2021-05-21 12:18 ` [PATCH 2/2] media: cxd2880-spi: Fix some error messages Christophe JAILLET
2021-05-31 10:17 ` [PATCH 1/2] media: cxd2880-spi: Fix an error handling path Sean Young
2021-05-31 11:57   ` Dan Carpenter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.