Re: lockdep warning in urb.c:363 usb_submit_urb

From: Rafael J. Wysocki
Date: Mon Apr 06 2020 - 12:45:54 EST


On Friday, April 3, 2020 10:15:09 PM CEST Alan Stern wrote:
> For the most part we seem to be in agreement.

Indeed.

> On Fri, 3 Apr 2020, Rafael J. Wysocki wrote:
>
> > On Friday, April 3, 2020 6:41:05 PM CEST Alan Stern wrote:
> > > On Fri, 3 Apr 2020, Rafael J. Wysocki wrote:
>
> > > > (b) Drivers that set SMART_SUSPEND are allowed to reuse their PM-runtime
> > > > callbacks for system-wide suspend and resume.
> > > >
> > > > That is, they can point either the ->suspend_late or the ->suspend_noirq
> > > > callback pointer to the same function as ->runtime_suspend and they can
> > > > point either the ->resume_noirq or ->the resume_early callback to the'
> > > > same function as ->runtime_resume.
> > >
> > > Well, in theory any driver or subsystem can do this whenever it wants
> > > to, regardless of any flag settings.
> >
> > Not exactly.
> >
> > Say the driver wants to point both ->runtime_suspend and ->suspend_late to
> > the same function.
> >
> > If the bus type doesn't provide system-wide PM callbacks at all (which is
> > the case for some bus types), that only works if the device is never
> > runtime-suspended when ->suspend_late is about to run, because otherwise
> > the function in question needs to check the context in which it is running
> > (PM-runtime vs system-wide and runtime-suspended vs runtime-active in the
> > latter case) which at least is awkward and hard to get right.
> >
> > > It's then up to the driver or
> > > subsystem to make sure the callback "does the right thing".
> >
> > In theory.
>
> Okay. In any case, this is about what drivers should do, not about
> what the core should do.

These two things are related to each other though.

> > > What I'm concerned about now is: What guarantees can the core give to
> > > the driver and subsystem, so that they will know what is necessary in
> > > order to "do the right thing"?
> >
> > I'm not sure what you mean.
> >
> > If the subsystem provides callbacks, the core will run them regardless.
> >
> > If it does not provide callbacks, the core will skip ->suspend_late and
> > ->suspend_noirq for the driver and the device will remain suspended.
> >
> > If SMART_SUSPEND is not set, the core will execute all of the callbacks
> > that are present.
>
> All right, then those are the guarantees I was thinking of.

OK

> > > > (c) Drivers that set SMART_SUSPEND are alwo allowed to provide special
> > > > simplified callbacks for the "freeze" and "thaw" transitions during
> > > > hibernation (and restore) and (if they do so) special callbacks for the
> > > > "restore" phase.
> > >
> > > What do you mean by "simplified"?
> >
> > Avoiding actual PM.
> >
> > > As I see it, the suspend-side callbacks are generally responsible for
> > > four things:
> > >
> > > 1. Quiesce the device (finish ongoing I/O and do not allow any
> > > more to start).
> > >
> > > 2. Save the current device state.
> > >
> > > 3. Install the appropriate wakeup settings.
> > >
> > > 4. Put the device into low-power mode.
> > >
> > > (Not explicitly listed: Perform a runtime-resume if needed in order to
> > > carry out these four items.)
> > >
> > > During a SUSPEND transition, we usually expect all four to happen.
>
> Based on what you said elsewhere, 4 may not be needed for SUSPEND
> (depending on the platform).

Right.

> > > During a FREEZE transition, we only require 1.
>
> Actually, FREEZE should do 2 as well. Doing all four is acceptable,
> though not optimal.

Right.

> > That's what I mean by "simplified".
> >
> > > During a POWEROFF
> > > transition we require 1 and 3, and possibly 4 (depending on how the
> > > platform handles poweroff).
> >
> > But doing 2 is not a bug AFAICS.
>
> Agreed.
>
> > > Similar requirements apply to the resume-side callbacks. (But note
> > > that RESTORE is not the inverse of POWEROFF; it is more like an inverse
> > > of FREEZE with the added complication that the device's initial state
> > > is unknown.)
> >
> > It actually isn't even an inverse of FREEZE. It is like RESUME with the
> > additional requirements that (a) it can never be skipped and (b) the
> > device need not be in a low-power state when it runs (the initial state
> > of it is unknown if you will).
>
> Let's put it like this: The resume-side callbacks should have the
> overall effect of bringing the device back to its initial state, with
> the following exceptions and complications:
>
> Unless SMART_SUSPEND and LEAVE_SUSPEND are both set, a device
> that was in runtime suspend before the suspend_late phase
> must end up being runtime-active after the matching RESUME.
>
> Unless SMART_SUSPEND is set, a device that was in runtime
> suspend before the freeze_late phase must end up being
> runtime-active after the matching THAW.

Correct.

> [I'm not so sure about this. Wouldn't it make more sense to treat
> _every_ device as though SMART_SUSPEND was set for FREEZE/THAW
> transitions, and require subsystems to do the same?]

Drivers may expect devices to be runtime-active when their suspend
callbacks are invoked unless they set SMART_SUSPEND. IOW, without
SMART_SUSPEND set the device should not be left in runtime suspend
during system-wide suspend at all unless direct-complete is applied
to it.

> After RESTORE, _every_ device must end up being runtime
> active.

Correct.

> In general, each resume-side callback should undo the effect
> of the matching suspend-side callback. However, because of
> the requirements mentioned in the preceding sentences,
> sometimes a resume-side callback will be issued even though
> the matching suspend-side callback was skipped -- i.e., when
> a device that starts out runtime-suspended ends up being
> runtime-active.
>
> How does that sound?

It is correct, but in general the other way around is possible too.
That is, a suspend-side callback may be issued without the matching
resume-side one and the device's PM runtime status may be changed
if LEAVE_SUSPENDED is set and SMART_SUSPEND is unset.

> > > What changes to this analysis would SMART_SUSPEND allow? None if the
> > > device is runtime-active. But if the device is runtime-suspended and
> > > the wakeup settings don't need to be changed, then presumably none of
> > > the four items are necessary.
> > >
> > > Is this what you mean?
> >
> > No.
> >
> > What I meant was that even if the driver pointed ->runtime_suspend and
> > ->suspend_late (say) to the same function and it pointed ->resume_early
> > and ->runtime_resume to the same function, it didn't have to point
> > ->freeze_late and ->thaw_early to the same pair of functions, respectively.
> >
> > It can point ->freeze_late and ->thaw_early to a pair of different functions
> > that only quiesce the device and reverse that, respectively.
>
> Again, that describes what drivers or subsystems should do, not what
> the core will do.
>
> > > > [OK, I realize that (b) and (c) are not documented, see the notes below.]
> > > >
> > > > Because of (a), if the device with SMART_SUSPEND set is still runtime-suspended
> > > > during the "late" phase of suspend, the core will not invoke the driver's
> > > > "late" and "noirq" suspend callbacks directly (*). Middle layer (subsystem)
> > > > code is expected to behave accordingly.
> > >
> > > Okay, this agrees with what I wrote above.
> > >
> > > > Because of (b), if the "late" and "noirq" driver callbacks were skipped during
> > > > the "freeze" transition, the core will also avoid invoking the "noirq" and
> > > > "early" callbacks provided by the driver during the "thaw" transition and
> > > > the callbacks during the "restore" transition will be executed unconditionally
> > > > (**). Middle layer code is expected to behave accordingly.
> > >
> > > All right. To summarize: If the driver's ->freeze_late callback is
> > > skipped then the driver's ->thaw-early will be skipped, and similarly
> > > for ->freeze_noirq and ->thaw_noirq. But RESTORE callbacks are never
> > > skipped. Correct?
> >
> > Yes.
>
> And this will be true whether or not LEAVE_SUSPENDED is set, right?

Right.

> > > However, the most difficult transitions are SUSPEND and RESUME. Is it
> > > accurate to say that if the driver's ->suspend_late callback is skipped
> > > then the driver's ->resume_early will be skipped, and similarly for
> > > ->suspend_noirq and ->resume_noirq?
> >
> > If LEAVE_SUSPENDED is set in addition to SMART_SUSPEND, then yes.
> >
> > > > Notes:
> > > >
> > > > 1. I have considered splitting SMART_SUSPEND into two or even three flags
> > > > so that (a), (b) and (c) are each associated with a separate flag, but
> > > > then I would expect the majority of users to use all of them anyway.
> > > >
> > > > 2. LEAVE_SUSPENDED (which may be better renamed to SKIP_RESUME) is kind of
> > > > expected to be used along with SMART_SUSPEND unless there is a good enough
> > > > reason to avoid using it. I admit that this isn't really straightforward,
> > > > maybe the default behavior should be to skip the resume and there should be
> > > > FORCE_RESUME instead of LEAVE_SUSPENDED.
> > >
> > > One question not addressed above (in fact, the original reason for
> > > getting you involved in this discussion): What about the device's
> > > power.runtime_status? Shall we say that that core will call
> > > pm_runtime_set_active() at some point before issuing the ->complete
> > > callback unless some combination of flags is set? And what should that
> > > combination be?
> > >
> > > After all, we expect that most drivers will want their devices to be in
> > > the runtime-active state at the end of a system sleep or hibernation.
> > > It makes sense for the core to do the necessary housekeeping.
> >
> > The core will set the PM-runtime status to "active" in device_resume_noirq()
> > if (a) the subsystem callbacks are not invoked (otherwise the subsystem is
> > responsible for doing that) and (b) if the driver's callback not skipped
> > (in which case its ->resume_early callback will not be skipped too).
>
> Are you certain you want the subsystem callback to be responsible for
> setting the runtime status to "active"? Isn't this an example of
> something the core could do in order to help simplify subsystems?

The rationale here is that whoever decides whether or not to skip the
driver-level callbacks, should also set the PM-runtime status of the
device to match that decision.

> > > > 3. (*) Under the assumption that either ->suspend_late or ->suspend_noirq
> > > > points to the same routine as ->runtime_suspend (and the other is NULL),
> > > > invokig that callback for a runtime-suspended device is technically invalid.
> > >
> > > Does this invalidate anything I wrote above?
> >
> > I don't think so. It is the reason why driver callbacks are skipped for
> > runtime-suspended devices.
>
> And this brings up another thing the core might do to help simplify
> drivers and subsystems: If SMART_SUSPEND isn't set and the device is in
> runtime suspend, couldn't the core do a pm_runtime_resume before
> issuing the ->suspend or ->suspend_late callback?

It could, but sometimes that is not desirable. Like when the drivver points its
suspend callback to pm_runtime_force_suspend().

> > > > In turn, under the assumption that either ->resume_early or ->resume_noirq
> > > > points to the same routine as ->runtime_resume (and the other is NULL), it is
> > > > valid to invoke that callback if the late/noirq suspend was skipped.
> > >
> > > In other words, it's okay for the core either to issue or skip those
> > > callbacks. Presumably the decision will be made based on some flag
> > > setting?
> >
> > Yes. A flag combined with the PM-runtime status of the device in
> > device_suspend_noirq().
> >
> > > > 4. (**) If the "freeze" and "thaw" callbacks are simplified, they cannot be
> > > > run back-to-back with ->runtime_resume and ->runtime_suspend, respectively.
> > > > Thus if "freeze" is skippend, "thaw" must be skipped too. However,
> > > > "restore" needs to be prepared to be invoked after "freeze" or
> > > > ->runtime_suspend (and the state of the device may not match the
> > > > callback that ran previously), so it must be special.
> > > >
> > > > 5. I agree that skipping the driver level of callbacks depending on what is
> > > > provided by the middle layer is inconsistent, but I wanted to take the
> > > > users of pm_runtime_force_suspend/resume() into account by letting those
> > > > things run.
> > > >
> > > > It would be more consistent to expect middle layer code (bus types, PM
> > > > domains) to provide either all of the noirq/early/late callbacks, or none
> > > > of them and make SMART_SUSPEND and pm_runtime_force_suspend/resume()
> > > > mutually exclusive.
> > >
> > > I don't have a clear idea of how pm_runtime_force_suspend/resume() gets
> > > used. Are we better off ignoring it for the time being?
> >
> > Yes, we are.
>
> We're converging on a final answer!

I think so.

In the meantime I have created a git branch with changes to simplify the code,
rename some things and clarify the documentation a bit:

git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
pm-sleep-core

(https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=pm-sleep-core
for web access).

I'm going to post these changes as patches soon.

Cheers!