[PATCH] iio: magnetometer: ak8974: Fix the error handling of ak8974_probe()

[PATCH] iio: magnetometer: ak8974: Fix the error handling of ak8974_probe()

[Zheyu Ma]

When the driver fail at devm_regmap_init_i2c(), we will get the
following splat:

[  106.797388] WARNING: CPU: 4 PID: 413 at drivers/regulator/core.c:2257 _regulator_put+0x3ec/0x4e0
[  106.802183] RIP: 0010:_regulator_put+0x3ec/0x4e0
[  106.811237] Call Trace:
[  106.811515]  <TASK>
[  106.811695]  regulator_bulk_free+0x82/0xe0
[  106.812032]  devres_release_group+0x319/0x3d0
[  106.812425]  i2c_device_probe+0x766/0x940

Fix this by disabling the regulators at the error path.

Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
---
 drivers/iio/magnetometer/ak8974.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
index e54feacfb980..84bbf7ccc887 100644
--- a/drivers/iio/magnetometer/ak8974.c
+++ b/drivers/iio/magnetometer/ak8974.c
@@ -862,6 +862,7 @@ static int ak8974_probe(struct i2c_client *i2c,
 		dev_err(&i2c->dev, "failed to allocate register map\n");
 		pm_runtime_put_noidle(&i2c->dev);
 		pm_runtime_disable(&i2c->dev);
+		regulator_bulk_disable(ARRAY_SIZE(ak8974->regs), ak8974->regs);
 		return PTR_ERR(ak8974->map);
 	}
 
-- 
2.25.1

[PATCH] iio: magnetometer: ak8975: Fix the error handling in ak8975_power_on()

[Zheyu Ma]

When the driver fails to enable the regulator 'vid', we will get the
following splat:

[   79.955610] WARNING: CPU: 5 PID: 441 at drivers/regulator/core.c:2257 _regulator_put+0x3ec/0x4e0
[   79.959641] RIP: 0010:_regulator_put+0x3ec/0x4e0
[   79.967570] Call Trace:
[   79.967773]  <TASK>
[   79.967951]  regulator_put+0x1f/0x30
[   79.968254]  devres_release_group+0x319/0x3d0
[   79.968608]  i2c_device_probe+0x766/0x940

Fix this by disabling the 'vdd' regulator when failing to enable 'vid'
regulator.

Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
---
 drivers/iio/magnetometer/ak8975.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 088f748b683e..2432e697150c 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -416,6 +416,7 @@ static int ak8975_power_on(const struct ak8975_data *data)
 	if (ret) {
 		dev_warn(&data->client->dev,
 			 "Failed to enable specified Vid supply\n");
+		regulator_disable(data->vdd);
 		return ret;
 	}
 
-- 
2.25.1

Re: [PATCH] iio: magnetometer: ak8974: Fix the error handling of ak8974_probe()

[Jonathan Cameron]

On Sat,  9 Apr 2022 11:48:48 +0800
Zheyu Ma <zheyuma97@gmail.com> wrote:

> When the driver fail at devm_regmap_init_i2c(), we will get the
> following splat:
> 
> [  106.797388] WARNING: CPU: 4 PID: 413 at drivers/regulator/core.c:2257 _regulator_put+0x3ec/0x4e0
> [  106.802183] RIP: 0010:_regulator_put+0x3ec/0x4e0
> [  106.811237] Call Trace:
> [  106.811515]  <TASK>
> [  106.811695]  regulator_bulk_free+0x82/0xe0
> [  106.812032]  devres_release_group+0x319/0x3d0
> [  106.812425]  i2c_device_probe+0x766/0x940
> 
> Fix this by disabling the regulators at the error path.
> 
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
+ CC Linus W as it's his driver.

Fix looks correct to me, though the handling of runtime pm in here is
probably more complex than it needs to be (and hence this odd error
handling for this one place in the probe).

Jonathan

> ---
>  drivers/iio/magnetometer/ak8974.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
> index e54feacfb980..84bbf7ccc887 100644
> --- a/drivers/iio/magnetometer/ak8974.c
> +++ b/drivers/iio/magnetometer/ak8974.c
> @@ -862,6 +862,7 @@ static int ak8974_probe(struct i2c_client *i2c,
>  		dev_err(&i2c->dev, "failed to allocate register map\n");
>  		pm_runtime_put_noidle(&i2c->dev);
>  		pm_runtime_disable(&i2c->dev);
> +		regulator_bulk_disable(ARRAY_SIZE(ak8974->regs), ak8974->regs);
>  		return PTR_ERR(ak8974->map);
>  	}
>  

Re: [PATCH] iio: magnetometer: ak8975: Fix the error handling in ak8975_power_on()

[Jonathan Cameron]

On Sat,  9 Apr 2022 11:48:49 +0800
Zheyu Ma <zheyuma97@gmail.com> wrote:

> When the driver fails to enable the regulator 'vid', we will get the
> following splat:
> 
> [   79.955610] WARNING: CPU: 5 PID: 441 at drivers/regulator/core.c:2257 _regulator_put+0x3ec/0x4e0
> [   79.959641] RIP: 0010:_regulator_put+0x3ec/0x4e0
> [   79.967570] Call Trace:
> [   79.967773]  <TASK>
> [   79.967951]  regulator_put+0x1f/0x30
> [   79.968254]  devres_release_group+0x319/0x3d0
> [   79.968608]  i2c_device_probe+0x766/0x940
> 
> Fix this by disabling the 'vdd' regulator when failing to enable 'vid'
> regulator.
> 
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
This driver doesn't really have a maintainer any more, though Linus W actually
last touched this bit of code. So +CC.

However, it's pretty obviously correct so applied to the fixes-togreg branch of iio.git.

For future reference, please don't send one fix as reply to a fix on a different driver.
Makes a mess of thread handling and tracking in patchwork etc.

Ideally also please provide a fixes tag.  For this one I have applied with out it
because it was a long time back and looks like the bug predates a bunch of refactoring
of this code.  

Applied to the fixes-togreg branch of iio.git and marked for stable.

Jonathan


> ---
>  drivers/iio/magnetometer/ak8975.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 088f748b683e..2432e697150c 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -416,6 +416,7 @@ static int ak8975_power_on(const struct ak8975_data *data)
>  	if (ret) {
>  		dev_warn(&data->client->dev,
>  			 "Failed to enable specified Vid supply\n");
> +		regulator_disable(data->vdd);
>  		return ret;
>  	}
>  

Re: [PATCH] iio: magnetometer: ak8975: Fix the error handling in ak8975_power_on()

[Zheyu Ma]

On Sun, Apr 10, 2022 at 11:53 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat,  9 Apr 2022 11:48:49 +0800
> Zheyu Ma <zheyuma97@gmail.com> wrote:
>
> > When the driver fails to enable the regulator 'vid', we will get the
> > following splat:
> >
> > [   79.955610] WARNING: CPU: 5 PID: 441 at drivers/regulator/core.c:2257 _regulator_put+0x3ec/0x4e0
> > [   79.959641] RIP: 0010:_regulator_put+0x3ec/0x4e0
> > [   79.967570] Call Trace:
> > [   79.967773]  <TASK>
> > [   79.967951]  regulator_put+0x1f/0x30
> > [   79.968254]  devres_release_group+0x319/0x3d0
> > [   79.968608]  i2c_device_probe+0x766/0x940
> >
> > Fix this by disabling the 'vdd' regulator when failing to enable 'vid'
> > regulator.
> >
> > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> This driver doesn't really have a maintainer any more, though Linus W actually
> last touched this bit of code. So +CC.
>
> However, it's pretty obviously correct so applied to the fixes-togreg branch of iio.git.
>
> For future reference, please don't send one fix as reply to a fix on a different driver.
> Makes a mess of thread handling and tracking in patchwork etc.

Sorry for the confusion, I will take care of it next time.

> Ideally also please provide a fixes tag.  For this one I have applied with out it
> because it was a long time back and looks like the bug predates a bunch of refactoring
> of this code.

Thanks for your reminder, I will add the fixes tag in the next submission.

Regards,
Zheyu Ma

Re: [PATCH] iio: magnetometer: ak8974: Fix the error handling of ak8974_probe()

[Linus Walleij]

On Sun, Apr 10, 2022 at 5:46 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Sat,  9 Apr 2022 11:48:48 +0800
> Zheyu Ma <zheyuma97@gmail.com> wrote:
>
> > When the driver fail at devm_regmap_init_i2c(), we will get the
> > following splat:
> >
> > [  106.797388] WARNING: CPU: 4 PID: 413 at drivers/regulator/core.c:2257 _regulator_put+0x3ec/0x4e0
> > [  106.802183] RIP: 0010:_regulator_put+0x3ec/0x4e0
> > [  106.811237] Call Trace:
> > [  106.811515]  <TASK>
> > [  106.811695]  regulator_bulk_free+0x82/0xe0
> > [  106.812032]  devres_release_group+0x319/0x3d0
> > [  106.812425]  i2c_device_probe+0x766/0x940
> >
> > Fix this by disabling the regulators at the error path.
> >
> > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> + CC Linus W as it's his driver.
>
> Fix looks correct to me, though the handling of runtime pm in here is
> probably more complex than it needs to be (and hence this odd error
> handling for this one place in the probe).

At the time I discussed how to do runtime pm with Ulf Hansson a lot
and I think it was the state of the art at that time. It might have
changed since.

> > ---
> >  drivers/iio/magnetometer/ak8974.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
> > index e54feacfb980..84bbf7ccc887 100644
> > --- a/drivers/iio/magnetometer/ak8974.c
> > +++ b/drivers/iio/magnetometer/ak8974.c
> > @@ -862,6 +862,7 @@ static int ak8974_probe(struct i2c_client *i2c,
> >               dev_err(&i2c->dev, "failed to allocate register map\n");
> >               pm_runtime_put_noidle(&i2c->dev);
> >               pm_runtime_disable(&i2c->dev);
> > +             regulator_bulk_disable(ARRAY_SIZE(ak8974->regs), ak8974->regs);

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH] iio: magnetometer: ak8974: Fix the error handling of ak8974_probe()

[Ulf Hansson]

On Thu, 21 Apr 2022 at 01:14, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Sun, Apr 10, 2022 at 5:46 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Sat,  9 Apr 2022 11:48:48 +0800
> > Zheyu Ma <zheyuma97@gmail.com> wrote:
> >
> > > When the driver fail at devm_regmap_init_i2c(), we will get the
> > > following splat:
> > >
> > > [  106.797388] WARNING: CPU: 4 PID: 413 at drivers/regulator/core.c:2257 _regulator_put+0x3ec/0x4e0
> > > [  106.802183] RIP: 0010:_regulator_put+0x3ec/0x4e0
> > > [  106.811237] Call Trace:
> > > [  106.811515]  <TASK>
> > > [  106.811695]  regulator_bulk_free+0x82/0xe0
> > > [  106.812032]  devres_release_group+0x319/0x3d0
> > > [  106.812425]  i2c_device_probe+0x766/0x940
> > >
> > > Fix this by disabling the regulators at the error path.
> > >
> > > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> > + CC Linus W as it's his driver.
> >
> > Fix looks correct to me, though the handling of runtime pm in here is
> > probably more complex than it needs to be (and hence this odd error
> > handling for this one place in the probe).
>
> At the time I discussed how to do runtime pm with Ulf Hansson a lot
> and I think it was the state of the art at that time. It might have
> changed since.

It still looks good to me. One could switch the order of the calls to
pm_runtime_disable() and pm_runtime_put_noidle(), but it's not a big
thing.

Whether it looks complicated or not, that's a different story. :-)

Note that some drivers are always being built with CONFIG_PM being
set, which allows some simplifications. Although, I don't think that's
the case here, right?

>
> > > ---
> > >  drivers/iio/magnetometer/ak8974.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
> > > index e54feacfb980..84bbf7ccc887 100644
> > > --- a/drivers/iio/magnetometer/ak8974.c
> > > +++ b/drivers/iio/magnetometer/ak8974.c
> > > @@ -862,6 +862,7 @@ static int ak8974_probe(struct i2c_client *i2c,
> > >               dev_err(&i2c->dev, "failed to allocate register map\n");
> > >               pm_runtime_put_noidle(&i2c->dev);
> > >               pm_runtime_disable(&i2c->dev);
> > > +             regulator_bulk_disable(ARRAY_SIZE(ak8974->regs), ak8974->regs);
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Yours,
> Linus Walleij

Kind regards
Uffe