Message ID | 20231213182656.6165-3-mario.limonciello@amd.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:3b04:b0:fb:cd0c:d3e with SMTP id c4csp7981914dys; Wed, 13 Dec 2023 10:27:43 -0800 (PST) X-Google-Smtp-Source: AGHT+IHlBdSwt47nUw6gD+IHsY3IgaxdBbH8fO6wBfic/XJaRlDtEvMk43p+nPLRSBiq22JFTwoj X-Received: by 2002:a17:902:c946:b0:1d0:c14b:dd0d with SMTP id i6-20020a170902c94600b001d0c14bdd0dmr4565322pla.54.1702492062613; Wed, 13 Dec 2023 10:27:42 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1702492062; cv=pass; d=google.com; s=arc-20160816; b=MTonRUiS1ekwy+cs/efKxaaAfCba9aK69tuA0jD3XbS8awEIckqiYCtDjBhMu0i4SY drwdWolYXsgnlLZjeJ5v+dvBwiJRh4xS0irBsm85cFjRVnGNmjsg5ut83L9PornuSpMB u3jGdFG5i2ld6O2soHtD6K5DwITcoclJ9JXzKIqBKYKIH8+PwOv42KxIfYOLIw1N6lSL ygVBHMN0gAwLQ4NNlUKu6wWM/g9TVgfrGJH6gzPYa6eeixB3orlI+OIKLLjJHZxzLOlj x7GzNMv3ozYzJgtw/RhD33SgF2VEHAwdoumEyfdVKh+5A+vuAck5bPGVyBTK6iURHSms U+RA== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=te3kowBT+jbDEk4VX+EtZh/khotxt1fF/P+qjIC+yDc=; fh=Rx9/fAnZzsj/hB/+nzcyRMJzmx0rQCrVMUHIwDTjudE=; b=UMIAndW36mXybcATmH75oY43WbJ33ydClXxkWTx2QQYAxyXg1rrspS98FN1peyI3Ca s6snfgb9PoRbH1gnV7rUDVjo6Qq/j8KO+aOLpC3KrTuSpc18o5rtO0eHsHBgNVtdOq9Y CZrprkeIBPTbDT24UE/xw7cCDkvecneE1SS+QoWG+QEJg+jnBDFZ9Ngls4K6cQ/C8a3F JhhsNJ9pngU7TabCv1ml39779th39BDHsANhaHaLZQFim+jOOcN1bJtBTztBzzyFp28d PxU8oKqP/PP71V/ZC4CsR8R1BLmPbq8lrvbmFZfnovYJIld3xvh9YytwS16N2Dh15gco 75wA== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@amd.com header.s=selector1 header.b=amwEBJQf; arc=pass (i=1 spf=pass spfdomain=amd.com dmarc=pass fromdomain=amd.com); spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=amd.com Received: from fry.vger.email (fry.vger.email. [2620:137:e000::3:8]) by mx.google.com with ESMTPS id 19-20020a170902c11300b001d3598b87cfsi723448pli.60.2023.12.13.10.27.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 13 Dec 2023 10:27:42 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8; Authentication-Results: mx.google.com; dkim=pass header.i=@amd.com header.s=selector1 header.b=amwEBJQf; arc=pass (i=1 spf=pass spfdomain=amd.com dmarc=pass fromdomain=amd.com); spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=amd.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by fry.vger.email (Postfix) with ESMTP id 1E5E780C3A9B; Wed, 13 Dec 2023 10:27:37 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at fry.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1442353AbjLMS1U (ORCPT <rfc822;dexuan.linux@gmail.com> + 99 others); Wed, 13 Dec 2023 13:27:20 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58608 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1442336AbjLMS1P (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 13 Dec 2023 13:27:15 -0500 Received: from NAM04-DM6-obe.outbound.protection.outlook.com (mail-dm6nam04on2044.outbound.protection.outlook.com [40.107.102.44]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6BB4CB9; Wed, 13 Dec 2023 10:27:21 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=fqHaIrehZU5VRXwz+PAitJuocjFdakOdpszCciN9R/rPPBJ2Hb36f3lawGOA+eeqKRIDao01Zr6imYmrEQ9yLDnUE7fcVsiqUaEXlhC9TIsQmIVkNl/8Jo9OOm4NS1g/zMNN6CupShJEMm9At9xFKkCH3UHZ5ADLEw/fx6QvRGJ9lUcxQdkNxxGboKCAN0kVeCOiyvBwBE9rm8HdArDRTXPCcvB20zgkuienkDrejpKRWFX4s7yup4OM7KORcaRdB9fBErjE35ncbaCQHoQRFi9g4fX2uy4sjtGZvBQGsuQkE2zg1AN46oLy2us5uP6q1wSWr7So8jKQEg9SjASFXg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=te3kowBT+jbDEk4VX+EtZh/khotxt1fF/P+qjIC+yDc=; b=R1mK9tJneQBq/62C8KRRasPIymJKvWO2gxM+Lgk/WVxOca+6MWkGyncJI6Qe9SeMSn++OInw1Ep6NjJDoZzLCCNhEkkR9EbOuxU3uvHD5V9h9//vo0SOYFLAWX869tsHhVjWlcm1sy2/NAy3irQ64pk4RUEnLG3X5c3BMkOvoor0rYk0GWswvGhaMnzzrpI9ZiJxa544gUu1P/rqYzpBoMGRxetvjSu5TmhJNt0IiwbvQgLB21oVUlUteek4KInkh9HGu5dLumudbAuXuixp1TJ8q5Op94KuayO/zTYDODNhdJQf0WK0xcHpnX3ECBkNUAdDwXAx1OojmzQ608g5Vw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=google.com smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none (0) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=te3kowBT+jbDEk4VX+EtZh/khotxt1fF/P+qjIC+yDc=; b=amwEBJQfwTFZS96vwJueBKExEEOrIKdx4fGnrw9T9wZJ323p0mH6Aid8SvLlI2qBwCFasw+noaM3lvDwn1oa930+9c4088oYLbZez7R2WLAgqbYkQEcJ1/DfSGjGgVaMvwwy8wiMUpYZ81+aXcghesan+v5mSaxy3rYvNNbQf3Q= Received: from BYAPR01CA0032.prod.exchangelabs.com (2603:10b6:a02:80::45) by SA1PR12MB7245.namprd12.prod.outlook.com (2603:10b6:806:2bf::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7091.26; Wed, 13 Dec 2023 18:27:18 +0000 Received: from CO1PEPF000044EF.namprd05.prod.outlook.com (2603:10b6:a02:80:cafe::2) by BYAPR01CA0032.outlook.office365.com (2603:10b6:a02:80::45) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7091.26 via Frontend Transport; Wed, 13 Dec 2023 18:27:18 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 165.204.84.17) smtp.mailfrom=amd.com; dkim=none (message not signed) header.d=none;dmarc=pass action=none header.from=amd.com; Received-SPF: Pass (protection.outlook.com: domain of amd.com designates 165.204.84.17 as permitted sender) receiver=protection.outlook.com; client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C Received: from SATLEXMB04.amd.com (165.204.84.17) by CO1PEPF000044EF.mail.protection.outlook.com (10.167.241.69) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.20.7091.26 via Frontend Transport; Wed, 13 Dec 2023 18:27:17 +0000 Received: from AUS-P9-MLIMONCI.amd.com (10.180.168.240) by SATLEXMB04.amd.com (10.181.40.145) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.34; Wed, 13 Dec 2023 12:27:16 -0600 From: Mario Limonciello <mario.limonciello@amd.com> To: Bjorn Helgaas <bhelgaas@google.com>, "Rafael J . Wysocki" <rjw@rjwysocki.net> CC: <linux-pci@vger.kernel.org>, <linux-acpi@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Mario Limonciello <mario.limonciello@amd.com>, <mpearson-lenovo@squebb.ca> Subject: [PATCH 2/2] PCI/portdrv: Place PCIe port hierarchy into D3cold at shutdown Date: Wed, 13 Dec 2023 12:26:56 -0600 Message-ID: <20231213182656.6165-3-mario.limonciello@amd.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20231213182656.6165-1-mario.limonciello@amd.com> References: <20231213182656.6165-1-mario.limonciello@amd.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Originating-IP: [10.180.168.240] X-ClientProxiedBy: SATLEXMB03.amd.com (10.181.40.144) To SATLEXMB04.amd.com (10.181.40.145) X-EOPAttributedMessage: 0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: CO1PEPF000044EF:EE_|SA1PR12MB7245:EE_ X-MS-Office365-Filtering-Correlation-Id: 2bd68045-4312-4317-a9c8-08dbfc09234f X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: KVAOi+hJeynuYf6L6vVSeiLTlOct0HrRqxEYadE94xHq8YUSm+v+6BBYTj4xnfGjcPVK3Ldda9Fk9Sg5/Hie/7pI45CPL2wg+eKgPXbx4piwt5R8BiIKTOfhDG3LBQV3GzYwI0BW5sLrxgPADcrhwoS/03BnPCC5F/LDqWlR0EccXqPgF+D9qcHeORIFqMVajbh/S4H//KeY8jbvtI2Ul8nGJw93y8k1TxCAVmsbPUGzARDtt7q47F6Oh7OtKIwFHsNzGSJNKs9MwqdrDSNJH/A6qw8aELPO2KTaST7xXIuxO/mnn1f0Ac/Xxz9gKZIQYRhGV1brtPxwuzeyWwFAPdhrNV6Lw4+Oyoa04oNVG8prN/cYqBu3LkMs0Qzw9MwfW56xnZ2G/4Xjt4AqGBZ4O9pTHsPNXXRb3W0KOtmOrwbrm/GAYhBHVjnQsspYIPUCvQfKl0S63j70PZOx7qU21RSo1PCgjcE68bE8vXoWGzlYl50mab/3fTWtcNcpY5nseiwlesWgjAqgcN4UMy0J2N34529KqvyWPFxMjtzrjM1E/xTdlG9QeZMccSz9acsmQJTpYv9gbQJ/Y9xiUC6sUETA0GZhqE2rV6oRu4I34ds3YjCbKcSQyXcI9+E6wvhFwu6eIOULBVkoQaFYnbg2ROoKcrJ/CioIQ5qjY0D4NpSo4r9Ou79ufeBicEjPc1EE0exxh7KXcg/OGELZBYh6GtNh7bDxPEg98bUDcBL+daGXl6B1MdO984Vq4uvc0YAr+1uRMgWzocfT4BrFSXT2gLVk31ReROZr8OlhVk2rduw= X-Forefront-Antispam-Report: CIP:165.204.84.17;CTRY:US;LANG:en;SCL:1;SRV:;IPV:CAL;SFV:NSPM;H:SATLEXMB04.amd.com;PTR:InfoDomainNonexistent;CAT:NONE;SFS:(13230031)(4636009)(136003)(346002)(376002)(39860400002)(396003)(230922051799003)(451199024)(64100799003)(82310400011)(186009)(1800799012)(46966006)(40470700004)(36840700001)(54906003)(70586007)(70206006)(110136005)(1076003)(6666004)(16526019)(2616005)(7696005)(336012)(26005)(36756003)(426003)(86362001)(47076005)(82740400003)(356005)(83380400001)(81166007)(36860700001)(41300700001)(478600001)(44832011)(40480700001)(8936002)(8676002)(2906002)(4326008)(5660300002)(316002)(40460700003)(32563001)(36900700001);DIR:OUT;SFP:1101; X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Dec 2023 18:27:17.8823 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 2bd68045-4312-4317-a9c8-08dbfc09234f X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com] X-MS-Exchange-CrossTenant-AuthSource: CO1PEPF000044EF.namprd05.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR12MB7245 X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Wed, 13 Dec 2023 10:27:37 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1785192316827972794 X-GMAIL-MSGID: 1785192316827972794 |
Series |
Improvements to system power consumption at S5
|
|
Commit Message
Mario Limonciello
Dec. 13, 2023, 6:26 p.m. UTC
When a system is being powered off it's important that PCIe ports
have been put into D3cold as there is no other software to turn
off the devices at S5.
If PCIe ports are left in D0 then any GPIOs toggled by the ACPI
power resources may be left enabled and devices may consume excess
power.
Cc: mpearson-lenovo@squebb.ca
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
drivers/pci/pcie/portdrv.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
Comments
On Wed, Dec 13, 2023 at 7:27 PM Mario Limonciello <mario.limonciello@amd.com> wrote: > > When a system is being powered off it's important that PCIe ports > have been put into D3cold as there is no other software to turn > off the devices at S5. > > If PCIe ports are left in D0 then any GPIOs toggled by the ACPI > power resources may be left enabled and devices may consume excess > power. Isn't that a platform firmware issue? It is the responsibility of the platform firmware to properly put the platform into S5, including power removal from devices that are not armed for power-on. > Cc: mpearson-lenovo@squebb.ca > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/pci/pcie/portdrv.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c > index 14a4b89a3b83..08238680c481 100644 > --- a/drivers/pci/pcie/portdrv.c > +++ b/drivers/pci/pcie/portdrv.c > @@ -734,9 +734,14 @@ static void pcie_portdrv_remove(struct pci_dev *dev) > static void pcie_portdrv_shutdown(struct pci_dev *dev) > { > if (pci_bridge_d3_possible(dev)) { > - pm_runtime_forbid(&dev->dev); > - pm_runtime_get_noresume(&dev->dev); > - pm_runtime_dont_use_autosuspend(&dev->dev); > + /* whole hierarchy goes into a low power state for S5 */ > + if (system_state == SYSTEM_POWER_OFF) { > + pci_set_power_state(dev, PCI_D3cold); > + } else { > + pm_runtime_forbid(&dev->dev); > + pm_runtime_get_noresume(&dev->dev); > + pm_runtime_dont_use_autosuspend(&dev->dev); > + } > } Wouldn't it be better to remove power from the port after running the code below? > pcie_port_device_remove(dev); > --
On 12/13/2023 12:38, Rafael J. Wysocki wrote: > On Wed, Dec 13, 2023 at 7:27 PM Mario Limonciello > <mario.limonciello@amd.com> wrote: >> >> When a system is being powered off it's important that PCIe ports >> have been put into D3cold as there is no other software to turn >> off the devices at S5. >> >> If PCIe ports are left in D0 then any GPIOs toggled by the ACPI >> power resources may be left enabled and devices may consume excess >> power. > > Isn't that a platform firmware issue? > > It is the responsibility of the platform firmware to properly put the > platform into S5, including power removal from devices that are not > armed for power-on. The specific issues that triggered this series were tied to the PCIe ports for dGPUs. There is a GPIO that is toggled by _ON or _OFF. Windows calls _OFF as part of S5.. > >> Cc: mpearson-lenovo@squebb.ca >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> drivers/pci/pcie/portdrv.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c >> index 14a4b89a3b83..08238680c481 100644 >> --- a/drivers/pci/pcie/portdrv.c >> +++ b/drivers/pci/pcie/portdrv.c >> @@ -734,9 +734,14 @@ static void pcie_portdrv_remove(struct pci_dev *dev) >> static void pcie_portdrv_shutdown(struct pci_dev *dev) >> { >> if (pci_bridge_d3_possible(dev)) { >> - pm_runtime_forbid(&dev->dev); >> - pm_runtime_get_noresume(&dev->dev); >> - pm_runtime_dont_use_autosuspend(&dev->dev); >> + /* whole hierarchy goes into a low power state for S5 */ >> + if (system_state == SYSTEM_POWER_OFF) { >> + pci_set_power_state(dev, PCI_D3cold); >> + } else { >> + pm_runtime_forbid(&dev->dev); >> + pm_runtime_get_noresume(&dev->dev); >> + pm_runtime_dont_use_autosuspend(&dev->dev); >> + } >> } > > Wouldn't it be better to remove power from the port after running the > code below? > Yes; I think you're right. I'll do some more testing with this. >> pcie_port_device_remove(dev); >> --
On Wed, Dec 13, 2023 at 7:42 PM Mario Limonciello <mario.limonciello@amd.com> wrote: > > On 12/13/2023 12:38, Rafael J. Wysocki wrote: > > On Wed, Dec 13, 2023 at 7:27 PM Mario Limonciello > > <mario.limonciello@amd.com> wrote: > >> > >> When a system is being powered off it's important that PCIe ports > >> have been put into D3cold as there is no other software to turn > >> off the devices at S5. > >> > >> If PCIe ports are left in D0 then any GPIOs toggled by the ACPI > >> power resources may be left enabled and devices may consume excess > >> power. > > > > Isn't that a platform firmware issue? > > > > It is the responsibility of the platform firmware to properly put the > > platform into S5, including power removal from devices that are not > > armed for power-on. > > The specific issues that triggered this series were tied to the PCIe > ports for dGPUs. There is a GPIO that is toggled by _ON or _OFF. > > Windows calls _OFF as part of S5.. I see. > > > >> Cc: mpearson-lenovo@squebb.ca > >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > >> --- > >> drivers/pci/pcie/portdrv.c | 11 ++++++++--- > >> 1 file changed, 8 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c > >> index 14a4b89a3b83..08238680c481 100644 > >> --- a/drivers/pci/pcie/portdrv.c > >> +++ b/drivers/pci/pcie/portdrv.c > >> @@ -734,9 +734,14 @@ static void pcie_portdrv_remove(struct pci_dev *dev) > >> static void pcie_portdrv_shutdown(struct pci_dev *dev) > >> { > >> if (pci_bridge_d3_possible(dev)) { > >> - pm_runtime_forbid(&dev->dev); > >> - pm_runtime_get_noresume(&dev->dev); > >> - pm_runtime_dont_use_autosuspend(&dev->dev); > >> + /* whole hierarchy goes into a low power state for S5 */ > >> + if (system_state == SYSTEM_POWER_OFF) { > >> + pci_set_power_state(dev, PCI_D3cold); > >> + } else { > >> + pm_runtime_forbid(&dev->dev); > >> + pm_runtime_get_noresume(&dev->dev); > >> + pm_runtime_dont_use_autosuspend(&dev->dev); > >> + } > >> } > > > > Wouldn't it be better to remove power from the port after running the > > code below? > > > > Yes; I think you're right. I'll do some more testing with this. > > >> pcie_port_device_remove(dev); > >> -- IIRC, to do this all properly, you'd need to rework the shutdown path to look like the hibernation power-off one. Or even use the latter for shutdown? There was no reason to do that till now, so it has not been done, but it looks like you have one.
Hi Mario and Rafael, On Thu, Dec 14, 2023 at 2:46 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Dec 13, 2023 at 7:42 PM Mario Limonciello > <mario.limonciello@amd.com> wrote: > > > > On 12/13/2023 12:38, Rafael J. Wysocki wrote: > > > On Wed, Dec 13, 2023 at 7:27 PM Mario Limonciello > > > <mario.limonciello@amd.com> wrote: > > >> > > >> When a system is being powered off it's important that PCIe ports > > >> have been put into D3cold as there is no other software to turn > > >> off the devices at S5. > > >> > > >> If PCIe ports are left in D0 then any GPIOs toggled by the ACPI > > >> power resources may be left enabled and devices may consume excess > > >> power. > > > > > > Isn't that a platform firmware issue? > > > > > > It is the responsibility of the platform firmware to properly put the > > > platform into S5, including power removal from devices that are not > > > armed for power-on. > > > > The specific issues that triggered this series were tied to the PCIe > > ports for dGPUs. There is a GPIO that is toggled by _ON or _OFF. > > > > Windows calls _OFF as part of S5.. > > I see. > > > > > > >> Cc: mpearson-lenovo@squebb.ca > > >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > >> --- > > >> drivers/pci/pcie/portdrv.c | 11 ++++++++--- > > >> 1 file changed, 8 insertions(+), 3 deletions(-) > > >> > > >> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c > > >> index 14a4b89a3b83..08238680c481 100644 > > >> --- a/drivers/pci/pcie/portdrv.c > > >> +++ b/drivers/pci/pcie/portdrv.c > > >> @@ -734,9 +734,14 @@ static void pcie_portdrv_remove(struct pci_dev *dev) > > >> static void pcie_portdrv_shutdown(struct pci_dev *dev) > > >> { > > >> if (pci_bridge_d3_possible(dev)) { > > >> - pm_runtime_forbid(&dev->dev); > > >> - pm_runtime_get_noresume(&dev->dev); > > >> - pm_runtime_dont_use_autosuspend(&dev->dev); > > >> + /* whole hierarchy goes into a low power state for S5 */ > > >> + if (system_state == SYSTEM_POWER_OFF) { > > >> + pci_set_power_state(dev, PCI_D3cold); > > >> + } else { > > >> + pm_runtime_forbid(&dev->dev); > > >> + pm_runtime_get_noresume(&dev->dev); > > >> + pm_runtime_dont_use_autosuspend(&dev->dev); > > >> + } > > >> } > > > > > > Wouldn't it be better to remove power from the port after running the > > > code below? > > > > > > > Yes; I think you're right. I'll do some more testing with this. > > > > >> pcie_port_device_remove(dev); > > >> -- > > IIRC, to do this all properly, you'd need to rework the shutdown path > to look like the hibernation power-off one. Or even use the latter > for shutdown? > > There was no reason to do that till now, so it has not been done, but > it looks like you have one. > I am working on exactly same thing but with a different approach. Because this is needed for more than just PCI devices. I haven't written a proper commit message yet, but the implementation is quite simple: diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c index f007116a8427..b90c6cf6faf4 100644 --- a/drivers/acpi/device_pm.c +++ b/drivers/acpi/device_pm.c @@ -967,15 +967,17 @@ EXPORT_SYMBOL_GPL(acpi_pm_set_device_wakeup); * @adev: ACPI device node corresponding to @dev. * @system_state: System state to choose the device state for. */ -static int acpi_dev_pm_low_power(struct device *dev, struct acpi_device *adev, - u32 system_state) +static int acpi_dev_pm_low_power(struct acpi_device *adev, void* data) { int ret, state; + u32 *system_state = data; if (!acpi_device_power_manageable(adev)) return 0; - ret = acpi_dev_pm_get_state(dev, adev, system_state, NULL, &state); + acpi_dev_for_each_child(adev, acpi_dev_pm_low_power, data); + + ret = acpi_dev_pm_get_state(&adev->dev, adev, *system_state, NULL, &state); return ret ? ret : acpi_device_set_power(adev, state); } @@ -1016,7 +1018,7 @@ int acpi_dev_suspend(struct device *dev, bool wakeup) wakeup = false; } - error = acpi_dev_pm_low_power(dev, adev, target_state); + error = acpi_dev_pm_low_power(adev, &target_state); if (error && wakeup) acpi_device_wakeup_disable(adev); @@ -1386,6 +1388,7 @@ static struct dev_pm_domain acpi_general_pm_domain = { static void acpi_dev_pm_detach(struct device *dev, bool power_off) { struct acpi_device *adev = ACPI_COMPANION(dev); + u32 state = ACPI_STATE_S0; if (adev && dev->pm_domain == &acpi_general_pm_domain) { dev_pm_domain_set(dev, NULL); @@ -1400,7 +1403,7 @@ static void acpi_dev_pm_detach(struct device *dev, bool power_off) dev_pm_qos_hide_latency_limit(dev); dev_pm_qos_hide_flags(dev); acpi_device_wakeup_disable(adev); - acpi_dev_pm_low_power(dev, adev, ACPI_STATE_S0); + acpi_dev_pm_low_power(adev, &state); } } } @@ -1514,4 +1517,16 @@ bool acpi_dev_state_d0(struct device *dev) } EXPORT_SYMBOL_GPL(acpi_dev_state_d0); +void acpi_dev_shutdown(struct device *dev) +{ + struct acpi_device *adev = ACPI_COMPANION(dev); + u32 state = ACPI_STATE_S5; + + if (!adev) + return; + + acpi_device_wakeup_disable(adev); + acpi_dev_pm_low_power(adev, &state); +} +EXPORT_SYMBOL_GPL(acpi_dev_shutdown); #endif /* CONFIG_PM */ diff --git a/drivers/base/core.c b/drivers/base/core.c index 6ceaf50f5a67..7e7c99eade63 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -45,6 +45,15 @@ static void __fw_devlink_link_to_consumers(struct device *dev); static bool fw_devlink_drv_reg_done; static bool fw_devlink_best_effort; +#ifdef CONFIG_ACPI +static inline void fw_dev_shutdown(struct device *dev) +{ + acpi_dev_shutdown(dev); +} +#else +static inline void fw_dev_shutdown(struct device *dev) { } +#endif + /** * __fwnode_link_add - Create a link between two fwnode_handles. * @con: Consumer end of the link. @@ -4780,6 +4789,8 @@ void device_shutdown(void) dev->driver->shutdown(dev); } + fw_dev_shutdown(dev); + device_unlock(dev); if (parent) device_unlock(parent); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 641dc4843987..374f9eb75c22 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -1130,6 +1130,7 @@ int acpi_subsys_runtime_resume(struct device *dev); int acpi_dev_pm_attach(struct device *dev, bool power_on); bool acpi_storage_d3(struct device *dev); bool acpi_dev_state_d0(struct device *dev); +void acpi_dev_shutdown(struct device *dev); #else static inline int acpi_subsys_runtime_suspend(struct device *dev) { return 0; } static inline int acpi_subsys_runtime_resume(struct device *dev) { return 0; } @@ -1145,6 +1146,7 @@ static inline bool acpi_dev_state_d0(struct device *dev) { return true; } +static inline void acpi_dev_shutdown(struct device *dev) { } #endif #if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP)
On Thu, Dec 14, 2023 at 4:46 AM Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > Hi Mario and Rafael, > > On Thu, Dec 14, 2023 at 2:46 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Wed, Dec 13, 2023 at 7:42 PM Mario Limonciello > > <mario.limonciello@amd.com> wrote: > > > > > > On 12/13/2023 12:38, Rafael J. Wysocki wrote: > > > > On Wed, Dec 13, 2023 at 7:27 PM Mario Limonciello > > > > <mario.limonciello@amd.com> wrote: > > > >> > > > >> When a system is being powered off it's important that PCIe ports > > > >> have been put into D3cold as there is no other software to turn > > > >> off the devices at S5. > > > >> > > > >> If PCIe ports are left in D0 then any GPIOs toggled by the ACPI > > > >> power resources may be left enabled and devices may consume excess > > > >> power. > > > > > > > > Isn't that a platform firmware issue? > > > > > > > > It is the responsibility of the platform firmware to properly put the > > > > platform into S5, including power removal from devices that are not > > > > armed for power-on. > > > > > > The specific issues that triggered this series were tied to the PCIe > > > ports for dGPUs. There is a GPIO that is toggled by _ON or _OFF. > > > > > > Windows calls _OFF as part of S5.. > > > > I see. > > > > > > > > > >> Cc: mpearson-lenovo@squebb.ca > > > >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > > >> --- > > > >> drivers/pci/pcie/portdrv.c | 11 ++++++++--- > > > >> 1 file changed, 8 insertions(+), 3 deletions(-) > > > >> > > > >> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c > > > >> index 14a4b89a3b83..08238680c481 100644 > > > >> --- a/drivers/pci/pcie/portdrv.c > > > >> +++ b/drivers/pci/pcie/portdrv.c > > > >> @@ -734,9 +734,14 @@ static void pcie_portdrv_remove(struct pci_dev *dev) > > > >> static void pcie_portdrv_shutdown(struct pci_dev *dev) > > > >> { > > > >> if (pci_bridge_d3_possible(dev)) { > > > >> - pm_runtime_forbid(&dev->dev); > > > >> - pm_runtime_get_noresume(&dev->dev); > > > >> - pm_runtime_dont_use_autosuspend(&dev->dev); > > > >> + /* whole hierarchy goes into a low power state for S5 */ > > > >> + if (system_state == SYSTEM_POWER_OFF) { > > > >> + pci_set_power_state(dev, PCI_D3cold); > > > >> + } else { > > > >> + pm_runtime_forbid(&dev->dev); > > > >> + pm_runtime_get_noresume(&dev->dev); > > > >> + pm_runtime_dont_use_autosuspend(&dev->dev); > > > >> + } > > > >> } > > > > > > > > Wouldn't it be better to remove power from the port after running the > > > > code below? > > > > > > > > > > Yes; I think you're right. I'll do some more testing with this. > > > > > > >> pcie_port_device_remove(dev); > > > >> -- > > > > IIRC, to do this all properly, you'd need to rework the shutdown path > > to look like the hibernation power-off one. Or even use the latter > > for shutdown? > > > > There was no reason to do that till now, so it has not been done, but > > it looks like you have one. > > > > I am working on exactly same thing but with a different approach. > Because this is needed for more than just PCI devices. > I haven't written a proper commit message yet, but the implementation > is quite simple: As I said, doing this properly requires something like the hibernation power-off transition to be carried out for S5. I think that the existing hibernation power-off code can be used as-is for this purpose even. > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c > index f007116a8427..b90c6cf6faf4 100644 > --- a/drivers/acpi/device_pm.c > +++ b/drivers/acpi/device_pm.c > @@ -967,15 +967,17 @@ EXPORT_SYMBOL_GPL(acpi_pm_set_device_wakeup); > * @adev: ACPI device node corresponding to @dev. > * @system_state: System state to choose the device state for. > */ > -static int acpi_dev_pm_low_power(struct device *dev, struct acpi_device *adev, > - u32 system_state) > +static int acpi_dev_pm_low_power(struct acpi_device *adev, void* data) > { > int ret, state; > + u32 *system_state = data; > > if (!acpi_device_power_manageable(adev)) > return 0; > > - ret = acpi_dev_pm_get_state(dev, adev, system_state, NULL, &state); > + acpi_dev_for_each_child(adev, acpi_dev_pm_low_power, data); > + > + ret = acpi_dev_pm_get_state(&adev->dev, adev, *system_state, NULL, &state); > return ret ? ret : acpi_device_set_power(adev, state); > } > > @@ -1016,7 +1018,7 @@ int acpi_dev_suspend(struct device *dev, bool wakeup) > wakeup = false; > } > > - error = acpi_dev_pm_low_power(dev, adev, target_state); > + error = acpi_dev_pm_low_power(adev, &target_state); > if (error && wakeup) > acpi_device_wakeup_disable(adev); > > @@ -1386,6 +1388,7 @@ static struct dev_pm_domain acpi_general_pm_domain = { > static void acpi_dev_pm_detach(struct device *dev, bool power_off) > { > struct acpi_device *adev = ACPI_COMPANION(dev); > + u32 state = ACPI_STATE_S0; > > if (adev && dev->pm_domain == &acpi_general_pm_domain) { > dev_pm_domain_set(dev, NULL); > @@ -1400,7 +1403,7 @@ static void acpi_dev_pm_detach(struct device > *dev, bool power_off) > dev_pm_qos_hide_latency_limit(dev); > dev_pm_qos_hide_flags(dev); > acpi_device_wakeup_disable(adev); > - acpi_dev_pm_low_power(dev, adev, ACPI_STATE_S0); > + acpi_dev_pm_low_power(adev, &state); > } > } > } > @@ -1514,4 +1517,16 @@ bool acpi_dev_state_d0(struct device *dev) > } > EXPORT_SYMBOL_GPL(acpi_dev_state_d0); > > +void acpi_dev_shutdown(struct device *dev) > +{ > + struct acpi_device *adev = ACPI_COMPANION(dev); > + u32 state = ACPI_STATE_S5; > + > + if (!adev) > + return; > + > + acpi_device_wakeup_disable(adev); > + acpi_dev_pm_low_power(adev, &state); > +} > +EXPORT_SYMBOL_GPL(acpi_dev_shutdown); > #endif /* CONFIG_PM */ > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 6ceaf50f5a67..7e7c99eade63 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -45,6 +45,15 @@ static void __fw_devlink_link_to_consumers(struct > device *dev); > static bool fw_devlink_drv_reg_done; > static bool fw_devlink_best_effort; > > +#ifdef CONFIG_ACPI > +static inline void fw_dev_shutdown(struct device *dev) > +{ > + acpi_dev_shutdown(dev); > +} > +#else > +static inline void fw_dev_shutdown(struct device *dev) { } > +#endif > + > /** > * __fwnode_link_add - Create a link between two fwnode_handles. > * @con: Consumer end of the link. > @@ -4780,6 +4789,8 @@ void device_shutdown(void) > dev->driver->shutdown(dev); > } > > + fw_dev_shutdown(dev); > + > device_unlock(dev); > if (parent) > device_unlock(parent); > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 641dc4843987..374f9eb75c22 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -1130,6 +1130,7 @@ int acpi_subsys_runtime_resume(struct device *dev); > int acpi_dev_pm_attach(struct device *dev, bool power_on); > bool acpi_storage_d3(struct device *dev); > bool acpi_dev_state_d0(struct device *dev); > +void acpi_dev_shutdown(struct device *dev); > #else > static inline int acpi_subsys_runtime_suspend(struct device *dev) { return 0; } > static inline int acpi_subsys_runtime_resume(struct device *dev) { return 0; } > @@ -1145,6 +1146,7 @@ static inline bool acpi_dev_state_d0(struct device *dev) > { > return true; > } > +static inline void acpi_dev_shutdown(struct device *dev) { } > #endif > > #if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP) >
On 12/14/2023 03:00, Rafael J. Wysocki wrote: > On Thu, Dec 14, 2023 at 4:46 AM Kai-Heng Feng > <kai.heng.feng@canonical.com> wrote: >> >> Hi Mario and Rafael, >> >> On Thu, Dec 14, 2023 at 2:46 AM Rafael J. Wysocki <rafael@kernel.org> wrote: >>> >>> On Wed, Dec 13, 2023 at 7:42 PM Mario Limonciello >>> <mario.limonciello@amd.com> wrote: >>>> >>>> On 12/13/2023 12:38, Rafael J. Wysocki wrote: >>>>> On Wed, Dec 13, 2023 at 7:27 PM Mario Limonciello >>>>> <mario.limonciello@amd.com> wrote: >>>>>> >>>>>> When a system is being powered off it's important that PCIe ports >>>>>> have been put into D3cold as there is no other software to turn >>>>>> off the devices at S5. >>>>>> >>>>>> If PCIe ports are left in D0 then any GPIOs toggled by the ACPI >>>>>> power resources may be left enabled and devices may consume excess >>>>>> power. >>>>> >>>>> Isn't that a platform firmware issue? >>>>> >>>>> It is the responsibility of the platform firmware to properly put the >>>>> platform into S5, including power removal from devices that are not >>>>> armed for power-on. >>>> >>>> The specific issues that triggered this series were tied to the PCIe >>>> ports for dGPUs. There is a GPIO that is toggled by _ON or _OFF. >>>> >>>> Windows calls _OFF as part of S5.. >>> >>> I see. >>> >>>>> >>>>>> Cc: mpearson-lenovo@squebb.ca >>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>>>>> --- >>>>>> drivers/pci/pcie/portdrv.c | 11 ++++++++--- >>>>>> 1 file changed, 8 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c >>>>>> index 14a4b89a3b83..08238680c481 100644 >>>>>> --- a/drivers/pci/pcie/portdrv.c >>>>>> +++ b/drivers/pci/pcie/portdrv.c >>>>>> @@ -734,9 +734,14 @@ static void pcie_portdrv_remove(struct pci_dev *dev) >>>>>> static void pcie_portdrv_shutdown(struct pci_dev *dev) >>>>>> { >>>>>> if (pci_bridge_d3_possible(dev)) { >>>>>> - pm_runtime_forbid(&dev->dev); >>>>>> - pm_runtime_get_noresume(&dev->dev); >>>>>> - pm_runtime_dont_use_autosuspend(&dev->dev); >>>>>> + /* whole hierarchy goes into a low power state for S5 */ >>>>>> + if (system_state == SYSTEM_POWER_OFF) { >>>>>> + pci_set_power_state(dev, PCI_D3cold); >>>>>> + } else { >>>>>> + pm_runtime_forbid(&dev->dev); >>>>>> + pm_runtime_get_noresume(&dev->dev); >>>>>> + pm_runtime_dont_use_autosuspend(&dev->dev); >>>>>> + } >>>>>> } >>>>> >>>>> Wouldn't it be better to remove power from the port after running the >>>>> code below? >>>>> >>>> >>>> Yes; I think you're right. I'll do some more testing with this. >>>> >>>>>> pcie_port_device_remove(dev); >>>>>> -- >>> >>> IIRC, to do this all properly, you'd need to rework the shutdown path >>> to look like the hibernation power-off one. Or even use the latter >>> for shutdown? >>> >>> There was no reason to do that till now, so it has not been done, but >>> it looks like you have one. >>> >> >> I am working on exactly same thing but with a different approach. >> Because this is needed for more than just PCI devices. >> I haven't written a proper commit message yet, but the implementation >> is quite simple: > > As I said, doing this properly requires something like the hibernation > power-off transition to be carried out for S5. > > I think that the existing hibernation power-off code can be used as-is > for this purpose even. > I feel Rafael is right here that unifying the hibernation and shutdown paths is the right direction. Our team just double checked the "unpatched" Linux S4 measurements on a system that otherwise had problems with S5 and they show the same decreases in power my patch series showed. KH, I'm going to be OOO for a while with the holidays around the corner and some personal time. If you end up working on some patches to unify the S4/S5 codepaths CC me on them and I'll look when I'm back from my leave. Thanks,
On Fri, Dec 15, 2023 at 12:44 AM Mario Limonciello <mario.limonciello@amd.com> wrote: > > On 12/14/2023 03:00, Rafael J. Wysocki wrote: > > On Thu, Dec 14, 2023 at 4:46 AM Kai-Heng Feng > > <kai.heng.feng@canonical.com> wrote: > >> > >> Hi Mario and Rafael, > >> > >> On Thu, Dec 14, 2023 at 2:46 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > >>> > >>> On Wed, Dec 13, 2023 at 7:42 PM Mario Limonciello > >>> <mario.limonciello@amd.com> wrote: > >>>> > >>>> On 12/13/2023 12:38, Rafael J. Wysocki wrote: > >>>>> On Wed, Dec 13, 2023 at 7:27 PM Mario Limonciello > >>>>> <mario.limonciello@amd.com> wrote: > >>>>>> > >>>>>> When a system is being powered off it's important that PCIe ports > >>>>>> have been put into D3cold as there is no other software to turn > >>>>>> off the devices at S5. > >>>>>> > >>>>>> If PCIe ports are left in D0 then any GPIOs toggled by the ACPI > >>>>>> power resources may be left enabled and devices may consume excess > >>>>>> power. > >>>>> > >>>>> Isn't that a platform firmware issue? > >>>>> > >>>>> It is the responsibility of the platform firmware to properly put the > >>>>> platform into S5, including power removal from devices that are not > >>>>> armed for power-on. > >>>> > >>>> The specific issues that triggered this series were tied to the PCIe > >>>> ports for dGPUs. There is a GPIO that is toggled by _ON or _OFF. > >>>> > >>>> Windows calls _OFF as part of S5.. > >>> > >>> I see. > >>> > >>>>> > >>>>>> Cc: mpearson-lenovo@squebb.ca > >>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > >>>>>> --- > >>>>>> drivers/pci/pcie/portdrv.c | 11 ++++++++--- > >>>>>> 1 file changed, 8 insertions(+), 3 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c > >>>>>> index 14a4b89a3b83..08238680c481 100644 > >>>>>> --- a/drivers/pci/pcie/portdrv.c > >>>>>> +++ b/drivers/pci/pcie/portdrv.c > >>>>>> @@ -734,9 +734,14 @@ static void pcie_portdrv_remove(struct pci_dev *dev) > >>>>>> static void pcie_portdrv_shutdown(struct pci_dev *dev) > >>>>>> { > >>>>>> if (pci_bridge_d3_possible(dev)) { > >>>>>> - pm_runtime_forbid(&dev->dev); > >>>>>> - pm_runtime_get_noresume(&dev->dev); > >>>>>> - pm_runtime_dont_use_autosuspend(&dev->dev); > >>>>>> + /* whole hierarchy goes into a low power state for S5 */ > >>>>>> + if (system_state == SYSTEM_POWER_OFF) { > >>>>>> + pci_set_power_state(dev, PCI_D3cold); > >>>>>> + } else { > >>>>>> + pm_runtime_forbid(&dev->dev); > >>>>>> + pm_runtime_get_noresume(&dev->dev); > >>>>>> + pm_runtime_dont_use_autosuspend(&dev->dev); > >>>>>> + } > >>>>>> } > >>>>> > >>>>> Wouldn't it be better to remove power from the port after running the > >>>>> code below? > >>>>> > >>>> > >>>> Yes; I think you're right. I'll do some more testing with this. > >>>> > >>>>>> pcie_port_device_remove(dev); > >>>>>> -- > >>> > >>> IIRC, to do this all properly, you'd need to rework the shutdown path > >>> to look like the hibernation power-off one. Or even use the latter > >>> for shutdown? > >>> > >>> There was no reason to do that till now, so it has not been done, but > >>> it looks like you have one. > >>> > >> > >> I am working on exactly same thing but with a different approach. > >> Because this is needed for more than just PCI devices. > >> I haven't written a proper commit message yet, but the implementation > >> is quite simple: > > > > As I said, doing this properly requires something like the hibernation > > power-off transition to be carried out for S5. > > > > I think that the existing hibernation power-off code can be used as-is > > for this purpose even. > > > > I feel Rafael is right here that unifying the hibernation and shutdown > paths is the right direction. Our team just double checked the > "unpatched" Linux S4 measurements on a system that otherwise had > problems with S5 and they show the same decreases in power my patch > series showed. I agree this is the right approach because Windows is using S4 for its "Fast Startup" feature. Is there any historical reason that .power_off and .shutdown are separated? And, should we take a step further and also unify the .remove callback? I recall there was a discussion on consolidating .shutdown and .remove but somehow it didn't take off. > > KH, > > I'm going to be OOO for a while with the holidays around the corner and > some personal time. If you end up working on some patches to unify the > S4/S5 codepaths CC me on them and I'll look when I'm back from my leave. Sure thing. This can take a while. Kai-Heng > > Thanks, >
diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c index 14a4b89a3b83..08238680c481 100644 --- a/drivers/pci/pcie/portdrv.c +++ b/drivers/pci/pcie/portdrv.c @@ -734,9 +734,14 @@ static void pcie_portdrv_remove(struct pci_dev *dev) static void pcie_portdrv_shutdown(struct pci_dev *dev) { if (pci_bridge_d3_possible(dev)) { - pm_runtime_forbid(&dev->dev); - pm_runtime_get_noresume(&dev->dev); - pm_runtime_dont_use_autosuspend(&dev->dev); + /* whole hierarchy goes into a low power state for S5 */ + if (system_state == SYSTEM_POWER_OFF) { + pci_set_power_state(dev, PCI_D3cold); + } else { + pm_runtime_forbid(&dev->dev); + pm_runtime_get_noresume(&dev->dev); + pm_runtime_dont_use_autosuspend(&dev->dev); + } } pcie_port_device_remove(dev);