Message ID | 20230111203116.4896-1-vidyas@nvidia.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4e01:0:0:0:0:0 with SMTP id p1csp3525214wrt; Wed, 11 Jan 2023 12:36:33 -0800 (PST) X-Google-Smtp-Source: AMrXdXuDQIjpI9yEH47jbOn5q2vpbvqFQIMITC/7s9YSQVLEbwoIcheLPKB0iacK0kESTo+pNnFZ X-Received: by 2002:aa7:c907:0:b0:499:b672:ee3a with SMTP id b7-20020aa7c907000000b00499b672ee3amr9795705edt.12.1673469393071; Wed, 11 Jan 2023 12:36:33 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1673469393; cv=pass; d=google.com; s=arc-20160816; b=yY+5om3bkHcDsBpQGEPzX6BEr3jY+SXNVt+L9jBVMrpRiEi3IZ0P5v4p10ufiJ883N pZuNsF4R/dvw2ni5mcf7b2p67weVJI5CjBJw9zMn3uTEjVGbswAzRhs4v+nn1ZU04pJO Gc2gJ/FntRTlTdbMpUZvdiIQlS6wZQAukxaJmQnXvdRNW9wnY9lwD5XR8WpyQfzBB1rH qea2fadzSEyUxYdvqKp8syXKKWj9fbDzrg2rIjurs+Q5029Nyg8svqIXjvVmuiiZEjQZ X+Xg1cJ1cPskktuBmX8A82d5pB2gnKBKLDZ12kpOuPGyTwLg+E9z56r5YonYqbelo5Ow phcw== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:message-id:date:subject:cc:to:from :dkim-signature; bh=VJfTbG/Qv88ipqQH0Jw+ZamckaKF6FdA2RU/JpQhjv4=; b=jUQGkhrNVxPZF7OToL72I4oyq4tX/xLgGzPgo8+TaFJSLeGFHgtxljf/bPryuLVu/c 0l02LM+DJDCCPlWA2+mSmVAizHjPZ7H6HdOf1dE9fw8KQS1L9Ee0iT4K3bzX2Y9K1Ixd LuEId3jts70XpQseaxzBQJhiGqWg08RZi4oiXq9V5qHoy6KL6WMtcumRjgfua4XA7WpM fv6RdvjOSSjinnKmDgj1RevL7fxVsOM5UUTZYEVVDaJqDcVONWbEQzDdZJurJ61Qq1eh EuOzGnbMS3n78in/Hkctapo3FiYn9XPtaFO2dUKudtrZq2vOBwtnyKiHbKdBIA1UEc8u PGHQ== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@Nvidia.com header.s=selector2 header.b=GhcvYnTC; arc=pass (i=1 spf=pass spfdomain=nvidia.com dmarc=pass fromdomain=nvidia.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=REJECT sp=REJECT dis=NONE) header.from=nvidia.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id f8-20020a0564021e8800b00483dc6852f8si17440136edf.406.2023.01.11.12.36.08; Wed, 11 Jan 2023 12:36:33 -0800 (PST) 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=@Nvidia.com header.s=selector2 header.b=GhcvYnTC; arc=pass (i=1 spf=pass spfdomain=nvidia.com dmarc=pass fromdomain=nvidia.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=REJECT sp=REJECT dis=NONE) header.from=nvidia.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231175AbjAKUbi (ORCPT <rfc822;syz17693488234@gmail.com> + 99 others); Wed, 11 Jan 2023 15:31:38 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48376 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230047AbjAKUbg (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 11 Jan 2023 15:31:36 -0500 Received: from NAM12-DM6-obe.outbound.protection.outlook.com (mail-dm6nam12on2071.outbound.protection.outlook.com [40.107.243.71]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6169E2AC2; Wed, 11 Jan 2023 12:31:34 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Je1L2GwZi78LbxOivJG8ikF07l+JMva0/M9wrOQJG/DPA8j/Y+vEQTGpK+5xloNq56R6FYPuc7lcN5ZRBtP2tnLP3lE29meUYzh0TdMJU7AcASik6SUVcPgI7erQGR92wCCk1k6SDKXGQQ0V0bJdzmZuRAeB2630DV2Im9Q3TbgcmfO/e7m7S6IVSRtsa/ESabDiufqrwMHqdwlNq0Mw5E5VX7ecHON6w8itfyYZCkJV4x/FLIBsfPmlwGTPjl1Jwe1AAAkx820THmRxU6AvaAWXJVDa/nJ3MaNAx1kpa0Vprcy1x/HmYgESdGUxDg8DXLlGYVvjFtdFBlojRYehrA== 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=VJfTbG/Qv88ipqQH0Jw+ZamckaKF6FdA2RU/JpQhjv4=; b=Yto4ky3rJeEmNUFiwHMe/bFYCeTyG/NDyRIFC7InJ8gYin21XRQWgqG9sVUtkDNWVMQz+krFwarPdWVIAeQWN83VSO7Y1Fsvf933GiXr4yTqnENBJtr1fS4OmLLO9m+hlWjXigf1JBIKcqK7m8wdCTnCQVkQ95nx/SYtokSEy1mmQWJHMB61Grc3rc97SVs6+gfzllSb5PjVQBphX8hxsPR1B3VBbdcWcH+cXycGGKdzoTMEwlm6LCbO/UH9avOig+k/G/oljlCfjxyo8LgjCVpB8mByupZztaGUmyFnuhSbDrOGe0EKl1hajKyjIMVD2ltLEeuAoDJbYQB8zhmU2A== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 216.228.117.160) smtp.rcpttodomain=google.com smtp.mailfrom=nvidia.com; dmarc=pass (p=reject sp=reject pct=100) action=none header.from=nvidia.com; dkim=none (message not signed); arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Nvidia.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=VJfTbG/Qv88ipqQH0Jw+ZamckaKF6FdA2RU/JpQhjv4=; b=GhcvYnTCDRUOXDR78HFk6QTXYkAi9slDiYJR8MpxAkx/4VU+x6oyrOFRb94k4MRlgonW5+g/xJcFXF/oBin5qSHzKJAuJ9Bgh7oghCi4CG8KKVQ4ZJFc+eF1gZcBkPjTfSTQuC0qt7vh1UXW7oknwtgL7rLjcsZoLn2wUzOhuamzpiNrqGFXU0yA+uNec4yK+WvbfrhOkBED6dSG1c0hN/tZ+mV9G0qR2oz9x0R7cbJXaIGylP0YySOLSz2Gqo2dSkdDBPHy74VPfLsnfrtiiEQWA1N/XkVZSAPCrag0xTMJAcDdAl/bmG+yUSO64QkWio2z3qAk5EqwYiXhh/e39g== Received: from BN0PR07CA0030.namprd07.prod.outlook.com (2603:10b6:408:141::13) by IA1PR12MB6385.namprd12.prod.outlook.com (2603:10b6:208:38b::6) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5986.18; Wed, 11 Jan 2023 20:31:32 +0000 Received: from BN8NAM11FT025.eop-nam11.prod.protection.outlook.com (2603:10b6:408:141:cafe::5f) by BN0PR07CA0030.outlook.office365.com (2603:10b6:408:141::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6002.13 via Frontend Transport; Wed, 11 Jan 2023 20:31:32 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 216.228.117.160) smtp.mailfrom=nvidia.com; dkim=none (message not signed) header.d=none;dmarc=pass action=none header.from=nvidia.com; Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates 216.228.117.160 as permitted sender) receiver=protection.outlook.com; client-ip=216.228.117.160; helo=mail.nvidia.com; pr=C Received: from mail.nvidia.com (216.228.117.160) by BN8NAM11FT025.mail.protection.outlook.com (10.13.177.136) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6002.13 via Frontend Transport; Wed, 11 Jan 2023 20:31:32 +0000 Received: from rnnvmail201.nvidia.com (10.129.68.8) by mail.nvidia.com (10.129.200.66) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.36; Wed, 11 Jan 2023 12:31:22 -0800 Received: from rnnvmail202.nvidia.com (10.129.68.7) by rnnvmail201.nvidia.com (10.129.68.8) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.36; Wed, 11 Jan 2023 12:31:22 -0800 Received: from vidyas-desktop.nvidia.com (10.127.8.9) by mail.nvidia.com (10.129.68.7) with Microsoft SMTP Server id 15.2.986.36 via Frontend Transport; Wed, 11 Jan 2023 12:31:18 -0800 From: Vidya Sagar <vidyas@nvidia.com> To: <bhelgaas@google.com>, <ruscur@russell.cc>, <oohall@gmail.com>, <treding@nvidia.com>, <jonathanh@nvidia.com> CC: <linux-pci@vger.kernel.org>, <linuxppc-dev@lists.ozlabs.org>, <linux-kernel@vger.kernel.org>, <vsethi@nvidia.com>, <kthota@nvidia.com>, <mmaddireddy@nvidia.com>, <vidyas@nvidia.com>, <sagar.tv@gmail.com> Subject: [PATCH V1] PCI/AER: Configure ECRC only AER is native Date: Thu, 12 Jan 2023 02:01:16 +0530 Message-ID: <20230111203116.4896-1-vidyas@nvidia.com> X-Mailer: git-send-email 2.17.1 X-NVConfidentiality: public MIME-Version: 1.0 Content-Type: text/plain X-EOPAttributedMessage: 0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: BN8NAM11FT025:EE_|IA1PR12MB6385:EE_ X-MS-Office365-Filtering-Correlation-Id: 3c7be6e6-03a2-417a-684d-08daf412d3e4 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: ui3JpQf3jPfI+bEj5ddrbHZoCUkNA+0ZjhmAO6vkGbUmCCSkjN8t99Q+5SlEyTCFonsNrvs45ZVnjYc14Q4PunKMlP7IWMSmt3ghKmOlMh3Vr+GgLSyywxtMGKnaeO+gYWNr9+4NVBU3oIhutzncu87kUQP+8WUbAh9hFppJKadO0BhR8k7unc1mdPEtaPsDQNxYLg7aEUrVp/6xOjk6YDuShsmGAC1ZvIFhgLmF9YlRoelbNGxxlhC8EWwOie6wjeN+2pGsHNtG06cKeD47pXMZtVEueKUP3fJLFFZe8b2Ws+bPrgTrz4uWH29VB+uW0yepWZ/UWHdYGi1R8VnFL6TZk1P2uhn+3nbHmw13Q5SXnxA+JkDn1vnKNMIcDqV6fGgSlSWtUz/BShFbOqVO2Oaj6Qq7rJTWISxavMWWkwqPP/M3DbgFAllgIvw4CNT0fh5NiKkwaQvCpre9tEMFI931ABRDopSArOahTXxN42w6ocbYU6bpDDMJ0ROqOEjziBYnUMpkZP8sghkV7Xw2a8AQ5XpXOQ3kpc2OyosuSxGTN7MfBXS8judggBHQmqlyussS8HB38giJnFweNr8KiJiw1fZXjmniIcm//kmGDh0aEzv2e4VQrVdrLlHO4glBbJkvJlfgY4XwU9kIUvAFgabdk+dliydgCtdQGGrUTF0dkydMr0z0jW7tCZjjA69U3PLFcEd0H4R8POfJvQRM2A== X-Forefront-Antispam-Report: CIP:216.228.117.160;CTRY:US;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:mail.nvidia.com;PTR:dc6edge1.nvidia.com;CAT:NONE;SFS:(13230022)(4636009)(346002)(136003)(376002)(39860400002)(396003)(451199015)(40470700004)(36840700001)(46966006)(36756003)(7696005)(40480700001)(54906003)(316002)(86362001)(110136005)(6636002)(8676002)(2906002)(478600001)(70586007)(41300700001)(70206006)(4326008)(8936002)(82740400003)(356005)(5660300002)(40460700003)(4744005)(2616005)(36860700001)(1076003)(7636003)(186003)(336012)(26005)(47076005)(426003)(82310400005);DIR:OUT;SFP:1101; X-OriginatorOrg: Nvidia.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Jan 2023 20:31:32.5865 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 3c7be6e6-03a2-417a-684d-08daf412d3e4 X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.160];Helo=[mail.nvidia.com] X-MS-Exchange-CrossTenant-AuthSource: BN8NAM11FT025.eop-nam11.prod.protection.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB6385 X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO, RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE 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?1754759842558693938?= X-GMAIL-MSGID: =?utf-8?q?1754759842558693938?= |
Series |
[V1] PCI/AER: Configure ECRC only AER is native
|
|
Commit Message
Vidya Sagar
Jan. 11, 2023, 8:31 p.m. UTC
As the ECRC configuration bits are part of AER registers, configure
ECRC only if AER is natively owned by the kernel.
Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
drivers/pci/pcie/aer.c | 3 +++
1 file changed, 3 insertions(+)
Comments
On 1/11/23 12:31 PM, Vidya Sagar wrote: > As the ECRC configuration bits are part of AER registers, configure > ECRC only if AER is natively owned by the kernel. ecrc command line option takes "bios/on/off" as possible options. It does not clarify whether "on/off" choices can only be used if AER is owned by OS or it can override the ownership of ECRC configuration similar to pcie_ports=native option. Maybe that needs to be clarified. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > drivers/pci/pcie/aer.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index e2d8a74f83c3..730b47bdcdef 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -184,6 +184,9 @@ static int disable_ecrc_checking(struct pci_dev *dev) > */ > void pcie_set_ecrc_checking(struct pci_dev *dev) > { > + if (!pcie_aer_is_native(dev)) > + return; > + > switch (ecrc_policy) { > case ECRC_POLICY_DEFAULT: > return;
On Wed, Jan 11, 2023 at 01:42:21PM -0800, Sathyanarayanan Kuppuswamy wrote: > On 1/11/23 12:31 PM, Vidya Sagar wrote: > > As the ECRC configuration bits are part of AER registers, configure > > ECRC only if AER is natively owned by the kernel. > > ecrc command line option takes "bios/on/off" as possible options. It > does not clarify whether "on/off" choices can only be used if AER is > owned by OS or it can override the ownership of ECRC configuration > similar to pcie_ports=native option. Maybe that needs to be clarified. Good point, what do you think of an update like this: diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 6cfa6e3996cf..f7b40a439194 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -4296,7 +4296,9 @@ specified, e.g., 12@pci:8086:9c22:103c:198f for 4096-byte alignment. ecrc= Enable/disable PCIe ECRC (transaction layer - end-to-end CRC checking). + end-to-end CRC checking). Only effective + if OS has native AER control (either granted by + ACPI _OSC or forced via "pcie_ports=native"). bios: Use BIOS/firmware settings. This is the the default. off: Turn ECRC off I don't know whether the "ecrc=" parameter is really needed. If we were adding it today, I would ask "why not enable ECRC wherever it is supported?" If there are devices where it's broken, we could always add quirks to disable it on a case-by-case basis. But I think the patch below is the right thing to do for now. Vidya, did you trip over an issue because of this, e.g., a conflict between firmware use of AER and Linux use of it? If so, maybe we could mention a symptom on the commit log. But my guess is you probably found this by inspection. Bjorn > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > --- > > drivers/pci/pcie/aer.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > > index e2d8a74f83c3..730b47bdcdef 100644 > > --- a/drivers/pci/pcie/aer.c > > +++ b/drivers/pci/pcie/aer.c > > @@ -184,6 +184,9 @@ static int disable_ecrc_checking(struct pci_dev *dev) > > */ > > void pcie_set_ecrc_checking(struct pci_dev *dev) > > { > > + if (!pcie_aer_is_native(dev)) > > + return; > > + > > switch (ecrc_policy) { > > case ECRC_POLICY_DEFAULT: > > return; > > -- > Sathyanarayanan Kuppuswamy > Linux Kernel Developer
Hi, On 1/11/23 3:10 PM, Bjorn Helgaas wrote: > On Wed, Jan 11, 2023 at 01:42:21PM -0800, Sathyanarayanan Kuppuswamy wrote: >> On 1/11/23 12:31 PM, Vidya Sagar wrote: >>> As the ECRC configuration bits are part of AER registers, configure >>> ECRC only if AER is natively owned by the kernel. >> >> ecrc command line option takes "bios/on/off" as possible options. It >> does not clarify whether "on/off" choices can only be used if AER is >> owned by OS or it can override the ownership of ECRC configuration >> similar to pcie_ports=native option. Maybe that needs to be clarified. > > Good point, what do you think of an update like this: > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 6cfa6e3996cf..f7b40a439194 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -4296,7 +4296,9 @@ > specified, e.g., 12@pci:8086:9c22:103c:198f > for 4096-byte alignment. > ecrc= Enable/disable PCIe ECRC (transaction layer > - end-to-end CRC checking). > + end-to-end CRC checking). Only effective > + if OS has native AER control (either granted by > + ACPI _OSC or forced via "pcie_ports=native"). > bios: Use BIOS/firmware settings. This is the > the default. > off: Turn ECRC off Looks fine. But do we even need "bios" option? Since it is the default value, I am not sure why we need to list that as an option again. IMO this could be removed. > > I don't know whether the "ecrc=" parameter is really needed. If we > were adding it today, I would ask "why not enable ECRC wherever it is > supported?" If there are devices where it's broken, we could always > add quirks to disable it on a case-by-case basis. Checking the original patch which added it, it looks like the intention is to give option to boost performance over integrity. commit 43c16408842b0eeb367c23a6fa540ce69f99e347 Author: Andrew Patterson <andrew.patterson@hp.com> Date: Wed Apr 22 16:52:09 2009 -0600 PCI: Add support for turning PCIe ECRC on or off Adds support for PCI Express transaction layer end-to-end CRC checking (ECRC). This patch will enable/disable ECRC checking by setting/clearing the ECRC Check Enable and/or ECRC Generation Enable bits for devices that support ECRC. The ECRC setting is controlled by the "pci=ecrc=<policy>" command-line option. If this option is not set or is set to 'bios", the enable and generation bits are left in whatever state that firmware/BIOS set them to. The "off" setting turns them off, and the "on" option turns them on (if the device supports it). Turning ECRC on or off can be a data integrity versus performance tradeoff. In theory, turning it on will catch more data errors, turning it off means possibly better performance since CRC does not need to be calculated by the PCIe hardware and packet sizes are reduced. > > But I think the patch below is the right thing to do for now. Vidya, Agree. > did you trip over an issue because of this, e.g., a conflict between > firmware use of AER and Linux use of it? If so, maybe we could > mention a symptom on the commit log. But my guess is you probably > found this by inspection. > > Bjorn > >>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >>> --- >>> drivers/pci/pcie/aer.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c >>> index e2d8a74f83c3..730b47bdcdef 100644 >>> --- a/drivers/pci/pcie/aer.c >>> +++ b/drivers/pci/pcie/aer.c >>> @@ -184,6 +184,9 @@ static int disable_ecrc_checking(struct pci_dev *dev) >>> */ >>> void pcie_set_ecrc_checking(struct pci_dev *dev) >>> { >>> + if (!pcie_aer_is_native(dev)) >>> + return; >>> + >>> switch (ecrc_policy) { >>> case ECRC_POLICY_DEFAULT: >>> return; >> >> -- >> Sathyanarayanan Kuppuswamy >> Linux Kernel Developer
On 1/12/2023 4:57 AM, Sathyanarayanan Kuppuswamy wrote: > External email: Use caution opening links or attachments > > > Hi, > > On 1/11/23 3:10 PM, Bjorn Helgaas wrote: >> On Wed, Jan 11, 2023 at 01:42:21PM -0800, Sathyanarayanan Kuppuswamy wrote: >>> On 1/11/23 12:31 PM, Vidya Sagar wrote: >>>> As the ECRC configuration bits are part of AER registers, configure >>>> ECRC only if AER is natively owned by the kernel. >>> >>> ecrc command line option takes "bios/on/off" as possible options. It >>> does not clarify whether "on/off" choices can only be used if AER is >>> owned by OS or it can override the ownership of ECRC configuration >>> similar to pcie_ports=native option. Maybe that needs to be clarified. >> >> Good point, what do you think of an update like this: >> >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt >> index 6cfa6e3996cf..f7b40a439194 100644 >> --- a/Documentation/admin-guide/kernel-parameters.txt >> +++ b/Documentation/admin-guide/kernel-parameters.txt >> @@ -4296,7 +4296,9 @@ >> specified, e.g., 12@pci:8086:9c22:103c:198f >> for 4096-byte alignment. >> ecrc= Enable/disable PCIe ECRC (transaction layer >> - end-to-end CRC checking). >> + end-to-end CRC checking). Only effective >> + if OS has native AER control (either granted by >> + ACPI _OSC or forced via "pcie_ports=native"). >> bios: Use BIOS/firmware settings. This is the >> the default. >> off: Turn ECRC off I'm also fine with this change. I'll take it in V2. > > Looks fine. But do we even need "bios" option? Since it is the default > value, I am not sure why we need to list that as an option again. IMO > this could be removed. I think we still need bios option. For example, consider a system where BIOS needs to keep ECRC enabled for integrity reasons but if kernel doesn't want it for perf reasons, then, kernel can always use 'ecrc=off' option. > >> >> I don't know whether the "ecrc=" parameter is really needed. If we >> were adding it today, I would ask "why not enable ECRC wherever it is >> supported?" If there are devices where it's broken, we could always >> add quirks to disable it on a case-by-case basis. > > Checking the original patch which added it, it looks like the intention > is to give option to boost performance over integrity. > > commit 43c16408842b0eeb367c23a6fa540ce69f99e347 > Author: Andrew Patterson <andrew.patterson@hp.com> > Date: Wed Apr 22 16:52:09 2009 -0600 > > PCI: Add support for turning PCIe ECRC on or off > > Adds support for PCI Express transaction layer end-to-end CRC checking > (ECRC). This patch will enable/disable ECRC checking by setting/clearing > the ECRC Check Enable and/or ECRC Generation Enable bits for devices that > support ECRC. > > The ECRC setting is controlled by the "pci=ecrc=<policy>" command-line > option. If this option is not set or is set to 'bios", the enable and > generation bits are left in whatever state that firmware/BIOS set them to. > The "off" setting turns them off, and the "on" option turns them on (if the > device supports it). > > Turning ECRC on or off can be a data integrity versus performance > tradeoff. In theory, turning it on will catch more data errors, turning > it off means possibly better performance since CRC does not need to be > calculated by the PCIe hardware and packet sizes are reduced. > > >> >> But I think the patch below is the right thing to do for now. Vidya, > > Agree. > >> did you trip over an issue because of this, e.g., a conflict between >> firmware use of AER and Linux use of it? If so, maybe we could >> mention a symptom on the commit log. But my guess is you probably >> found this by inspection. Not really. I was just checking when does kernel touch ECRC settings and happened to find this where it configures ECRC irrespective of its ownership of AER registers. >> >> Bjorn >> >>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >>>> --- >>>> drivers/pci/pcie/aer.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c >>>> index e2d8a74f83c3..730b47bdcdef 100644 >>>> --- a/drivers/pci/pcie/aer.c >>>> +++ b/drivers/pci/pcie/aer.c >>>> @@ -184,6 +184,9 @@ static int disable_ecrc_checking(struct pci_dev *dev) >>>> */ >>>> void pcie_set_ecrc_checking(struct pci_dev *dev) >>>> { >>>> + if (!pcie_aer_is_native(dev)) >>>> + return; >>>> + >>>> switch (ecrc_policy) { >>>> case ECRC_POLICY_DEFAULT: >>>> return; >>> >>> -- >>> Sathyanarayanan Kuppuswamy >>> Linux Kernel Developer > > -- > Sathyanarayanan Kuppuswamy > Linux Kernel Developer
On 1/11/23 7:33 PM, Vidya Sagar wrote:
> I think we still need bios option. For example, consider a system where BIOS needs to keep ECRC enabled for integrity reasons but if kernel doesn't want it for perf reasons, then, kernel can always use 'ecrc=off' option.
I agree that "on" and "off" option makes sense. Since the kernel defaults ecrc setting to "bios", why again allow it as a command line option?
On 1/12/2023 9:18 AM, Sathyanarayanan Kuppuswamy wrote: > External email: Use caution opening links or attachments > > > On 1/11/23 7:33 PM, Vidya Sagar wrote: >> I think we still need bios option. For example, consider a system where BIOS needs to keep ECRC enabled for integrity reasons but if kernel doesn't want it for perf reasons, then, kernel can always use 'ecrc=off' option. > > I agree that "on" and "off" option makes sense. Since the kernel defaults ecrc setting to "bios", why again allow it as a command line option? Agree. "on" and "off" are fine but "default" is redundant. Do you want me to push a change to remove that as part of this patch itself? I think it is more like a cleanup and should go separately. > > -- > Sathyanarayanan Kuppuswamy > Linux Kernel Developer
On 1/11/23 8:59 PM, Vidya Sagar wrote: > > > On 1/12/2023 9:18 AM, Sathyanarayanan Kuppuswamy wrote: >> External email: Use caution opening links or attachments >> >> >> On 1/11/23 7:33 PM, Vidya Sagar wrote: >>> I think we still need bios option. For example, consider a system where BIOS needs to keep ECRC enabled for integrity reasons but if kernel doesn't want it for perf reasons, then, kernel can always use 'ecrc=off' option. >> >> I agree that "on" and "off" option makes sense. Since the kernel defaults ecrc setting to "bios", why again allow it as a command line option? > > Agree. "on" and "off" are fine but "default" is redundant. Do you want me to push a change to remove that as part of this patch itself? I think > it is more like a cleanup and should go separately. IMO, the "bios" option cleanup and command line update from Bjorn can be in one patch, and your change could be a separate patch. But it is up to you and Bjorn. > >> >> -- >> Sathyanarayanan Kuppuswamy >> Linux Kernel Developer
On 1/12/2023 10:36 AM, Sathyanarayanan Kuppuswamy wrote: > External email: Use caution opening links or attachments > > > On 1/11/23 8:59 PM, Vidya Sagar wrote: >> >> >> On 1/12/2023 9:18 AM, Sathyanarayanan Kuppuswamy wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> On 1/11/23 7:33 PM, Vidya Sagar wrote: >>>> I think we still need bios option. For example, consider a system where BIOS needs to keep ECRC enabled for integrity reasons but if kernel doesn't want it for perf reasons, then, kernel can always use 'ecrc=off' option. >>> >>> I agree that "on" and "off" option makes sense. Since the kernel defaults ecrc setting to "bios", why again allow it as a command line option? >> >> Agree. "on" and "off" are fine but "default" is redundant. Do you want me to push a change to remove that as part of this patch itself? I think >> it is more like a cleanup and should go separately. > > IMO, the "bios" option cleanup and command line update from Bjorn can be in one patch, and your change could be a separate patch. But it is > up to you and Bjorn. I think Bjorn's command line suggestion should go along with my patch otherwise the ECRC control through command line doesn't work if OS doesn't own the AER. So, it helps to make it explicit that the 'ecrc=' option works only if either kernel has native AER control or 'pcie_ports' is set to 'native' > >> >>> >>> -- >>> Sathyanarayanan Kuppuswamy >>> Linux Kernel Developer > > -- > Sathyanarayanan Kuppuswamy > Linux Kernel Developer
On Wed, Jan 11, 2023 at 03:27:51PM -0800, Sathyanarayanan Kuppuswamy wrote: > On 1/11/23 3:10 PM, Bjorn Helgaas wrote: > > On Wed, Jan 11, 2023 at 01:42:21PM -0800, Sathyanarayanan Kuppuswamy wrote: > >> On 1/11/23 12:31 PM, Vidya Sagar wrote: > >>> As the ECRC configuration bits are part of AER registers, configure > >>> ECRC only if AER is natively owned by the kernel. > >> > >> ecrc command line option takes "bios/on/off" as possible options. It > >> does not clarify whether "on/off" choices can only be used if AER is > >> owned by OS or it can override the ownership of ECRC configuration > >> similar to pcie_ports=native option. Maybe that needs to be clarified. > > > > Good point, what do you think of an update like this: > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > index 6cfa6e3996cf..f7b40a439194 100644 > > --- a/Documentation/admin-guide/kernel-parameters.txt > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > @@ -4296,7 +4296,9 @@ > > specified, e.g., 12@pci:8086:9c22:103c:198f > > for 4096-byte alignment. > > ecrc= Enable/disable PCIe ECRC (transaction layer > > - end-to-end CRC checking). > > + end-to-end CRC checking). Only effective > > + if OS has native AER control (either granted by > > + ACPI _OSC or forced via "pcie_ports=native"). > > bios: Use BIOS/firmware settings. This is the > > the default. > > off: Turn ECRC off > > Looks fine. But do we even need "bios" option? Since it is the default > value, I am not sure why we need to list that as an option again. IMO > this could be removed. I agree, it seems pointless. > > I don't know whether the "ecrc=" parameter is really needed. If we > > were adding it today, I would ask "why not enable ECRC wherever it is > > supported?" If there are devices where it's broken, we could always > > add quirks to disable it on a case-by-case basis. > > Checking the original patch which added it, it looks like the intention > is to give option to boost performance over integrity. > > commit 43c16408842b0eeb367c23a6fa540ce69f99e347 > Author: Andrew Patterson <andrew.patterson@hp.com> > Date: Wed Apr 22 16:52:09 2009 -0600 > > PCI: Add support for turning PCIe ECRC on or off > > Adds support for PCI Express transaction layer end-to-end CRC checking > (ECRC). This patch will enable/disable ECRC checking by setting/clearing > the ECRC Check Enable and/or ECRC Generation Enable bits for devices that > support ECRC. > > The ECRC setting is controlled by the "pci=ecrc=<policy>" command-line > option. If this option is not set or is set to 'bios", the enable and > generation bits are left in whatever state that firmware/BIOS set them to. > The "off" setting turns them off, and the "on" option turns them on (if the > device supports it). > > Turning ECRC on or off can be a data integrity versus performance > tradeoff. In theory, turning it on will catch more data errors, turning > it off means possibly better performance since CRC does not need to be > calculated by the PCIe hardware and packet sizes are reduced. Ah, right, and I think I was even part of the conversation when this was added :) I'm not sure I would make the same choice today, though. IMHO it's kind of hard to defend choosing performance over data integrity. If a platform really wants to sacrifice integrity for performance, it could retain control of AER, and after Vidya's patch, Linux will leave the ECRC configuration alone. Straw-man: If Linux owns AER and ECRC is supported, enable ECRC by default. Retain "ecrc=off" to turn it off, but drop a note in dmesg and taint the kernel. Bjorn
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index e2d8a74f83c3..730b47bdcdef 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -184,6 +184,9 @@ static int disable_ecrc_checking(struct pci_dev *dev) */ void pcie_set_ecrc_checking(struct pci_dev *dev) { + if (!pcie_aer_is_native(dev)) + return; + switch (ecrc_policy) { case ECRC_POLICY_DEFAULT: return;