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.
