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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 26.08.2021 16:00, Jean Delvare wrote:
> 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.
> 
To follow-up on this: This patch has been applied already. Therefore,
if decision is to not go with it, it would need to be reverted.