linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
	Qais Yousef <qais.yousef@arm.com>,
	USB list <linux-usb@vger.kernel.org>,
	Linux-pm mailing list <linux-pm@vger.kernel.org>,
	Kernel development list <linux-kernel@vger.kernel.org>
Subject: Re: lockdep warning in urb.c:363 usb_submit_urb
Date: Mon, 13 Apr 2020 23:32:43 +0200	[thread overview]
Message-ID: <6362254.rXp5uA8eak@kreacher> (raw)
In-Reply-To: <Pine.LNX.4.44L0.2004102231270.30859-100000@netrider.rowland.org>

On Saturday, April 11, 2020 4:41:14 AM CEST Alan Stern wrote:
> Okay, this is my attempt to summarize what we have been discussing.  
> But first: There is a dev_pm_skip_resume() helper routine which
> subsystems can call to see whether resume-side _early and _noirq driver
> callbacks should be skipped.  But there is no corresponding
> dev_pm_skip_suspend() helper routine.  Let's add one, or rename
> dev_pm_smart_suspend_and_suspended() to dev_pm_skip_suspend().

OK

> Given that, here's my understanding of what should happen.  (I'm
> assuming the direct_complete mechanism is not being used.)  This tries
> to describe what we _want_ to happen, which is not always the same as
> what the current code actually _does_.

OK

> 	During the suspend side, for each of the
> 	{suspend,freeze,poweroff}_{late,noirq} phases: If
> 	dev_pm_skip_suspend() returns true then the subsystem should
> 	not invoke the driver's callback, and if there is no subsystem
> 	callback then the core will not invoke the driver's callback.
> 
> 	During the resume side, for each of the
> 	{resume,thaw,restore}_{early,noirq} phases: If
> 	dev_pm_skip_resume() returns true then the subsystem should
> 	not invoke the driver's callback, and if there is no subsystem
> 	callback then the core will not invoke the driver's callback.
> 
> 	dev_pm_skip_suspend() will return "true" if SMART_SUSPEND is
> 	set and the device's runtime status is "suspended".

Agreed with the above.

> 	power.must_resume gets set following the suspend-side _noirq
> 	phase if power.usage_count > 1 (indicating the device was
> 	in active use before the start of the sleep transition) or
> 	power.must_resume is set for any of the device's dependents.

Or MAY_SKIP_RESUME is unset (which means that the driver does not
allow its resume callbacks to be skipped), or power.may_skip_resume
is unset (which means that the subsystem does not allow the
driver callbacks to be skipped).

> 	dev_pm_skip_resume() will return "false" if the current
> 	transition is RESTORE or power.must_resume is set.  Otherwise:
> 	It will return true if the current transition is THAW,
> 	SMART_SUSPEND is set, and the device's runtime status is
> 	"suspended".

The other way around.  That is:

dev_pm_skip_resume() will return "true" if the current transition is
THAW and dev_pm_skip_suspend() returns "true" for that device (so
SMART_SUSPEND is set, and the device's runtime status is "suspended",
as per the definition of that function above).

Otherwise, it will return "true" if the current transition is RESTORE
(which means that all devices are resumed) or power.must_resume is not
set (so this particular device need not be resumed).

>  It will return "true" if the current transition is
> 	RESUME, SMART_SUSPEND and MAY_SKIP_RESUME are both set, and
> 	the device's runtime status is "suspended".

Unless MAY_SKIP_RESUME is unset for at least one of its descendants (or
dependent devices).

> 	For a RESUME
> 	transition, it will also return "true" if MAY_SKIP_RESUME and
> 	power.may_skip_resume are both set, regardless of
> 	SMART_SUSPEND or the current runtime status.

And if the device was not in active use before suspend (as per its usage
counter) or MAY_SKIP_RESUME is unset for at least one of its descendants (or
dependent devices in general).

> 	At the start of the {resume,thaw,restore}_noirq phase, if
> 	dev_pm_skip_resume() returns true then the core will set the
> 	runtime status to "suspended".  Otherwise it will set the
> 	runtime status to "active".  If this is not what the subsystem
> 	or driver wants, it must update the runtime status itself.

Right.

> Comments and differences with respect to the code in your pm-sleep-core
> branch:
> 
> 	I'm not sure whether we should specify other conditions for
> 	setting power.must_resume.

IMO we should.

Otherwise it is rather hard to catch the case in which one of the
device's descendants has MAY_SKIP_RESUME unset (and so the device
needs to be resumed).

> 	dev_pm_skip_resume() doesn't compute the value described
> 	above.  I'm pretty sure the existing code is wrong.

Well, we don't seem to have reached an agreement on some details
above ...

Cheers!




  reply	other threads:[~2020-04-13 21:32 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-23 14:38 lockdep warning in urb.c:363 usb_submit_urb Qais Yousef
2020-03-23 15:36 ` Oliver Neukum
2020-03-23 15:54   ` Alan Stern
2020-03-23 16:02     ` Qais Yousef
2020-03-23 16:18     ` Oliver Neukum
2020-03-23 15:57   ` Qais Yousef
2020-03-23 16:20     ` Oliver Neukum
2020-03-23 17:29   ` Qais Yousef
2020-03-24  9:08     ` Oliver Neukum
2020-03-24 10:46       ` Qais Yousef
2020-03-24 13:20         ` Oliver Neukum
2020-03-24 13:43           ` Qais Yousef
2020-03-24 13:52             ` Alan Stern
2020-03-24 14:06               ` Qais Yousef
2020-03-24 15:56                 ` Alan Stern
2020-03-24 17:22                   ` Qais Yousef
2020-03-24 19:22                     ` Alan Stern
2020-03-25 15:00                       ` Qais Yousef
2020-03-25 20:49                         ` Alan Stern
2020-03-25 21:28                           ` Rafael J. Wysocki
2020-03-26 12:27                             ` Qais Yousef
2020-03-27 20:45                               ` Alan Stern
2020-03-28 14:15                                 ` Rafael J. Wysocki
2020-03-28 19:58                                   ` Alan Stern
2020-03-29  9:16                                     ` Rafael J. Wysocki
2020-03-29 13:56                                       ` Rafael J. Wysocki
2020-03-29 16:27                                       ` Alan Stern
2020-04-03 15:04                                         ` Rafael J. Wysocki
2020-04-03 16:13                                           ` Rafael J. Wysocki
2020-04-03 16:41                                           ` Alan Stern
2020-04-03 18:32                                             ` Rafael J. Wysocki
2020-04-03 20:15                                               ` Alan Stern
2020-04-06 16:45                                                 ` Rafael J. Wysocki
2020-04-06 20:25                                                   ` Alan Stern
2020-04-09 18:45                                                     ` Rafael J. Wysocki
2020-04-11  2:41                                                       ` Alan Stern
2020-04-13 21:32                                                         ` Rafael J. Wysocki [this message]
2020-04-14 13:43                                                           ` Rafael J. Wysocki
2020-04-14 17:47                                                             ` Alan Stern
2020-04-15 22:20                                                               ` Rafael J. Wysocki
2020-04-16 15:18                                                                 ` Alan Stern
2020-04-17  9:57                                                                   ` Rafael J. Wysocki
2020-04-17 16:10                                                                     ` Alan Stern
2020-04-17 21:54                                                                       ` Rafael J. Wysocki
2020-04-17 23:37                                                                         ` Alan Stern
2020-04-18  9:40                                                                           ` Rafael J. Wysocki
2020-04-03 17:08                                         ` Rafael J. Wysocki
2020-04-20 20:26                           ` Alan Stern
2020-04-21 11:15                             ` Qais Yousef
2020-03-24 13:47         ` Alan Stern

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6362254.rXp5uA8eak@kreacher \
    --to=rjw@rjwysocki.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=qais.yousef@arm.com \
    --cc=rafael@kernel.org \
    --cc=stern@rowland.harvard.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).