From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 362BAC2D0E8 for ; Fri, 27 Mar 2020 20:45:12 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 131CD206DB for ; Fri, 27 Mar 2020 20:45:12 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727600AbgC0UpL (ORCPT ); Fri, 27 Mar 2020 16:45:11 -0400 Received: from netrider.rowland.org ([192.131.102.5]:48975 "HELO netrider.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1727322AbgC0UpK (ORCPT ); Fri, 27 Mar 2020 16:45:10 -0400 Received: (qmail 5854 invoked by uid 500); 27 Mar 2020 16:45:09 -0400 Received: from localhost (sendmail-bs@127.0.0.1) by localhost with SMTP; 27 Mar 2020 16:45:09 -0400 Date: Fri, 27 Mar 2020 16:45:09 -0400 (EDT) From: Alan Stern X-X-Sender: stern@netrider.rowland.org To: "Rafael J. Wysocki" , Qais Yousef cc: USB list , Linux-pm mailing list , Kernel development list Subject: Re: lockdep warning in urb.c:363 usb_submit_urb In-Reply-To: <20200326122744.kbtlmev2ravn3wey@e107158-lin> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 26 Mar 2020, Qais Yousef wrote: > On 03/25/20 22:28, Rafael J. Wysocki wrote: > > On Wed, Mar 25, 2020 at 9:49 PM Alan Stern wrote: > > > Raphael, now that we have the direct_complete mechanism, can we revisit > > > this? Should the PM core automatically call pm_runtime_set_active() if > > > dev->power.direct_complete isn't set? Perhaps in device_resume_early() > > > prior to the pm_runtime_enable() call? > > > > > > It's possible we discussed this and decided against it at the time when > > > direct_complete was added, but if so I don't remember what was said. > > > > Me neither. :-) > > > > That said complexity has grown since then and there are the > > DPM_FLAG_SMART_SUSPEND and DPM_FLAG_LEAVE_SUSPENDED flags that can be > > used to control that behavior to some extent. > > > > Setting DPM_FLAG_SMART_SUSPEND alone, in particular, causes > > pm_runtime_set_active() to be called at the noirq stage of device > > resume either by the core or by bus types (e.g. PCI) etc. > > > > It looks like ohci-platform might use DPM_FLAG_SMART_SUSPEND, but I > > need to take a closer look at that (possibly later this week). > > Okay I take it this was root caused correctly and now it's a question of which > is a better fix. Indeed. Raphael, I've been going over the PM core code, trying to figure out what it's really doing. It's kind of a mess. A large part of the problem is related to an inconsistency between the documentation and the code. include/linux/pm.h says that DPM_FLAG_SMART_SUSPEND tells bus types and PM domains about what the driver wants. This strongly implies that the PM core will ignore SMART_SUSPEND. But in fact the core does check that flag and takes its own actions if the device has no subsystem-level callbacks! Furthermore, the PM core's actions don't seem to make sense. If the flag is set and the device is runtime-suspended when the system sleep begins, the core will skip issuing the suspend_late and suspend_noirq callbacks to the driver. But it doesn't skip issuing the suspend callback! I can't figure that out. Furthermore, the decisions about whether to skip the resume_noirq, resume_early, and resume callbacks are based on different criteria from the decisions on the suspend side. That's not all: The SMART_SUSPEND decisions completely ignore the value of DPM_FLAG_NEVER_SKIP! NEVER_SKIP affects only the direct_completion pathway. SMART_SUSPEND seems to have two different meanings. (1) If the device is already in runtime suspend when a system sleep starts, skip the suspend_late and suspend_noirq callbacks. (2) Under certain (similar) circumstances, skip the resume callbacks. The documentation only mentions (1) but the code also handles (2). Other things in there also seem strange. device_prepare() does a WARN_ON if either SMART_SUSPEND or LEAVE_SUSPENDED is set and the device is not runtime-PM-enabled. That's understandable, but it's also racy. A system sleep can begin at any time; how can a driver know when it is safe to disable a device's runtime PM briefly? When device_prepare() calculates the power.direct_complete flag, it checks to see whether the device is currently in runtime suspend in some cases but not in others, as in the code added by your commit c62ec4610c40 ("PM / core: Fix direct_complete handling for devices with no callbacks"). Since the runtime-PM state is going to checked in __device_suspend() anyway, we shouldn't need to check it here at all. At a couple of points in the code, THAW and RESTORE events are each treatedly specially, with no explanation. The power.may_skip_resume flag is used in only one place, when LEAVE_SUSPENDED is set and there are subsystem-level callbacks. In particular, it is _not_ used by dev_pm_may_skip_resume(). That seems highly suspicious at best. I think it would be worthwhile to expend some serious effort straightening all this stuff out. Perhaps we could start with a more explicit description of what is supposed to happen at each step. (Things to be careful about include phrases like "leave suspended", which is not the same as "don't call the resume callbacks", even though the two are easily conflated.) What do you think? Alan Stern