Hi Mark,

On Mon, Aug 15, 2022 at 05:33:06PM +0100, Mark Brown wrote:

On Mon, Aug 15, 2022 at 06:54:45PM +0300, Laurent Pinchart wrote:

quoted

- With devres, you don't have full control over the order in which
  resources will be released, which means that you can't control the
  power off sequence, in particular if it needs to be sequenced with
  GPIOs and clocks. That's not a concern for all drivers, but this API
  will creep in in places where it shouldn't be used, driver authours
  should really pay attention to power management and not live with the
  false impression that everything will be handled automatically for
  them. In the worst cases, an incorrect power off sequence could lead
  to hardware damage.

I basically agree with these concerns which is why I was only happy with
this API when Matti suggested doing it in a way that meant that the
callers are unable to access the regulator at runtime, this means that
if anyone wants to do any kind of management of the power state outside
of probe and remove they are forced to convert to the full fat APIs.
The general ordering concern with devm is that the free happens too late
but for the most part this isn't such a concern with regulators, they
might have delayed power off anyway due to sharing - it's no worse than
memory allocation AFAICT.  Given all the other APIs using devm it's
probably going to end up fixing some bugs.

For sequencing I'm not convinced it's much worse than the bulk API is
anyway, and practically speaking I expect most devices that have
problems here will also need more control over power anyway - it's
certainly the common case that hardware has pretty basic requirements
and is fairly tolerant.

I'm not extremely concerned here at the moment, as power should be the
last thing to be turned off, after clocks and reset signals. As clocks
and GPIOs will still be controlled manually in the driver .remove()
function, it means that power will go last, which should be fine.
However, should a devm_clk_get_enable() or similar function be
implemented, we'll run into trouble. Supplying active high input signals
to a device that is not powered can lead to latch-up, which tends to
only manifest after a statistically significant number of occurrences of
the condition, and can slowly damage the hardware over time. This is a
real concern as it will typically not be caught during early
development. I think we would still be better off with requiring drivers
to manually handle powering off the device until we provide a mechanism
that can do so safely in an automated way.

quoted

- Powering regulators on at probe time and leaving them on is a very bad
  practice from a power management point of view, and should really be
  discouraged. Adding convenience helpers to make this easy is the wrong
  message, we should instead push driver authors to implement proper
  runtime PM.

The stick simply isn't working here as far as I can see.

Do you think there's no way we can get it to work, instead of giving up
and adding an API that goes in the wrong direction ? :-( I'll give a
talk about the dangers of devm_* at the kernel summit, this is something
I can mention to raise awareness of the issue among maintainers,
hopefully leading to improvements through better reviews.

-- 
Regards,

Laurent Pinchart