Keep using managed resources as much as possible.
Signed-off-by: Marcus Folkesson <[email protected]>
---
drivers/iio/adc/mcp3911.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 4338552cd0ca..3d9e8ed10874 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -306,7 +306,7 @@ static int mcp3911_probe(struct spi_device *spi)
mutex_init(&adc->lock);
- ret = iio_device_register(indio_dev);
+ ret = devm_iio_device_register(&adc->spi->dev, indio_dev);
if (ret)
goto clk_disable;
@@ -326,8 +326,6 @@ static void mcp3911_remove(struct spi_device *spi)
struct iio_dev *indio_dev = spi_get_drvdata(spi);
struct mcp3911 *adc = iio_priv(indio_dev);
- iio_device_unregister(indio_dev);
-
clk_disable_unprepare(adc->clki);
if (adc->vref)
regulator_disable(adc->vref);
--
2.36.1
On Thu, Jun 23, 2022 at 7:40 PM Marcus Folkesson
<[email protected]> wrote:
>
> Keep using managed resources as much as possible.
You may not mix devm_ and non-devm_ API calls like this.
So, you rule of thumb that goto is most of the time wrong after devm_ call.
--
With Best Regards,
Andy Shevchenko
Thank you for your comments (all of them) Andy!
On Thu, Jun 23, 2022 at 09:01:59PM +0200, Andy Shevchenko wrote:
> On Thu, Jun 23, 2022 at 7:40 PM Marcus Folkesson
> <[email protected]> wrote:
> >
> > Keep using managed resources as much as possible.
>
> You may not mix devm_ and non-devm_ API calls like this.
> So, you rule of thumb that goto is most of the time wrong after devm_ call.
Can you please confirm that clocks and regulators are disabled when the
resources are handed back?
I cannot see where when I'm trying to follow the code.
Best regards
Marcus Folkesson
On Fri, 24 Jun 2022 08:29:20 +0200
Marcus Folkesson <[email protected]> wrote:
> Thank you for your comments (all of them) Andy!
>
> On Thu, Jun 23, 2022 at 09:01:59PM +0200, Andy Shevchenko wrote:
> > On Thu, Jun 23, 2022 at 7:40 PM Marcus Folkesson
> > <[email protected]> wrote:
> > >
> > > Keep using managed resources as much as possible.
> >
> > You may not mix devm_ and non-devm_ API calls like this.
> > So, you rule of thumb that goto is most of the time wrong after devm_ call.
>
> Can you please confirm that clocks and regulators are disabled when the
> resources are handed back?
> I cannot see where when I'm trying to follow the code.
Andy isn't arguing that the goto is wrong but rather that you cannot
in general safely use devm_* calls if their failure leads to having to
do any cleanup. The reason is the ordering is hard to reason about. Sometimes
it's safe, but often enough causes problems that we basically refuse to think
hard enough to figure out if it is. Hence basic rule is don't do it.
The issue is this.
probe() {
non_devm_call_1();
ret = devm_call_2()
if (ret)
goto err;
return 0;
err:
unwind_non_devm_call_1()
}
remove() {
unwind_non_devm_call_1()
}
remove or error path should unwind in opposite order of what happens in probe.
On the rare occasion where that isn't the right choice, there should be very
clear comments to say why.
Order is
remove() -> unwind_non_devm_call_1()
devm_managed_cleanup() -> unwind_devm_call_2()
Whereas should be
remove()-> unwind_call_2() then unwind_call_1()
There are two ways to solve this. Either only use devm for those
elements in probe() that happen before the first thing you need to
unwind manually or make everything devm managed (it unwinds in reverse
order of setup) devm_add_action_or_reset() allows you to use your
own devm_ managed callbacks if there isn't a standard one available.
Jonathan
>
> Best regards
> Marcus Folkesson
On Sat, Jun 25, 2022 at 1:15 PM Jonathan Cameron <[email protected]> wrote:
>
> On Fri, 24 Jun 2022 08:29:20 +0200
> Marcus Folkesson <[email protected]> wrote:
>
> > Thank you for your comments (all of them) Andy!
> >
> > On Thu, Jun 23, 2022 at 09:01:59PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jun 23, 2022 at 7:40 PM Marcus Folkesson
> > > <[email protected]> wrote:
> > > >
> > > > Keep using managed resources as much as possible.
> > >
> > > You may not mix devm_ and non-devm_ API calls like this.
> > > So, you rule of thumb that goto is most of the time wrong after devm_ call.
> >
> > Can you please confirm that clocks and regulators are disabled when the
> > resources are handed back?
> > I cannot see where when I'm trying to follow the code.
> Andy isn't arguing that the goto is wrong but rather that you cannot
> in general safely use devm_* calls if their failure leads to having to
> do any cleanup. The reason is the ordering is hard to reason about. Sometimes
> it's safe, but often enough causes problems that we basically refuse to think
> hard enough to figure out if it is. Hence basic rule is don't do it.
>
> The issue is this.
> probe() {
>
> non_devm_call_1();
> ret = devm_call_2()
> if (ret)
> goto err;
>
> return 0;
> err:
> unwind_non_devm_call_1()
> }
>
> remove() {
> unwind_non_devm_call_1()
> }
>
> remove or error path should unwind in opposite order of what happens in probe.
> On the rare occasion where that isn't the right choice, there should be very
> clear comments to say why.
>
> Order is
>
> remove() -> unwind_non_devm_call_1()
> devm_managed_cleanup() -> unwind_devm_call_2()
>
> Whereas should be
>
> remove()-> unwind_call_2() then unwind_call_1()
>
>
> There are two ways to solve this. Either only use devm for those
> elements in probe() that happen before the first thing you need to
> unwind manually or make everything devm managed (it unwinds in reverse
> order of setup) devm_add_action_or_reset() allows you to use your
> own devm_ managed callbacks if there isn't a standard one available.
Thanks, Jonathan, that's exactly what I meant!
--
With Best Regards,
Andy Shevchenko