| Message ID | 28a115d7207b258715a029f822ab88f7a4ee05e6.1621599392.git.christophe.jaillet@wanadoo.fr |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] media: cxd2880-spi: Fix an error handling path | expand |
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
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
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; }
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(-)