Message ID | 20230606162321.34222-1-mario.limonciello@amd.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp3528396vqr; Tue, 6 Jun 2023 09:41:22 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5R5HGYSYRIkCN9E1cAB066/nJ/ab4X56eKv2L2uu4nWU/irsCpgnD0iCGc7z7DcrJENhYn X-Received: by 2002:a05:6214:5010:b0:629:58a7:9a98 with SMTP id jo16-20020a056214501000b0062958a79a98mr3400870qvb.53.1686069681949; Tue, 06 Jun 2023 09:41:21 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1686069681; cv=pass; d=google.com; s=arc-20160816; b=xfaPYxRQw2IR26zfTXawI+VzQvnhV1y+M/lz75Zki7FD/+VNImvQaFPnpSij6hMK6H fe+39GqiB0Igkk+aIWVm3ZbMGljnp4dcU587dmEGrBYNMjyiv8H6rCMCXPuZO0I4zYrD fqg1u90xSttFmyJO85zzfBnZz0cRLoHa4bZsHyJMGD8/j4+SOklxd63QeOvUahZa3n59 3CVHIQVhfd3yh4bnSoxlibyhuYEt4H5fHBl6G6noTlZ0LN2HLCU2y5fpL1fY9olAPRFA vPoYcLCI1keKwl9+0RbB3aXrCglitcYQj5JAO70FzFJ83jVugk9OtF0yL9xXvZpYdkKu Ps0w== 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 :message-id:date:subject:cc:to:from:dkim-signature; bh=hvaLigvBWC8xjoh3ywCjaq7FhlRykUwhuFVFH+4LNwI=; b=Pi3MRNnxrjXZLPXXHIibgWXLltHP4m3eoxwzBSAO7BUTSuvCj0EzTUt7vhB/kUG66J J6mKU0n1bswaTduxgj/yBg8UwB+ppwJheEeGEUkUHoeHN1I2XS6q/aBS9Tca7N+hkVtd ZjDNNZ5I2K/WB1XPdcLfz8r/JRsGIjpdH3qfDXqRBPSVIBLsMYchi11vLyK9uuKuCDvY BBISSqP7yoh7loJD4eOAdchpDh+CrYZWc+Cwn18Rl9zVILJKHnbxETvoPI0aOek/kj2H E2ep7ozhU8+lV47pHp9SIArn0g+0aZAzhnsHil+0fXws1yjmq13ZIe2GyAae5V7BvjjR PJtQ== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@amd.com header.s=selector1 header.b=voBbyO3G; 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::1:20 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 (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id b9-20020ac85bc9000000b003f228439ff2si6672201qtb.618.2023.06.06.09.40.53; Tue, 06 Jun 2023 09:41:21 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@amd.com header.s=selector1 header.b=voBbyO3G; 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::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=amd.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237503AbjFFQY0 (ORCPT <rfc822;xxoosimple@gmail.com> + 99 others); Tue, 6 Jun 2023 12:24:26 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57506 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237609AbjFFQYW (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 6 Jun 2023 12:24:22 -0400 Received: from NAM10-BN7-obe.outbound.protection.outlook.com (mail-bn7nam10on2061b.outbound.protection.outlook.com [IPv6:2a01:111:f400:7e8a::61b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 63F6C10EA; Tue, 6 Jun 2023 09:23:43 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=V7KOLY0qlkN7wbMp2H1xm5KI8NZ4TQtyLqLhldSXDMA6nifiYpKQspjQRZXkliPTwawj/de+9ditlvWX/va4joG2evJlkse4Fl+DHKeBP8+99B8ILb0x4kepHXE0UEH1Hc4d1eYcgMEroypeVc68MJLmrU8Rcj3ueqsf7qZObtp0XZcR/4GQY8wYmXaiYgt51Yx5+fPc67tMl109Y8JoxLHdzSASSVroBnWQ39UmdV4Wt3Rf52bfbkDj0MSCOvDm+vHyBOAJ6VBlJIcWBdvsyyWglv2uVuOGLgrZ+ePwY65LGiaO5S56qRT5wvOmzpjZiIjteI12Gr9PKq+O7sbMGw== 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=hvaLigvBWC8xjoh3ywCjaq7FhlRykUwhuFVFH+4LNwI=; b=mk9eKLIrN8ioyyIfDChRSdcsXAkpw1yVZkNWCuu5cLT4U02yG0UgCrRkstFCnx6ktTfdRcAr60Pq3QkM1yE56HCajzjdt5l40EDX/avFYvEevUyiBWSstZOHH2ShTHuWN46BE0d1Q57NUDzTD5MIA7wS1jq/Vpc18CIF67tUCE53sagkLZdj5qXBLNGTfmgnYVjSXm7NXOf8adszHsgbey7dc3guBaX+YxwBu+pBRFm5o0LH0nT6RprElzAoGj0qmNAdF3jtIrmagvelbv//v44iIj1WqApcd4qex4tKrixLI4Luw565xQ+nvEhgTHIBl6kJh0MEuPREZo22230CUw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=kernel.org 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 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=hvaLigvBWC8xjoh3ywCjaq7FhlRykUwhuFVFH+4LNwI=; b=voBbyO3GuE9zLHzG9YgAumYNnctKwQ5HGgaKW93rk4RNmEB+5uyOyfmb6XFMvCe7oPWMT0FuvRYhLt6W61/a1hALhdbc/MYLkqT+wkvxk1DRBrCWovGhH5jVjFU0Zci9uhiZsxAPfcVn4P4QAuRdgRKHTAS04qCZ7gHYxSC4Fzw= Received: from DM5PR07CA0092.namprd07.prod.outlook.com (2603:10b6:4:ae::21) by DM6PR12MB4563.namprd12.prod.outlook.com (2603:10b6:5:28e::10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6455.28; Tue, 6 Jun 2023 16:23:37 +0000 Received: from DM6NAM11FT054.eop-nam11.prod.protection.outlook.com (2603:10b6:4:ae:cafe::91) by DM5PR07CA0092.outlook.office365.com (2603:10b6:4:ae::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6477.19 via Frontend Transport; Tue, 6 Jun 2023 16:23:37 +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 DM6NAM11FT054.mail.protection.outlook.com (10.13.173.95) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.20.6455.33 via Frontend Transport; Tue, 6 Jun 2023 16:23:37 +0000 Received: from SITE-L-T34-2.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.2375.34; Tue, 6 Jun 2023 11:23:36 -0500 From: Mario Limonciello <mario.limonciello@amd.com> To: "Rafael J . Wysocki" <rafael@kernel.org>, Bjorn Helgaas <helgaas@kernel.org> CC: <linux-pci@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Len Brown <lenb@kernel.org>, <linux-acpi@vger.kernel.org>, Mario Limonciello <mario.limonciello@amd.com> Subject: [PATCH v2] PCI: Call _REG when saving/restoring PCI state Date: Tue, 6 Jun 2023 11:23:21 -0500 Message-ID: <20230606162321.34222-1-mario.limonciello@amd.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Originating-IP: [10.180.168.240] X-ClientProxiedBy: SATLEXMB04.amd.com (10.181.40.145) To SATLEXMB04.amd.com (10.181.40.145) X-EOPAttributedMessage: 0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: DM6NAM11FT054:EE_|DM6PR12MB4563:EE_ X-MS-Office365-Filtering-Correlation-Id: 1547660d-b622-4e79-9358-08db66aa61eb X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: +9Y+slEGwVywyysipWHbF4iY7Gjae8sFqySzaTmNuwl6PwSzuSxE2BNJ3CFMi5RrDU9Lpvwq/tXPwcEpnTBI4Gipn5sdv/N+aa31BVE6Tv64+7P72SSEkfwSRxZPE8SIg5mAoaksbBgr9HlXdbD43WBb66etRQhAMVDQzGrSw9yTv6aR1mARitR7jl5XZgFXCL/LGFvwi81LkRizKUh4dmD0RpSg8G6ZYqjbrpk7mPzh/f9sFHVZhODTV26IGa6d+IDdV3bHtFRGPxurlO889yNhaRYLmLI5CuEW6BMRKCgn92QrX5Ch5n0WuK11tZ8KZaiW3XaVtBPt+Wx/PcQLhqz3nsOSy/pxzbqMIvwirQFJAtssvcLN8l17DMmf61Pgd4owY1WkJWVj4hIFORNw0xJF695DLiKBMQrqY5DHnj8PKxKam0cr7jFGpv+mxN0ENrYBqQOEnVS6HobT4skmUm5rSMuKF6Ij3JyC2oYaiYrWpOPYGV+czF58EHtxQwbG8gN+5e6BXKm3mXGMctynEj0EUDDDiFrtCgMjwcsX+TVUcvc7CoZooWZcj02SSboplGef+6FgPQ03HQuZwmEQbRwd11Dp/l8Lak0rb+idPyVOf6Yv4yAe0aVKmtQLEbYzhgmJ12dfXYpm+AqXiRPeqL3VazSR1Ili6lei4Za09Ajk8cyZm0renwJOD1eVXy27GUv14+Sv8bcT/JrPpB1eFaqkilF18hpIGIdcbPttYsDY/bKCqTyAV+vXa8UQRdL3kMEKdw1eLGOw85+UGftir15C4imlZbeuYurjMkPbjz0= 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:(13230028)(4636009)(39860400002)(136003)(396003)(376002)(346002)(451199021)(36840700001)(40470700004)(46966006)(478600001)(41300700001)(316002)(54906003)(110136005)(44832011)(356005)(82740400003)(81166007)(5660300002)(2906002)(70206006)(4326008)(70586007)(8676002)(8936002)(6666004)(966005)(82310400005)(7696005)(40460700003)(86362001)(186003)(2616005)(36756003)(16526019)(26005)(36860700001)(40480700001)(426003)(83380400001)(336012)(1076003)(47076005)(21314003)(36900700001);DIR:OUT;SFP:1101; X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Jun 2023 16:23:37.5578 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 1547660d-b622-4e79-9358-08db66aa61eb 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: DM6NAM11FT054.eop-nam11.prod.protection.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4563 X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,SPF_HELO_PASS, SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1767594588921808779?= X-GMAIL-MSGID: =?utf-8?q?1767972202933044593?= |
Series |
[v2] PCI: Call _REG when saving/restoring PCI state
|
|
Commit Message
Mario Limonciello
June 6, 2023, 4:23 p.m. UTC
ASMedia PCIe GPIO controllers fail functional tests after returning from
suspend (S3 or s2idle). This is because the BIOS checks whether the
OSPM has called the `_REG` method to determine whether it can interact with
the OperationRegion assigned to the device.
As described in 6.5.4 in the APCI spec, `_REG` is used to inform the AML
code on the availability of an operation region.
To fix this issue, call acpi_evaluate_reg() when saving and restoring the
state of PCI devices.
Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#reg-region
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v1->v2:
* Handle case of no CONFIG_ACPI
* Rename function
* Update commit message
* Move ACPI calling code into pci-acpi.c instead
* Cite the ACPI spec
---
drivers/pci/pci-acpi.c | 10 ++++++++++
drivers/pci/pci.c | 14 ++++++++++++++
drivers/pci/pci.h | 2 ++
3 files changed, 26 insertions(+)
Comments
On Tue, Jun 6, 2023 at 6:23 PM Mario Limonciello <mario.limonciello@amd.com> wrote: > > ASMedia PCIe GPIO controllers fail functional tests after returning from > suspend (S3 or s2idle). This is because the BIOS checks whether the > OSPM has called the `_REG` method to determine whether it can interact with > the OperationRegion assigned to the device. > > As described in 6.5.4 in the APCI spec, `_REG` is used to inform the AML > code on the availability of an operation region. > > To fix this issue, call acpi_evaluate_reg() when saving and restoring the > state of PCI devices. > > Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#reg-region > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > v1->v2: > * Handle case of no CONFIG_ACPI > * Rename function > * Update commit message > * Move ACPI calling code into pci-acpi.c instead > * Cite the ACPI spec > --- > drivers/pci/pci-acpi.c | 10 ++++++++++ > drivers/pci/pci.c | 14 ++++++++++++++ > drivers/pci/pci.h | 2 ++ > 3 files changed, 26 insertions(+) > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index 1698205dd73c..abc8bcfc2c71 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -1209,6 +1209,16 @@ void acpi_pci_remove_bus(struct pci_bus *bus) > acpi_pci_slot_remove(bus); > } > > +void acpi_pci_set_register_access(struct pci_dev *dev, bool enable) > +{ > + int val = enable ? ACPI_REG_CONNECT : ACPI_REG_DISCONNECT; > + int ret = acpi_evaluate_reg(ACPI_HANDLE(&dev->dev), > + ACPI_ADR_SPACE_PCI_CONFIG, val); > + if (ret) > + pci_dbg(dev, "ACPI _REG %s evaluation failed (%d)\n", > + val ? "connect" : "disconnect", ret); s/val/enable/ ? Then I don't have to remember that ACPI_REG_DISCONNECT is 0. <bikeshedding> I would call this function something like acpi_pci_config_space_access(), because technically it is about allowing AML to access the PCI configuration space. </bikeshedding> > +} > + > /* ACPI bus type */ > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index e38c2f6eebd4..b2f1f603ec62 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -1068,6 +1068,14 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev) > return acpi_pci_bridge_d3(dev); > } > > +static inline void platform_set_register_access(struct pci_dev *dev, bool en) > +{ > + if (pci_use_mid_pm()) > + return; > + > + acpi_pci_set_register_access(dev, en); > +} > + > /** > * pci_update_current_state - Read power state of given device and cache it > * @dev: PCI device to handle. > @@ -1645,6 +1653,9 @@ static void pci_restore_ltr_state(struct pci_dev *dev) > int pci_save_state(struct pci_dev *dev) > { > int i; > + > + platform_set_register_access(dev, false); > + > /* XXX: 100% dword access ok here? */ > for (i = 0; i < 16; i++) { > pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]); > @@ -1790,6 +1801,8 @@ void pci_restore_state(struct pci_dev *dev) > pci_enable_acs(dev); > pci_restore_iov_state(dev); > > + platform_set_register_access(dev, true); > + > dev->state_saved = false; > } > EXPORT_SYMBOL(pci_restore_state); > @@ -3203,6 +3216,7 @@ void pci_pm_init(struct pci_dev *dev) > pci_read_config_word(dev, PCI_STATUS, &status); > if (status & PCI_STATUS_IMM_READY) > dev->imm_ready = 1; > + platform_set_register_access(dev, true); > } > > static unsigned long pci_ea_flags(struct pci_dev *dev, u8 prop) > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index ffccb03933e2..78961505aae2 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -703,6 +703,7 @@ void acpi_pci_refresh_power_state(struct pci_dev *dev); > int acpi_pci_wakeup(struct pci_dev *dev, bool enable); > bool acpi_pci_need_resume(struct pci_dev *dev); > pci_power_t acpi_pci_choose_state(struct pci_dev *pdev); > +void acpi_pci_set_register_access(struct pci_dev *dev, bool enable); > #else > static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe) > { > @@ -742,6 +743,7 @@ static inline pci_power_t acpi_pci_choose_state(struct pci_dev *pdev) > { > return PCI_POWER_ERROR; > } > +static inline void acpi_pci_set_register_access(struct pci_dev *dev, bool enable) {} > #endif > > #ifdef CONFIG_PCIEASPM > -- > 2.34.1 >
On 6/6/2023 1:21 PM, Rafael J. Wysocki wrote: > On Tue, Jun 6, 2023 at 6:23 PM Mario Limonciello > <mario.limonciello@amd.com> wrote: >> ASMedia PCIe GPIO controllers fail functional tests after returning from >> suspend (S3 or s2idle). This is because the BIOS checks whether the >> OSPM has called the `_REG` method to determine whether it can interact with >> the OperationRegion assigned to the device. >> >> As described in 6.5.4 in the APCI spec, `_REG` is used to inform the AML >> code on the availability of an operation region. >> >> To fix this issue, call acpi_evaluate_reg() when saving and restoring the >> state of PCI devices. >> >> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#reg-region >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> v1->v2: >> * Handle case of no CONFIG_ACPI >> * Rename function >> * Update commit message >> * Move ACPI calling code into pci-acpi.c instead >> * Cite the ACPI spec >> --- >> drivers/pci/pci-acpi.c | 10 ++++++++++ >> drivers/pci/pci.c | 14 ++++++++++++++ >> drivers/pci/pci.h | 2 ++ >> 3 files changed, 26 insertions(+) >> >> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c >> index 1698205dd73c..abc8bcfc2c71 100644 >> --- a/drivers/pci/pci-acpi.c >> +++ b/drivers/pci/pci-acpi.c >> @@ -1209,6 +1209,16 @@ void acpi_pci_remove_bus(struct pci_bus *bus) >> acpi_pci_slot_remove(bus); >> } >> >> +void acpi_pci_set_register_access(struct pci_dev *dev, bool enable) >> +{ >> + int val = enable ? ACPI_REG_CONNECT : ACPI_REG_DISCONNECT; >> + int ret = acpi_evaluate_reg(ACPI_HANDLE(&dev->dev), >> + ACPI_ADR_SPACE_PCI_CONFIG, val); >> + if (ret) >> + pci_dbg(dev, "ACPI _REG %s evaluation failed (%d)\n", >> + val ? "connect" : "disconnect", ret); > s/val/enable/ ? > Then I don't have to remember that ACPI_REG_DISCONNECT is 0. Sure, no problem. > > <bikeshedding> > > I would call this function something like > acpi_pci_config_space_access(), because technically it is about > allowing AML to access the PCI configuration space. > > </bikeshedding> Presumably platform_pci_config_space_access() below too. That's fine by me, Bjorn? >> +} >> + >> /* ACPI bus type */ >> >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index e38c2f6eebd4..b2f1f603ec62 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -1068,6 +1068,14 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev) >> return acpi_pci_bridge_d3(dev); >> } >> >> +static inline void platform_set_register_access(struct pci_dev *dev, bool en) >> +{ >> + if (pci_use_mid_pm()) >> + return; >> + >> + acpi_pci_set_register_access(dev, en); >> +} >> + >> /** >> * pci_update_current_state - Read power state of given device and cache it >> * @dev: PCI device to handle. >> @@ -1645,6 +1653,9 @@ static void pci_restore_ltr_state(struct pci_dev *dev) >> int pci_save_state(struct pci_dev *dev) >> { >> int i; >> + >> + platform_set_register_access(dev, false); >> + >> /* XXX: 100% dword access ok here? */ >> for (i = 0; i < 16; i++) { >> pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]); >> @@ -1790,6 +1801,8 @@ void pci_restore_state(struct pci_dev *dev) >> pci_enable_acs(dev); >> pci_restore_iov_state(dev); >> >> + platform_set_register_access(dev, true); >> + >> dev->state_saved = false; >> } >> EXPORT_SYMBOL(pci_restore_state); >> @@ -3203,6 +3216,7 @@ void pci_pm_init(struct pci_dev *dev) >> pci_read_config_word(dev, PCI_STATUS, &status); >> if (status & PCI_STATUS_IMM_READY) >> dev->imm_ready = 1; >> + platform_set_register_access(dev, true); >> } >> >> static unsigned long pci_ea_flags(struct pci_dev *dev, u8 prop) >> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h >> index ffccb03933e2..78961505aae2 100644 >> --- a/drivers/pci/pci.h >> +++ b/drivers/pci/pci.h >> @@ -703,6 +703,7 @@ void acpi_pci_refresh_power_state(struct pci_dev *dev); >> int acpi_pci_wakeup(struct pci_dev *dev, bool enable); >> bool acpi_pci_need_resume(struct pci_dev *dev); >> pci_power_t acpi_pci_choose_state(struct pci_dev *pdev); >> +void acpi_pci_set_register_access(struct pci_dev *dev, bool enable); >> #else >> static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe) >> { >> @@ -742,6 +743,7 @@ static inline pci_power_t acpi_pci_choose_state(struct pci_dev *pdev) >> { >> return PCI_POWER_ERROR; >> } >> +static inline void acpi_pci_set_register_access(struct pci_dev *dev, bool enable) {} >> #endif >> >> #ifdef CONFIG_PCIEASPM >> -- >> 2.34.1 >>
On Tue, Jun 06, 2023 at 11:23:21AM -0500, Mario Limonciello wrote: > ASMedia PCIe GPIO controllers fail functional tests after returning from > suspend (S3 or s2idle). This is because the BIOS checks whether the > OSPM has called the `_REG` method to determine whether it can interact with > the OperationRegion assigned to the device. > > As described in 6.5.4 in the APCI spec, `_REG` is used to inform the AML > code on the availability of an operation region. > > To fix this issue, call acpi_evaluate_reg() when saving and restoring the > state of PCI devices. > > Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#reg-region > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > v1->v2: > * Handle case of no CONFIG_ACPI > * Rename function > * Update commit message > * Move ACPI calling code into pci-acpi.c instead > * Cite the ACPI spec Thanks for the spec reference (s/APCI/ACPI/ and add the revision if you rev this (r6.5 is the latest, AFAIK) if you rev this). I don't see text in that section that connects S3 with _REG. If it's there, you might have to quote the relevant sentence or two in the commit log. You mentioned _REG being sort of a mutex to synchronize OSPM vs platform access; if there's spec language to that effect, let's cite it. Ideally we should have been able to read the PCI and ACPI specs and implement this without tripping over problem on this particular hardware. I'm looking for the text that enables that "clean-room" implementation. If the spec doesn't have that text, it's either a hole in the spec or a BIOS defect that depends on something the spec doesn't require. Doing this in pci_save_state() still seems wrong to me. For example, e1000_probe() calls pci_save_state(), but this is not part of suspend. IIUC, this patch will disconnect the opregion when we probe an e1000 NIC. Is that what you intend? > --- > drivers/pci/pci-acpi.c | 10 ++++++++++ > drivers/pci/pci.c | 14 ++++++++++++++ > drivers/pci/pci.h | 2 ++ > 3 files changed, 26 insertions(+) > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index 1698205dd73c..abc8bcfc2c71 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -1209,6 +1209,16 @@ void acpi_pci_remove_bus(struct pci_bus *bus) > acpi_pci_slot_remove(bus); > } > > +void acpi_pci_set_register_access(struct pci_dev *dev, bool enable) > +{ > + int val = enable ? ACPI_REG_CONNECT : ACPI_REG_DISCONNECT; > + int ret = acpi_evaluate_reg(ACPI_HANDLE(&dev->dev), > + ACPI_ADR_SPACE_PCI_CONFIG, val); > + if (ret) > + pci_dbg(dev, "ACPI _REG %s evaluation failed (%d)\n", > + val ? "connect" : "disconnect", ret); > +} > + > /* ACPI bus type */ > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index e38c2f6eebd4..b2f1f603ec62 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -1068,6 +1068,14 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev) > return acpi_pci_bridge_d3(dev); > } > > +static inline void platform_set_register_access(struct pci_dev *dev, bool en) > +{ > + if (pci_use_mid_pm()) > + return; > + > + acpi_pci_set_register_access(dev, en); > +} > + > /** > * pci_update_current_state - Read power state of given device and cache it > * @dev: PCI device to handle. > @@ -1645,6 +1653,9 @@ static void pci_restore_ltr_state(struct pci_dev *dev) > int pci_save_state(struct pci_dev *dev) > { > int i; > + > + platform_set_register_access(dev, false); > + > /* XXX: 100% dword access ok here? */ > for (i = 0; i < 16; i++) { > pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]); > @@ -1790,6 +1801,8 @@ void pci_restore_state(struct pci_dev *dev) > pci_enable_acs(dev); > pci_restore_iov_state(dev); > > + platform_set_register_access(dev, true); > + > dev->state_saved = false; > } > EXPORT_SYMBOL(pci_restore_state); > @@ -3203,6 +3216,7 @@ void pci_pm_init(struct pci_dev *dev) > pci_read_config_word(dev, PCI_STATUS, &status); > if (status & PCI_STATUS_IMM_READY) > dev->imm_ready = 1; > + platform_set_register_access(dev, true); > } > > static unsigned long pci_ea_flags(struct pci_dev *dev, u8 prop) > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index ffccb03933e2..78961505aae2 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -703,6 +703,7 @@ void acpi_pci_refresh_power_state(struct pci_dev *dev); > int acpi_pci_wakeup(struct pci_dev *dev, bool enable); > bool acpi_pci_need_resume(struct pci_dev *dev); > pci_power_t acpi_pci_choose_state(struct pci_dev *pdev); > +void acpi_pci_set_register_access(struct pci_dev *dev, bool enable); > #else > static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe) > { > @@ -742,6 +743,7 @@ static inline pci_power_t acpi_pci_choose_state(struct pci_dev *pdev) > { > return PCI_POWER_ERROR; > } > +static inline void acpi_pci_set_register_access(struct pci_dev *dev, bool enable) {} > #endif > > #ifdef CONFIG_PCIEASPM > -- > 2.34.1 >
On 6/6/2023 2:23 PM, Bjorn Helgaas wrote: > On Tue, Jun 06, 2023 at 11:23:21AM -0500, Mario Limonciello wrote: >> ASMedia PCIe GPIO controllers fail functional tests after returning from >> suspend (S3 or s2idle). This is because the BIOS checks whether the >> OSPM has called the `_REG` method to determine whether it can interact with >> the OperationRegion assigned to the device. >> >> As described in 6.5.4 in the APCI spec, `_REG` is used to inform the AML >> code on the availability of an operation region. >> >> To fix this issue, call acpi_evaluate_reg() when saving and restoring the >> state of PCI devices. >> >> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#reg-region >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> v1->v2: >> * Handle case of no CONFIG_ACPI >> * Rename function >> * Update commit message >> * Move ACPI calling code into pci-acpi.c instead >> * Cite the ACPI spec > Thanks for the spec reference (s/APCI/ACPI/ and add the revision if > you rev this (r6.5 is the latest, AFAIK) if you rev this). > > I don't see text in that section that connects S3 with _REG. If it's > there, you might have to quote the relevant sentence or two in the > commit log. I don't think there is anything the spec connecting this with S3. At least from my perspective S3 is the reason this was exposed but there is a deficiency that exists that _REG is not being called by Linux. I intend to re-word the commit message something to the effect of explaining what _REG does and why _REG should be called, along with citations. Then in another paragraph "Fixing this resolves an issue ...". > > You mentioned _REG being sort of a mutex to synchronize OSPM vs > platform access; if there's spec language to that effect, let's cite > it. That sentence I included was cited from the spec. > Ideally we should have been able to read the PCI and ACPI specs and > implement this without tripping over problem on this particular > hardware. I'm looking for the text that enables that "clean-room" > implementation. If the spec doesn't have that text, it's either a > hole in the spec or a BIOS defect that depends on something the spec > doesn't require. IMO both the spec and BIOS are correct, it's a Linux issue that _REG wasn't used. Hopefully disconnecting the issue as just an example will achieve what you're looking for. > > Doing this in pci_save_state() still seems wrong to me. For example, > e1000_probe() calls pci_save_state(), but this is not part of suspend. > IIUC, this patch will disconnect the opregion when we probe an e1000 > NIC. Is that what you intend? Thanks for pointing this one out. I was narrowly focused on callers in PCI core. This was a caller I wasn't aware of; I agree it doesn't make sense. I think pci_set_power_state() might be another good candidate to use. What do you think of this? Or can you suggest another call site? I'm hesitant to put this into PCI suspend/resume code, because I think this same issue could happen at runtime too. >> --- >> drivers/pci/pci-acpi.c | 10 ++++++++++ >> drivers/pci/pci.c | 14 ++++++++++++++ >> drivers/pci/pci.h | 2 ++ >> 3 files changed, 26 insertions(+) >> >> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c >> index 1698205dd73c..abc8bcfc2c71 100644 >> --- a/drivers/pci/pci-acpi.c >> +++ b/drivers/pci/pci-acpi.c >> @@ -1209,6 +1209,16 @@ void acpi_pci_remove_bus(struct pci_bus *bus) >> acpi_pci_slot_remove(bus); >> } >> >> +void acpi_pci_set_register_access(struct pci_dev *dev, bool enable) >> +{ >> + int val = enable ? ACPI_REG_CONNECT : ACPI_REG_DISCONNECT; >> + int ret = acpi_evaluate_reg(ACPI_HANDLE(&dev->dev), >> + ACPI_ADR_SPACE_PCI_CONFIG, val); >> + if (ret) >> + pci_dbg(dev, "ACPI _REG %s evaluation failed (%d)\n", >> + val ? "connect" : "disconnect", ret); >> +} >> + >> /* ACPI bus type */ >> >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index e38c2f6eebd4..b2f1f603ec62 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -1068,6 +1068,14 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev) >> return acpi_pci_bridge_d3(dev); >> } >> >> +static inline void platform_set_register_access(struct pci_dev *dev, bool en) >> +{ >> + if (pci_use_mid_pm()) >> + return; >> + >> + acpi_pci_set_register_access(dev, en); >> +} >> + >> /** >> * pci_update_current_state - Read power state of given device and cache it >> * @dev: PCI device to handle. >> @@ -1645,6 +1653,9 @@ static void pci_restore_ltr_state(struct pci_dev *dev) >> int pci_save_state(struct pci_dev *dev) >> { >> int i; >> + >> + platform_set_register_access(dev, false); >> + >> /* XXX: 100% dword access ok here? */ >> for (i = 0; i < 16; i++) { >> pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]); >> @@ -1790,6 +1801,8 @@ void pci_restore_state(struct pci_dev *dev) >> pci_enable_acs(dev); >> pci_restore_iov_state(dev); >> >> + platform_set_register_access(dev, true); >> + >> dev->state_saved = false; >> } >> EXPORT_SYMBOL(pci_restore_state); >> @@ -3203,6 +3216,7 @@ void pci_pm_init(struct pci_dev *dev) >> pci_read_config_word(dev, PCI_STATUS, &status); >> if (status & PCI_STATUS_IMM_READY) >> dev->imm_ready = 1; >> + platform_set_register_access(dev, true); >> } >> >> static unsigned long pci_ea_flags(struct pci_dev *dev, u8 prop) >> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h >> index ffccb03933e2..78961505aae2 100644 >> --- a/drivers/pci/pci.h >> +++ b/drivers/pci/pci.h >> @@ -703,6 +703,7 @@ void acpi_pci_refresh_power_state(struct pci_dev *dev); >> int acpi_pci_wakeup(struct pci_dev *dev, bool enable); >> bool acpi_pci_need_resume(struct pci_dev *dev); >> pci_power_t acpi_pci_choose_state(struct pci_dev *pdev); >> +void acpi_pci_set_register_access(struct pci_dev *dev, bool enable); >> #else >> static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe) >> { >> @@ -742,6 +743,7 @@ static inline pci_power_t acpi_pci_choose_state(struct pci_dev *pdev) >> { >> return PCI_POWER_ERROR; >> } >> +static inline void acpi_pci_set_register_access(struct pci_dev *dev, bool enable) {} >> #endif >> >> #ifdef CONFIG_PCIEASPM >> -- >> 2.34.1 >>
On Tue, Jun 06, 2023 at 02:40:45PM -0500, Limonciello, Mario wrote: > On 6/6/2023 2:23 PM, Bjorn Helgaas wrote: > > On Tue, Jun 06, 2023 at 11:23:21AM -0500, Mario Limonciello wrote: > > > ASMedia PCIe GPIO controllers fail functional tests after returning from > > > suspend (S3 or s2idle). This is because the BIOS checks whether the > > > OSPM has called the `_REG` method to determine whether it can interact with > > > the OperationRegion assigned to the device. > > > > > > As described in 6.5.4 in the APCI spec, `_REG` is used to inform the AML > > > code on the availability of an operation region. > > > > > > To fix this issue, call acpi_evaluate_reg() when saving and restoring the > > > state of PCI devices. > > > > > > Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#reg-region > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > > --- > > > v1->v2: > > > * Handle case of no CONFIG_ACPI > > > * Rename function > > > * Update commit message > > > * Move ACPI calling code into pci-acpi.c instead > > > * Cite the ACPI spec > > > > Thanks for the spec reference (s/APCI/ACPI/ and add the revision if > > you rev this (r6.5 is the latest, AFAIK) if you rev this). > > > > I don't see text in that section that connects S3 with _REG. If it's > > there, you might have to quote the relevant sentence or two in the > > commit log. > > I don't think there is anything the spec connecting this > with S3. At least from my perspective S3 is the reason > this was exposed but there is a deficiency that exists > that _REG is not being called by Linux. > > I intend to re-word the commit message something to the > effect of explaining what _REG does and why _REG should be > called, along with citations. > > Then in another paragraph "Fixing this resolves an issue ...". > > > You mentioned _REG being sort of a mutex to synchronize OSPM vs > > platform access; if there's spec language to that effect, let's cite > > it. > > That sentence I included was cited from the spec. If it's necessary to justify the commit, include the citation in the commit log. > > Ideally we should have been able to read the PCI and ACPI specs and > > implement this without tripping over problem on this particular > > hardware. I'm looking for the text that enables that "clean-room" > > implementation. If the spec doesn't have that text, it's either a > > hole in the spec or a BIOS defect that depends on something the spec > > doesn't require. > > IMO both the spec and BIOS are correct, it's a Linux > issue that _REG wasn't used. What tells Linux that _REG needs to be used here? If there's nothing that tells Linux to use _REG here, I claim it's a BIOS defect. I'm happy to be convinced otherwise; the way to convince me is to point to the spec. If it's a BIOS defect, it's fine to work around it, but we need to understand that, own up to it, and make the exact requirements very clear. Otherwise we're likely to break this in the future because future developers and maintainers will rely on the specs. > > Doing this in pci_save_state() still seems wrong to me. For example, > > e1000_probe() calls pci_save_state(), but this is not part of suspend. > > IIUC, this patch will disconnect the opregion when we probe an e1000 > > NIC. Is that what you intend? > > Thanks for pointing this one out. I was narrowly focused > on callers in PCI core. This was a caller I wasn't > aware of; I agree it doesn't make sense. > > I think pci_set_power_state() might be another good > candidate to use. What do you think of this? I can't suggest a call site because (1) I'm not a power management person, and (2) I don't think we have a clear statement of when it is required. This must be expressed in terms of PCI power state transitions, or at least something discoverable from a pci_dev, not "s2idle" or even "S3" because those are meaningless in the PCI context. Bjorn
On 6/6/2023 2:58 PM, Bjorn Helgaas wrote: > On Tue, Jun 06, 2023 at 02:40:45PM -0500, Limonciello, Mario wrote: >> On 6/6/2023 2:23 PM, Bjorn Helgaas wrote: >>> On Tue, Jun 06, 2023 at 11:23:21AM -0500, Mario Limonciello wrote: >>>> ASMedia PCIe GPIO controllers fail functional tests after returning from >>>> suspend (S3 or s2idle). This is because the BIOS checks whether the >>>> OSPM has called the `_REG` method to determine whether it can interact with >>>> the OperationRegion assigned to the device. >>>> >>>> As described in 6.5.4 in the APCI spec, `_REG` is used to inform the AML >>>> code on the availability of an operation region. >>>> >>>> To fix this issue, call acpi_evaluate_reg() when saving and restoring the >>>> state of PCI devices. >>>> >>>> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#reg-region >>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>>> --- >>>> v1->v2: >>>> * Handle case of no CONFIG_ACPI >>>> * Rename function >>>> * Update commit message >>>> * Move ACPI calling code into pci-acpi.c instead >>>> * Cite the ACPI spec >>> Thanks for the spec reference (s/APCI/ACPI/ and add the revision if >>> you rev this (r6.5 is the latest, AFAIK) if you rev this). >>> >>> I don't see text in that section that connects S3 with _REG. If it's >>> there, you might have to quote the relevant sentence or two in the >>> commit log. >> I don't think there is anything the spec connecting this >> with S3. At least from my perspective S3 is the reason >> this was exposed but there is a deficiency that exists >> that _REG is not being called by Linux. >> >> I intend to re-word the commit message something to the >> effect of explaining what _REG does and why _REG should be >> called, along with citations. >> >> Then in another paragraph "Fixing this resolves an issue ...". >> >>> You mentioned _REG being sort of a mutex to synchronize OSPM vs >>> platform access; if there's spec language to that effect, let's cite >>> it. >> That sentence I included was cited from the spec. > If it's necessary to justify the commit, include the citation in the > commit log. > >>> Ideally we should have been able to read the PCI and ACPI specs and >>> implement this without tripping over problem on this particular >>> hardware. I'm looking for the text that enables that "clean-room" >>> implementation. If the spec doesn't have that text, it's either a >>> hole in the spec or a BIOS defect that depends on something the spec >>> doesn't require. >> IMO both the spec and BIOS are correct, it's a Linux >> issue that _REG wasn't used. > What tells Linux that _REG needs to be used here? If there's nothing > that tells Linux to use _REG here, I claim it's a BIOS defect. I'm > happy to be convinced otherwise; the way to convince me is to point to > the spec. From the spec it says "control methods must assume all operation regions are inaccessible until the _REG(RegionSpace, 1) method is executed" It also points out the opposite: "Conversely, control methods must not access fields in operation regions when _REG method execution has not indicated that the operation region handler is ready." The ACPI spec doesn't refer to D3 in this context, but it does make an allusion to power off in an example case. "Also, when the host controller or bridge controller is turned off or disabled, PCI Config Space Operation Regions for child devices are no longer available. As such, ETH0’s _REG method will be run when it is turned off and will again be run when PCI1 is turned off." > > If it's a BIOS defect, it's fine to work around it, but we need to > understand that, own up to it, and make the exact requirements very > clear. Otherwise we're likely to break this in the future because > future developers and maintainers will rely on the specs. From my discussions with BIOS developers, this is entirely intended behavior based on the _REG section in the spec. >>> Doing this in pci_save_state() still seems wrong to me. For example, >>> e1000_probe() calls pci_save_state(), but this is not part of suspend. >>> IIUC, this patch will disconnect the opregion when we probe an e1000 >>> NIC. Is that what you intend? >> Thanks for pointing this one out. I was narrowly focused >> on callers in PCI core. This was a caller I wasn't >> aware of; I agree it doesn't make sense. >> >> I think pci_set_power_state() might be another good >> candidate to use. What do you think of this? > I can't suggest a call site because (1) I'm not a power management > person, and (2) I don't think we have a clear statement of when it is > required. This must be expressed in terms of PCI power state > transitions, or at least something discoverable from a pci_dev, not > "s2idle" or even "S3" because those are meaningless in the PCI > context. > > Bjorn Right; I'm with you on not putting it with a suspend transition. The spec indicates that control methods can't access the regions until _REG is called, so my leaning is to keep the call at init time, and then add another call for the D3 and D0 transitions which is why I think pci_set_power_state() is probably best.
On Tue, Jun 6, 2023 at 10:26 PM Limonciello, Mario <mario.limonciello@amd.com> wrote: > > > On 6/6/2023 2:58 PM, Bjorn Helgaas wrote: > > On Tue, Jun 06, 2023 at 02:40:45PM -0500, Limonciello, Mario wrote: > >> On 6/6/2023 2:23 PM, Bjorn Helgaas wrote: > >>> On Tue, Jun 06, 2023 at 11:23:21AM -0500, Mario Limonciello wrote: > >>>> ASMedia PCIe GPIO controllers fail functional tests after returning from > >>>> suspend (S3 or s2idle). This is because the BIOS checks whether the > >>>> OSPM has called the `_REG` method to determine whether it can interact with > >>>> the OperationRegion assigned to the device. > >>>> > >>>> As described in 6.5.4 in the APCI spec, `_REG` is used to inform the AML > >>>> code on the availability of an operation region. > >>>> > >>>> To fix this issue, call acpi_evaluate_reg() when saving and restoring the > >>>> state of PCI devices. > >>>> > >>>> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#reg-region > >>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > >>>> --- > >>>> v1->v2: > >>>> * Handle case of no CONFIG_ACPI > >>>> * Rename function > >>>> * Update commit message > >>>> * Move ACPI calling code into pci-acpi.c instead > >>>> * Cite the ACPI spec > >>> Thanks for the spec reference (s/APCI/ACPI/ and add the revision if > >>> you rev this (r6.5 is the latest, AFAIK) if you rev this). > >>> > >>> I don't see text in that section that connects S3 with _REG. If it's > >>> there, you might have to quote the relevant sentence or two in the > >>> commit log. > >> I don't think there is anything the spec connecting this > >> with S3. At least from my perspective S3 is the reason > >> this was exposed but there is a deficiency that exists > >> that _REG is not being called by Linux. > >> > >> I intend to re-word the commit message something to the > >> effect of explaining what _REG does and why _REG should be > >> called, along with citations. > >> > >> Then in another paragraph "Fixing this resolves an issue ...". > >> > >>> You mentioned _REG being sort of a mutex to synchronize OSPM vs > >>> platform access; if there's spec language to that effect, let's cite > >>> it. > >> That sentence I included was cited from the spec. > > If it's necessary to justify the commit, include the citation in the > > commit log. > > > >>> Ideally we should have been able to read the PCI and ACPI specs and > >>> implement this without tripping over problem on this particular > >>> hardware. I'm looking for the text that enables that "clean-room" > >>> implementation. If the spec doesn't have that text, it's either a > >>> hole in the spec or a BIOS defect that depends on something the spec > >>> doesn't require. > >> IMO both the spec and BIOS are correct, it's a Linux > >> issue that _REG wasn't used. > > What tells Linux that _REG needs to be used here? If there's nothing > > that tells Linux to use _REG here, I claim it's a BIOS defect. I'm > > happy to be convinced otherwise; the way to convince me is to point to > > the spec. > From the spec it says "control methods must assume > all operation regions are inaccessible until the > _REG(RegionSpace, 1) method is executed" > > It also points out the opposite: "Conversely, > control methods must not access fields in > operation regions when _REG method execution > has not indicated that the operation region > handler is ready." > > The ACPI spec doesn't refer to D3 in this context, but > it does make an allusion to power off in an example case. > > "Also, when the host controller or bridge controller > is turned off or disabled, PCI Config Space Operation > Regions for child devices are no longer available. > As such, ETH0’s _REG method will be run when it is > turned off and will again be run when PCI1 is > turned off." > > > > > If it's a BIOS defect, it's fine to work around it, but we need to > > understand that, own up to it, and make the exact requirements very > > clear. Otherwise we're likely to break this in the future because > > future developers and maintainers will rely on the specs. > From my discussions with BIOS developers, this is entirely > intended behavior based on the _REG section in the spec. > >>> Doing this in pci_save_state() still seems wrong to me. For example, > >>> e1000_probe() calls pci_save_state(), but this is not part of suspend. > >>> IIUC, this patch will disconnect the opregion when we probe an e1000 > >>> NIC. Is that what you intend? > >> Thanks for pointing this one out. I was narrowly focused > >> on callers in PCI core. This was a caller I wasn't > >> aware of; I agree it doesn't make sense. > >> > >> I think pci_set_power_state() might be another good > >> candidate to use. What do you think of this? > > I can't suggest a call site because (1) I'm not a power management > > person, and (2) I don't think we have a clear statement of when it is > > required. This must be expressed in terms of PCI power state > > transitions, or at least something discoverable from a pci_dev, not > > "s2idle" or even "S3" because those are meaningless in the PCI > > context. > > > > Bjorn > Right; I'm with you on not putting it with a suspend > transition. > > The spec indicates that control methods can't access > the regions until _REG is called, so > my leaning is to keep the call at init time, and > then add another call for the D3 and D0 transitions > which is why I think pci_set_power_state() is probably > best. Except that it is not used in all transitions; see the callers oif pci_power_up() for example. Technically, the config space should become accessible right after acpi_pci_set_power_state() has transitioned the device into D0 and it should be accessible still right before acpi_pci_set_power_state() attempts to transition the device into a low-power state, so it looks like _REG could be evaluated from there.
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index 1698205dd73c..abc8bcfc2c71 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -1209,6 +1209,16 @@ void acpi_pci_remove_bus(struct pci_bus *bus) acpi_pci_slot_remove(bus); } +void acpi_pci_set_register_access(struct pci_dev *dev, bool enable) +{ + int val = enable ? ACPI_REG_CONNECT : ACPI_REG_DISCONNECT; + int ret = acpi_evaluate_reg(ACPI_HANDLE(&dev->dev), + ACPI_ADR_SPACE_PCI_CONFIG, val); + if (ret) + pci_dbg(dev, "ACPI _REG %s evaluation failed (%d)\n", + val ? "connect" : "disconnect", ret); +} + /* ACPI bus type */ diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index e38c2f6eebd4..b2f1f603ec62 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1068,6 +1068,14 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev) return acpi_pci_bridge_d3(dev); } +static inline void platform_set_register_access(struct pci_dev *dev, bool en) +{ + if (pci_use_mid_pm()) + return; + + acpi_pci_set_register_access(dev, en); +} + /** * pci_update_current_state - Read power state of given device and cache it * @dev: PCI device to handle. @@ -1645,6 +1653,9 @@ static void pci_restore_ltr_state(struct pci_dev *dev) int pci_save_state(struct pci_dev *dev) { int i; + + platform_set_register_access(dev, false); + /* XXX: 100% dword access ok here? */ for (i = 0; i < 16; i++) { pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]); @@ -1790,6 +1801,8 @@ void pci_restore_state(struct pci_dev *dev) pci_enable_acs(dev); pci_restore_iov_state(dev); + platform_set_register_access(dev, true); + dev->state_saved = false; } EXPORT_SYMBOL(pci_restore_state); @@ -3203,6 +3216,7 @@ void pci_pm_init(struct pci_dev *dev) pci_read_config_word(dev, PCI_STATUS, &status); if (status & PCI_STATUS_IMM_READY) dev->imm_ready = 1; + platform_set_register_access(dev, true); } static unsigned long pci_ea_flags(struct pci_dev *dev, u8 prop) diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index ffccb03933e2..78961505aae2 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -703,6 +703,7 @@ void acpi_pci_refresh_power_state(struct pci_dev *dev); int acpi_pci_wakeup(struct pci_dev *dev, bool enable); bool acpi_pci_need_resume(struct pci_dev *dev); pci_power_t acpi_pci_choose_state(struct pci_dev *pdev); +void acpi_pci_set_register_access(struct pci_dev *dev, bool enable); #else static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe) { @@ -742,6 +743,7 @@ static inline pci_power_t acpi_pci_choose_state(struct pci_dev *pdev) { return PCI_POWER_ERROR; } +static inline void acpi_pci_set_register_access(struct pci_dev *dev, bool enable) {} #endif #ifdef CONFIG_PCIEASPM