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;
Message-ID: <4ddef2c9-44b6-4a08-dbeb-428cd0864c55@nvidia.com>
Date:   Thu, 12 Jan 2023 09:03:17 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.1
Subject: Re: [PATCH V1] PCI/AER: Configure ECRC only AER is native
To:     Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        Bjorn Helgaas <helgaas@kernel.org>
Cc:     bhelgaas@google.com, ruscur@russell.cc, oohall@gmail.com,
        treding@nvidia.com, jonathanh@nvidia.com, mmaddireddy@nvidia.com,
        kthota@nvidia.com, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, vsethi@nvidia.com,
        linuxppc-dev@lists.ozlabs.org, sagar.tv@gmail.com
References: <20230111231033.GA1714672@bhelgaas>
 <880c4d3c-86d2-082c-bb58-8212adc67ff3@linux.intel.com>
Content-Language: en-US
From:   Vidya Sagar <vidyas@nvidia.com>
In-Reply-To: <880c4d3c-86d2-082c-bb58-8212adc67ff3@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?eXpvakFKN1RQVXBpN0dVeXhzeG5EL2ZzWEtaaUFlSzk5cm1RcmoxcXk0Vmtk?=
 =?utf-8?B?MElmS0FvWm5tb0h4WlZ1NzZzZ0tTcitTcGlqeGJJdTMvSUtyeGdyMnViUnpt?=
 =?utf-8?B?Vk1pdDU4TTdyVUJYL004OWIxMXBJRGd0MVVVdzgyaFk3R2dySnpUY3h3WTY4?=
 =?utf-8?B?SVZQbnM5UFZpMlRqTXZjNmZxekROTnprN1N4Y1ZkbGllc05TOTRGUkdzcTJ4?=
 =?utf-8?B?T3FBMDNUWGNVOE9hcEFITm9Ma0ZOOVp6QUtEVTRJVWtUMTdqeU51dG1lV2Zm?=
 =?utf-8?B?NUNMdDg0N3BOMERNZlFyWlpxbmRuQnZQeWhVOVJsZjZOL29sUER3V0tnYVRP?=
 =?utf-8?B?Ym0rWHF1b1czU0VwS2xOUVFTVlZ6QkJhUTZQTVZDdEtTZTZKTFFGdVozMXA1?=
 =?utf-8?B?aStuOU5NL2JkQXhvbzZxeVd3cnZRNVpyV1E3VHI0aUU4RWVTTmczYXE1SkJN?=
 =?utf-8?B?K2ZWTnMyeGZ0blovdFpOR2VFUHZKZjBrSWp4YXJkTDBzeUZsbjVLMnBqQ0xP?=
 =?utf-8?B?OGovN21qNC9jV0lmVWsrdUlrMkFXdnB6VzZhTm5oMG00UVJFdG5oV05HNVQw?=
 =?utf-8?B?dXI4NFJJRTNtbHRyaEhSa29veUZ4NG4vUVV1d3pSSDZObUtaalkrdkJGbnhv?=
 =?utf-8?B?RG5JL1Fac0NQTjhXUW1FRjlEYklEUVZtdVF5VllmclpYZUpINFBJaWVlRjU1?=
 =?utf-8?B?OWVObXYvWWZsbEtnVXVhdGh0UjZJMzdUL0JjR1dBWlBuMmh0bnZhelpDQW5K?=
 =?utf-8?B?WnZaYnZ2UU1GQytkeVI4dlNHVTJhbHY0NUp3VC9RdlpmWEZOOC80dW1ESnJB?=
 =?utf-8?B?NnU2d0Y0aStOYlMyM3h3WjBRUVZYSnFLQ1Vmd2Z5b2xNdUY2WDhjTmdVdTNI?=
 =?utf-8?B?UDNXd0NGOWYzTU95ZGJsb3hmTlNNVDFXRG9PaUhjUlZxcS91N0N0ZXFVZ0Zp?=
 =?utf-8?B?VzlyNDRVakVrTkVvZFVaelhLVnRzK2ErRTU5aStlUk9DU0FmR0hJbkNBOFJM?=
 =?utf-8?B?M0xmLzRFZGdkMlBWOXd6Vi9oSkRDTDJnMHRGaEJTdWhraXUvaXZqMTNrWkFX?=
 =?utf-8?B?dWpCYWd5REh5QWpLMTVjYnFtakIrcXQrWFFzdzhLaFVwaUprV3ZxSmhjTFNU?=
 =?utf-8?B?S0ozbkNyNjlJNjg1dm1taW54WVR2TStuTVdXdGVwSHdSYklpMXFYVnJuKzJz?=
 =?utf-8?B?SXBuNXdPMmZMOWFoa2xEMk9BUTZTcUFUZ2xPTlNZd01IZCttNUZUZTVSOUQy?=
 =?utf-8?B?citaTTBQK21LU2hmOENVTVRINEloKzh6VTFxOHNkT3M3dTZxc3FQNjJKS2E0?=
 =?utf-8?B?cHRzU05CZ0VYK2VveWdjQVE4VVgxTUtLOGV4NnhPcDFpVW5obzRUV1NWSVpF?=
 =?utf-8?B?TjB6RnVTK2ZmU0tMVHY3RTFKVkM0OHdQVk0rd2NybHNSODVJdTVTdU5ITTlP?=
 =?utf-8?B?cm9ncHJqRWhpK0tQVGRrOXJyNDRXYVAyeEovWHlza3B1QWQ5dU5tZ05uSnd0?=
 =?utf-8?B?Y1lzaG1UblJqRlNvK2Y1OHBRQlJTc3krd1NFVld3bGthM2JYaXpOY0NwaGVF?=
 =?utf-8?B?ak93U1A3Wmo5WGVLYzlCQjIzY0VWcXVTVGpWNlZSbElpSWJoY1FSbElTTDFT?=
 =?utf-8?B?QXF1VmEyNnBGUWlVM2pNUGp3RFBrb3lVZjRrR0pRZFZBRkNJMmtLTXdkWmZz?=
 =?utf-8?B?M1g5bmovVWZDbnBPQ2F0NWg4QTg0U29tcmwwdzM1Q3NqMGt3OGptMllCR0x5?=
 =?utf-8?B?MkZUb2pLNHVnQlp2VEVCRXpFVEl1MWpOUFFhcFh6ZHd4b1paUWhzbitaTVNS?=
 =?utf-8?B?Nk54RlI2bFZLVllTUlFMb2E5NlEyN0NsSVAya0pRcUEzTEJiMlRpUE9UM2Uy?=
 =?utf-8?B?RkdSYUk4dm5iRDViNm9xM2F0aTlPcHc1bHJVZGNzYWNnVW9QTzJ5QmNGbysw?=
 =?utf-8?B?QjRscWtKSVBhejRSTzhMcWVQajlDTDhvckJPZnFoWEFoUllFREc0U1JXbmlK?=
 =?utf-8?B?YVZrb0FFQjVBWGp4Y1hlbGp1VjRZT1dwTXRQZmFmbzh4YytxQWh5MFUrNG5T?=
 =?utf-8?B?SVl2MEFIZjkzb0poTm5OZndWcHBZS21OL3lmUTRCcjZBV25aaXgraDdSWU85?=
 =?utf-8?Q?EJ+6QaMhcIGBfdqTMrXiwwW78?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 86bfc1af-484a-4779-4ec8-08daf44dc5b5
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB2900.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Jan 2023 03:33:29.8936
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: uObn6AZwVXjnUHalj1WiD/zOjY9oXC1goYvCmwJ5AG5vhcGqd5EPFl5e9n6CFBX4fWdQ+XH85Zp0BK/wK2oq7Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR12MB4040
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,
        NICE_REPLY_A,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


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