On Thu, Aug 26, 2021 at 04:00:21PM +0200, Jean Delvare wrote: > On Tue, 17 Aug 2021 22:15:58 +0200, Wolfram Sang wrote: Jean, thanks for your response. My 2 cents below. > > > > > 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. Correct, it's expected that either system wide tool or user with enough privileges may enable it. > The 8250_omap driver, Oh, no, please, do not use that driver as an example for runtime PM. It's (somehow) broken there! I Cc'ed Tony in case you have more Q:s about it. It's on a healing curve, though. The idea behind I believe is the same, i.e. to allow user to turn it on and off. > 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. -- With Best Regards, Andy Shevchenko
