On Tue, Apr 04, 2023, Johan Hovold wrote: > On Tue, Apr 04, 2023 at 05:46:13PM +0530, Udipto Goswami wrote: > > On 4/4/23 5:13 PM, Johan Hovold wrote: > > > On Tue, Apr 04, 2023 at 01:09:19PM +0200, Oliver Neukum wrote: > > >> On 04.04.23 13:05, Johan Hovold wrote: > > > >>> This is backwards. If you need the device to be active to access these > > >>> registers then you should resume it unconditionally instead of failing. > > > > > >> usually you would be right. But this is debugfs. It is intended to observe > > >> the system in the state it is actually in. If by the act of observation you > > >> wake up the device, you change the experiment. > > > > > > I admittedly didn't look to closely at what this particular debugfs > > > interface is used for, but I generally do not agree with that reasoning. > > > > > > The device is being used; by the driver and ultimately by a user telling > > > the driver to do something on their behalf. The fact that the user is > > > initiating an action through an interface which intended for debugging > > > should not matter (and the user always has the option to check the > > > runtime pm state before initiating the action if that matters at all) > > > for instance, lets say user is trying to dump the value of link_state > > register through dwc3_link_state_show, wouldn't a pm_runtime_get would > > change the link_state? or I'm assuming it wrong? > > There may be some specific debugfs interfaces (e.g. related to PM) where > taking the runtime pm state into account makes sense, but then I don't > think you should return an error if the device happens to be suspended. Agree here. > > Looking at the dwc3 'link_state', it currently just returns "Not > available\n" when not in peripheral mode, for example. Perhaps you can > do something similar if you can neither infer or retrieve the actual > link state. > > But after skimming the backstory for this patch, you yourself said that > the motivation for this patch is simply to avoid crashing when accessing > these interfaces if the device happens to be runtime suspended. > > For that you should just resume the device unconditionally before the > register accesses as all other drivers do (and such a patch should > be backported to stable). > > There's no need to take hypothetical PM debugging issues into account to > address this. > I disagree here. We should not unconditionally resume the device. Checking these states should not interfere with the device current operation. These debugfs attributes are meant to provide the user with debug info. Whether the controller is currently in suspend or not, that's a data point. Thanks, Thinh
