Date:   Mon, 15 Aug 2022 23:55:54 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Stephen Boyd <sboyd@...nel.org>,
        Matti Vaittinen <mazziesaccount@...il.com>,
        Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
        dri-devel@...ts.freedesktop.org,
        Johan Hovold <johan+linaro@...nel.org>,
        Neil Armstrong <narmstrong@...libre.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Kevin Hilman <khilman@...libre.com>,
        linux-kernel@...r.kernel.org, Daniel Vetter <daniel@...ll.ch>,
        linux-amlogic@...ts.infradead.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-doc@...r.kernel.org, Jonathan Cameron <jic23@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Miaoqian Lin <linmq006@...il.com>,
        linux-arm-kernel@...ts.infradead.org,
        Alexandru Tachici <alexandru.tachici@...log.com>,
        Jerome Brunet <jbrunet@...libre.com>,
        Andrzej Hajda <andrzej.hajda@...el.com>,
        Jonathan Corbet <corbet@....net>,
        Guenter Roeck <linux@...ck-us.net>,
        Jonas Karlman <jonas@...boo.se>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        Michael Turq uette <mturquette@...libre.com>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Jean Delvare <jdelvare@...e.com>,
        Alexandru Ardelean <aardelean@...iqon.com>,
        linux-hwmon@...r.kernel.org, linux-clk@...r.kernel.org,
        Nuno Sá <nuno.sa@...log.com>,
        Robert Foss <robert.foss@...aro.org>,
        Aswath Govindraju <a-govindraju@...com>,
        David Airlie <airlied@...ux.ie>, linux-iio@...r.kernel.org
Subject: Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable

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:

> > The basic idea is that drivers should be focused on what they're
> > driving, not navigating the (sometimes) complex integration that's
> > taking place around them. When a device driver probe function is called
> > the device should already be powered on.

> No. ACPI does that in many cases, and that's a real bad idea. There are
> devices that you do *not* want to power up on probe. I'm thinking, for
> example, about camera sensors that have a privacy LED that will light up
> when the sensor is powered up. You don't want it to flash on boot. There
> are also other use cases related to fault tolerance where you want
> drivers to initialize properly and only access the device later.

I don't think it's an either/or thing in terms of approach here - we
need a range of options to choose from.  ACPI is totally fine and solves
real problems for the systems it targets, the problems we see with it
are mainly that it has a very strong system abstraction and doesn't cope
well when things go outside that coupled with the fact that Windows long
ago decided that board files were totally fine for papering over any
problems so people haven't worked on standardisation where they should.
Some SoCs like to do similar things with their power controller cores.

Conversely for example with many (but not all) SoC IPs the mechanics of
the system integration and range of options available are such that
dealing with them is kind of out of scope of the driver, but they're
often very repetitive over any given SoC so there is a benefit in
pushing things into power domains rather than having the driver for the
IP manage everything.  We need to be able to be flexible so we can find
the best idioms to represent the different systems in front of us rather
than trying to force all systems into a single idiom.

> These devres helpers go in the exact opposite direction of what we
> should be doing, by telling driver authors it's totally fine to not
> implement power management. Why don't we just drop error handling and go
> back to the big kernel lock in that case ? That was much easier to
> program too.

Sometimes it's totally fine to not worry, at least at a first pass.
Perhaps you're more concerned with real time, perhaps your system
doesn't provide control for the relevant resources.  Sometimes the
savings are so negligable that it's questionable if doing the power
manageement is an overall power saving.

> 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. 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.

TBH I think the problem you have here is with devm not with this
particular function.  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.  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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists