On Tue, Apr 04, 2023 at 09:36:28PM +0000, Thinh Nguyen wrote:
> On Tue, Apr 04, 2023, Johan Hovold wrote:
> > On Tue, Apr 04, 2023 at 05:46:13PM +0530, Udipto Goswami wrote:
> > > 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.
But this is not what other drivers do, which means that you will end up
with different behaviour with regards to runtime PM (possibly even for
the very same piece of hardware, see below) depending, on which file in
debugfs you access.
See for example commit 30332eeefec8 ("debugfs: regset32: Add Runtime PM
support") which made this behaviour part of the generic debugfs helpers,
and grepping for drivers that bail out from their debugsfs callbacks
does not seem to show any other instances.
Alan did point out though, that the ehci driver returns a string like
"suspended" when trying to access registers for a suspended device.
That behaviour dates back to before the git era though and long before
we had runtime PM. In fact, ehci still does not seem to implement
runtime PM so this check would essentially only kick when the HCD is
dead IIUC.
Notably both the chipidea and musb drivers runtime resume the controller
when accessing registers through debugfs.
I just tried the xhci debugfs interface and it apparently crashes just
like the dwc3 debugfs do if the device is suspended. Turns out no one
has yet wired up the device pointer which would be used by the generic
debugfs helpers to resume the device before register accesses.
I'll send a patch to fix that up.
Johan
