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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 90CF4ECE564 for ; Tue, 10 Sep 2024 13:51:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:References:From: To:Cc:Subject:Message-Id:Date:Content-Type:Content-Transfer-Encoding: Mime-Version:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=FKaZoEWr8UWtqcWAfis4JtnDRQPZMMSkWUHYpeNsgqY=; b=bcwk6J/3Cs+3c/31PpiWlHd4re fqjpMZ9lJsL4RjtfcvLjFuBimjIMqlgy/1befHh/KagrCSCKP4ki4cmyP4OjBp7isRbupVrMWKmqO xNS+uAO3+kbfW9w1xm+6qv3DLhgiQBtA7MjzP55yHM+jzlCQBq2g8K/F7tnku7eLU0iTdVbxP1IdP v5nYfSOiPMSTgVZvXzpSaqgAPY2mBbkdIU/XSRsfitanH5BYyPXR+FkyGi9dHSC5J7IlLynSSHJLf SaDtTFIFxs7r71TMveuukTkxfIyGE684SjIcfPmyLB69Pj9yBieDQprbkYF2apt6B9hQU0QqVzM2j W1wgZ7qw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1so1HW-00000005mRg-0dtw; Tue, 10 Sep 2024 13:51:46 +0000 Received: from relay6-d.mail.gandi.net ([2001:4b98:dc4:8::226]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1so1GT-00000005mJl-1t9F for linux-arm-kernel@lists.infradead.org; Tue, 10 Sep 2024 13:50:43 +0000 Received: by mail.gandi.net (Postfix) with ESMTPSA id 4D6B7C000D; Tue, 10 Sep 2024 13:50:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1725976238; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=FKaZoEWr8UWtqcWAfis4JtnDRQPZMMSkWUHYpeNsgqY=; b=eaGPx47vgkkjXWUCMPhevbzMVrQx+6xiGAlWW1jjipJdpQiNhIsAiuyDC7TDz/BFoQFJSH 5FbcGgVYB41zBYzcUmIxkZGyrp2FqyVK0tDTCtDBbCDAjYLpg2Ps1+zkVOAFEhNrPNpK9a knMtt3c6Emm6LpuHYQf0/QGK9NFH3Wlq52sL8/lmQcQbckzrh8F6/GPa/Xm6B0gff9i5AT Mbyt4rbAEzoyCHsCipRVb0jWL1KJvId5qvMHc1228PvJXjkN42P6o0VBwuywXW5Naq6D+X fJUQMEwVlEg38PNdi0PxJ8jCSHcDLiTY0LPBO76k2yZOlD0t39bv78NPLKdghA== Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 10 Sep 2024 15:50:37 +0200 Message-Id: Subject: Re: [PATCH v5 09/12] xhci: introduce xhci->lost_power flag Cc: , , , , "Kevin Hilman" , =?utf-8?q?Gr=C3=A9gory_Clement?= , "Thomas Petazzoni" To: "Roger Quadros" , "Greg Kroah-Hartman" , "Rob Herring" , "Krzysztof Kozlowski" , "Conor Dooley" , "Peter Chen" , "Pawel Laszczak" , "Mathias Nyman" , "Nishanth Menon" , "Vignesh Raghavendra" , "Tero Kristo" From: =?utf-8?q?Th=C3=A9o_Lebrun?= X-Mailer: aerc 0.18.2-0-ge037c095a049 References: <20240726-s2r-cdns-v5-0-8664bfb032ac@bootlin.com> <20240726-s2r-cdns-v5-9-8664bfb032ac@bootlin.com> <1cd45625-84e4-43aa-ae2b-a59f10add898@kernel.org> In-Reply-To: <1cd45625-84e4-43aa-ae2b-a59f10add898@kernel.org> X-GND-Sasl: theo.lebrun@bootlin.com X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240910_065041_980027_E9460059 X-CRM114-Status: GOOD ( 28.49 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon Aug 5, 2024 at 3:41 PM CEST, Roger Quadros wrote: > On 26/07/2024 21:17, Th=C3=A9o Lebrun wrote: > > The XHCI_RESET_ON_RESUME quirk allows wrappers to signal that they > > expect a reset after resume. It is also used by some to enforce a XHCI > > reset on resume (see needs-reset-on-resume DT prop). > >=20 > > Some wrappers are unsure beforehands if they will reset. Add a mechanis= m > > to signal *at resume* if power has been lost. Parent devices can set > > this flag, that defaults to the XHCI_RESET_ON_RESUME value. > >=20 > > The XHCI_RESET_ON_RESUME quirk still triggers a runtime_pm_get() on the > > controller. This is required as we do not know if a suspend will > > trigger a reset, so the best guess is to avoid runtime PM. > >=20 > > Reset the xhci->lost_power value each time in xhci_resume(), making it > > safe for devices to only set lost_power on some resumes. > >=20 > > Signed-off-by: Th=C3=A9o Lebrun > > --- > > drivers/usb/host/xhci.c | 8 +++++++- > > drivers/usb/host/xhci.h | 6 ++++++ > > 2 files changed, 13 insertions(+), 1 deletion(-) > >=20 > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > > index 0a8cf6c17f82..2c9b32d339f9 100644 > > --- a/drivers/usb/host/xhci.c > > +++ b/drivers/usb/host/xhci.c > > @@ -1029,9 +1029,12 @@ int xhci_resume(struct xhci_hcd *xhci, pm_messag= e_t msg) > > =20 > > spin_lock_irq(&xhci->lock); > > =20 > > - if (hibernated || xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken= _suspend) > > + if (hibernated || xhci->lost_power || xhci->broken_suspend) > > Why not treat xhci->lost_power and xhci->quriks & XHCI_RESET_ON_RESUME in= dependently? > > XHCI_RESET_ON_RESUME is sued by devices that know they always need to be = reset on resume. > > xhci->lost_power is used by devices that don't have consistent behavior. The goal is to avoid almost-duplicate functionality. I feel like: XHCI_RESET_ON_RESUME is the default value of xhci->lost_power, which might be modified at resume. Is a more straight forward solution than: Both XHCI_RESET_ON_RESUME and xhci->lost_power define if power was lost at resume. First must be statically known, second can be updated during runtime. If second is used, first one must NOT be set. Indeed, the first solution brings two additional lines of code as you commented below. I'd argue the easier-to-wrap-your-head-around logic is more important. Tell me if you are convinced the second approach is better. > > > > reinit_xhc =3D true; > > =20 > > + /* Reset to default value, parent devices might correct it at next re= sume. */ > > + xhci->lost_power =3D !!(xhci->quirks & XHCI_RESET_ON_RESUME); > > + > > then you don't need to do this. To be honest, I added this line out of rigor. We could remove it and say that any device that modifies xhci->lost_power once at resume must set it at each later resume. The above line felt like a small safety net to avoid logic mistakes. > > > if (!reinit_xhc) { > > /* > > * Some controllers might lose power during suspend, so wait > > @@ -5228,6 +5231,9 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_= quirks_t get_quirks) > > if (get_quirks) > > get_quirks(dev, xhci); > > =20 > > + /* Default value, that can be corrected at resume. */ > > + xhci->lost_power =3D !!(xhci->quirks & XHCI_RESET_ON_RESUME); > > +=20 > > nor this. Regards, -- Th=C3=A9o Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com