Re: [PATCH 02/10] iio: adc: mcp3911: use resource-managed version of iio_device_register

[Jonathan Cameron]

On Fri, 24 Jun 2022 08:29:20 +0200
Marcus Folkesson <marcus.folkesson@xxxxxxxxx> 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
> > <marcus.folkesson@xxxxxxxxx> 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