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
