Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

[Matti Vaittinen]

Hi dee Ho Mark, Laurent, Stephen, all

On 8/16/22 01:55, Mark Brown wrote:
> On Tue, Aug 16, 2022 at 12:17:17AM +0300, Laurent Pinchart wrote:
> > On Mon, Aug 15, 2022 at 01:58:55PM -0700, Stephen Boyd wrote:
>
> > You will very quickly see drivers doing this (either directly or
> > indirectly):
>
> > probe()
> > {
> > 	devm_clk_get_enabled();
> > 	devm_regulator_get_enable();
> > }
>
> > Without a devres-based get+enable API drivers can get the resources they
> > need in any order, possibly moving some of those resource acquisition
> > operations to different functions, and then have a clear block of code
> > that enables the resources in the right order.

I agree. And I think that drivers which do that should stick with it. 
Still, as you know the devm-unwinding is also done in well defined 
order. I believe that instead of fighting against the devm we should try 
educate people to pay attention in the order of unwinding (also when not 
handled by the devm. Driver writers occasionally break things also w/o 
devm for example by freeing resources needed by IRQ handlers prior 
freeing the IRQ.)

If "purging" must not be done in reverse order compared to the 
aquisition - then one should not use devm. I know people have done 
errors with devm - OTOH, I've seen devm also fixing bunch of errors.

> > These devres helpers give
> > a false sense of security to driver authors and they will end up
> > introducing problems, the same way that devm_kzalloc() makes it
> > outrageously easy to crash the kernel by disconnecting a device that is
> > in use.

I think this is going a bit "off-topic" but I'd like to understand what 
is behind this statement? From device-writer's perspective - I don't 
know much better alternatives to free up the memory. I don't see how 
freeing stuff at .remove would be any better? As far as I understand - 
if someone is using driver's resources after the device has gone and the 
driver is detached, then there is not much the driver could do to 
free-up the stuff be it devm or not? This sounds like fundamentally 
different problem (to me).

> TBH I think the problem you have here is with devm not with this
> particular function.

I must say I kind of agree with Mark. If we stop for a second to think 
what would the Laurent's example look like if there were no 
devm_regulator_get_enable() provided. I bet the poor driver author could 
have used devm_clk_get_enabled() - and then implemented a .remove for 
disabling the regulator. That would be even worse, right?

> That's a different conversation, and a totally
> valid one especially when you start looking at things like implementing
> userspace APIs which need to cope with hardware going away while still
> visible to userspace.

This is interesting. It's not easy for me to spot how devm changes 
things here? If we consider some removable device - then I guess also 
the .remove() is ran only after HW has already gone? Yes, devm might 
make the time window when userspace can see hardware that has gone 
longer but does it bring any new problem there? It seems to me devm can 
make hitting the spot more likely - but I don't think it brings 
completely new issues? (Well, I may be wrong here - wouldn't be the 
first time :])

> It's *probably* more of a subsystem conversation
> than a driver one though, or at least I think subsystems should try to
> arrange to make it so.


--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Discuss - Estimate - Plan - Report and finally accomplish this:
void do_work(int time) __attribute__ ((const));