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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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));



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux