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