Re: [PATCH v2 1/9] i2c: i801: Improve disabling runtime pm

[Jean Delvare <jdelvare@suse.de>]

Hi Wolfram,

On Tue, 17 Aug 2021 22:15:58 +0200, Wolfram Sang wrote:
> > > > I dunno if it's being discussed, but with this you effectively allow user to
> > > > override the setting. It may screw things up AFAIU the comment above.
> > >
> > > No, this hasn't been discussed. At least not now. Thanks for the hint.
> > > This attribute is writable for the root user, so we could argue that
> > > the root user has several options to break the system anyway.  

This is something we hear frequently when people don't want to address
problems in their code, but that's not enough to convince me ;-)

> > But it will mean the side effect on this driver and typical (root-run) system
> > application (systemd like?) should care now the knowledge about this
> > side-effect. I do not think it is desired behaviour. But I'm not a maintainer
> > and I commented here just to make everybody understand the consequences of the
> > change.  

Is systemd going to actually make any change to that attribute? I'm no
systemd expert, but I can't see any option in the configuration files
that would be related to autosuspend.

> Jean, are you still fine with this patch then?

My original position was that there are a few other drivers already
doing "this". It's not like we are doing something completely new and
using an API in a way it had never been used before, so it can't be
that bad.

On the other hand, after taking a closer look, I'm not fully certain
that "this" is exactly the same in all these drivers. For example, in
blk-pm.c, pm_runtime_set_autosuspend_delay() is being called with value
-1 initially, but with the idea that someone else (device driver, user)
may set a positive value later. It's not a permanent disable. The
8250_omap driver, however, seems to match the i2c-i801 driver here (I
say "seems" because honestly I'm not sure I fully understand the
comments there, but my understanding is that at least in some
situations, enabling autosuspend later would cause problems).

That being said, it starts looking like a problem for the PM subsystem
maintainers. Basically Heiner is trying to move away from an API which
requires cleaning up on driver removal. This is definitely the
direction we are collectively taking for years now (the whole devm_*
family of functions is about exactly that). So it's considered a good
thing.

If pm_runtime_set_autosuspend_delay() is not suitable for the task then
maybe we need a better API. I will admit I'm at a loss when it comes to
the many pm_runtime_* calls, I'm not going to claim I fully understand
what each of them is doing exactly. But don't we want to simply call
pm_runtime_dont_use_autosuspend() here?

If not and there's no suitable API for the task at the moment, then
better do not apply this patch, and instead ask the PM subsystem
maintainers if they would be willing to implement what we need.

-- 
Jean Delvare
SUSE L3 Support