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: <f044379c-f363-0c94-cde3-8b062f1628ee@amd.com>
Date:   Tue, 8 Aug 2023 12:37:57 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH v2 1/3] cxl/pci: Fix appropriate checking for _OSC while
 handling CXL RAS registers
Content-Language: en-US
To:     Dan Williams <dan.j.williams@intel.com>, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-cxl@vger.kernel.org
Cc:     Bjorn Helgaas <bhelgaas@google.com>, oohall@gmail.com,
        Lukas Wunner <lukas@wunner.de>,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        Mahesh J Salgaonkar <mahesh@linux.ibm.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Ira Weiny <ira.weiny@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Yazen Ghannam <yazen.ghannam@amd.com>,
        Terry Bowman <terry.bowman@amd.com>,
        Robert Richter <rrichter@amd.com>
References: <20230721214740.256602-1-Smita.KoralahalliChannabasappa@amd.com>
 <20230721214740.256602-2-Smita.KoralahalliChannabasappa@amd.com>
 <64d1b3e78629f_5ea6e2944@dwillia2-xfh.jf.intel.com.notmuch>
From:   Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
In-Reply-To: <64d1b3e78629f_5ea6e2944@dwillia2-xfh.jf.intel.com.notmuch>
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?U2d6NExRdEpSamVVcjJoYUlBdkdUOVJaWmtVcFNsVTRmemdCZUI2ZWpaam1n?=
 =?utf-8?B?T0lDQi8xOEZPWmdWNkcyaVVoY0RnVHl3UnZKejB4aTllZ2xzSk9HYzIwdEVM?=
 =?utf-8?B?WFJaWXFYZE5IMXJTR09DeUg5RmNiMWVMTzJWeXVBTDVzKzJueDVnVEo3d1da?=
 =?utf-8?B?NE9qaFdWU1J4TzdhOUxYaU10c2lvUUhjWU5CNktJaWF6bXZrSk4xcWw3ajJB?=
 =?utf-8?B?cmtKOS9aWjRVbXJsMmw2OFJpTFVHUnpyT0lEQkxuTkpUZjBoTlRTMjVZemFs?=
 =?utf-8?B?NUc3empkTklSM1FaZ2lTbUxHZkl1bnJHR1A5eFNPcCtISDZLUTVUQ1hYcU45?=
 =?utf-8?B?YzUyMVhXVXloRXRyYkFDSlVkNDlpSDQ1Y3QxeVlERVo3MEZYOFhQT2MxZDgz?=
 =?utf-8?B?R2xJVHVsZ2k0QjlnRjI5RUpWK3U4aXhycytlYU9tejJDbXR6ckF0M3Z2U3No?=
 =?utf-8?B?NENialN5NlYxa0taT3VWR2t1U09EWWoxb0ZOSUpCTUw3YTRlTGxRZVhNeGgw?=
 =?utf-8?B?WHNxU2hhbzJaMTFIRzgvNURuVnNCK1RGakJ0RWNwMTBNM2pyOUM5dVE4QW1o?=
 =?utf-8?B?d0FMeTAvUmh0QllKSEhBT1RSaUNjc0l5b0tOcEd6OFJRb0VsbVRZeXJUYk13?=
 =?utf-8?B?Z2ZROXlrS3FuSTd1a2drYTFERm9tTWI2aHJjalhyemo0YnR2ZmhNYnU3ODU0?=
 =?utf-8?B?R1hKT3BDUjcwSG8zdGVCTzZFb2I3ZEJoQVpCYnJicXNPUldMVWRkVTlmc2FC?=
 =?utf-8?B?SktnZEhuK3pxc3ppRFdIQkNWcVJON1N5bm83a0RmTUs0dDNjQnlueVlxUmdK?=
 =?utf-8?B?b1MwTzN6aFNFZHhaSVNlMGlGOFJhdGxFaWtWQndneml3bG11MHpaaGhITWZX?=
 =?utf-8?B?d0VCMUhtN0VOaFlQOVF2MTA4aUREOGJBTWNaTjVTUGNLSXNVSEJ4aElvOVZ4?=
 =?utf-8?B?MlE5QjBIQkUyUExPVjVvYTR5L0ErMnY1YUlpS0lKZ25vUW1oVVpWWFlqSGhn?=
 =?utf-8?B?UFg1RVpJc0k3ckVkdHFJWFRnb0ZUVUsxbzRwL01kZWVtVC85NkZvQ1J3bWlE?=
 =?utf-8?B?Ly9xdUsveTFmdE5TUE5uUTlmbUxJUUhTSGNqZlNqS1Z4b1RlYlNwZDJMMTdS?=
 =?utf-8?B?WEJVczlEeG5kRytsRFd0ZkpDc25hbWdOYUV4MHZtMElTem9BcGpqeW1RNHEw?=
 =?utf-8?B?amN0MGhlVTVGaThkSUUwdWVKOGJUWWNnREN3djEycVZHb2wyZFk0c2JKWDZr?=
 =?utf-8?B?Sll0SnViSVcxTDMrdTNTZEJ6UndCa2dBVGJjWmxqdmFZYzkvbGltSFZWNXVk?=
 =?utf-8?B?ZFFkTEZTcXhxZnU2Vi84TWdBVmY3NVRnTU9RN2IxSERORm1BbDhvZHluQWtH?=
 =?utf-8?B?OUVYZjRldWdoNVNZNStEeXF5WDVkMTJpYmtYRWhaU05xdENZQlI0Y1ppd0I4?=
 =?utf-8?B?MFlLV0dOblM3QTRMczBENkRtYVEzeUFuVW9MUkM1VmhlVDIvUTdXVWtrbjl6?=
 =?utf-8?B?bmk4bHhGdlNpUzdINnJybjhWd0lpaXhxdTBZOEF6M0NoOW10QzNVb3hiR1BO?=
 =?utf-8?B?bThGTjdMVktDaUxPT09UaGRUVWpwc0dBck5ITFRwd1BqTUxOOW0xQUEreWww?=
 =?utf-8?B?RUFLL2grci9RblYxR1h6c2NkMG5La2N3MUI2eHFPNXRLZ0tiZXdhRzQ0Wk9Y?=
 =?utf-8?B?Tk11dWdWNEt1Z21jc2tkUExyVDhSQU9ScXRQOWhkbXgwYmtmOTlnRmZ1K3NG?=
 =?utf-8?B?NVhZTWY0UGFRMy9ER3Y4WjNGd2xncXJwQWRLb1dmMGgvZ0xGSWg3VEpJRnE0?=
 =?utf-8?B?Zk5TMkUyYUtOSzFQM1hEZEpMZE1lQ2k2RXhHVWFaanlzR2ROejdZMDNTWEpw?=
 =?utf-8?B?cGl4NHFieUhNNmlCVDE5UnJEczhvakV5U2U2UmF3eEtRcHQ4OVY4Q2Z3OFNG?=
 =?utf-8?B?NFBXWXhNRG9RMnpQalNVQWEvT1pkb3l4b05peHgxWU1NeDVJQ0pDSzQ1NUla?=
 =?utf-8?B?Ykw3cmk5TEM2anZjSXM2WlUwTDNuRW9KTi9HbXJ5Uko1cTd2eEVRbzZrNEFT?=
 =?utf-8?B?Y1c5SG9yZ00rTUVPVlAvRHRBODZoVGVOYWVjaWJpenJwaDF1c2MwWGhvQjFj?=
 =?utf-8?Q?//vgrcdb4VZhNUUfOLqxa7NVs?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9dc1b9bd-a746-49c6-cfa0-08db9846f9cf
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB2869.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Aug 2023 19:38:01.1480
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: lWKwyjQk8RCOXeG+y5pobstPjuY+zJqwIZMEHYJUzw1UsMYcspo0Xhzho2kWDGymHyXh1hZGRGZ7h3u0ZoZQrg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB6305
X-Spam-Status: No, score=-2.9 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_BLOCKED,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE,URIBL_BLOCKED autolearn=ham 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 8/7/2023 8:17 PM, Dan Williams wrote:
> Smita Koralahalli wrote:
>> According to Section 9.17.2, Table 9-26 of CXL Specification [1], owner
>> of AER should also own CXL Protocol Error Management as there is no
>> explicit control of CXL Protocol error. And the CXL RAS Cap registers
>> reported on Protocol errors should check for AER _OSC rather than CXL
>> Memory Error Reporting Control _OSC.
>>
>> The CXL Memory Error Reporting Control _OSC specifically highlights
>> handling Memory Error Logging and Signaling Enhancements. These kinds of
>> errors are reported through a device's mailbox and can be managed
>> independently from CXL Protocol Errors.
>>
>> This change fixes handling and reporting CXL Protocol Errors and RAS
>> registers natively with native AER and FW-First CXL Memory Error Reporting
>> Control.
> 
> I feel like this could be said more succinctly and with an indication of
> what the end user should expect to see. Something like:
> 
> "cxl_pci fails to unmask CXL protocol errors when CXL memory error
> reporting is not granted native control. Given that CXL memory error
> reporting uses the event interface and protocol errors use AER, unmask
> protocol errors based only on the native AER setting. Without this
> change end user deployments will fail to report protocol errors in the
> case where native memory error handling is not granted to Linux."

Sure, will make the change for a more clearer description. Thanks!
> 
>>
>> [1] Compute Express Link (CXL) Specification, Revision 3.1, Aug 1 2022.
>>
>> Fixes: 248529edc86f ("cxl: add RAS status unmasking for CXL")
>> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
>> ---
>> v2:
>> 	Added fixes tag.
>> 	Included what the patch fixes in commit message.
>> ---
>>   drivers/cxl/pci.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>> index 1cb1494c28fe..2323169b6e5f 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -541,9 +541,9 @@ static int cxl_pci_ras_unmask(struct pci_dev *pdev)
>>   		return 0;
>>   	}
>>   
>> -	/* BIOS has CXL error control */
>> -	if (!host_bridge->native_cxl_error)
>> -		return -ENXIO;
>> +	/* BIOS has PCIe AER error control */
>> +	if (!host_bridge->native_aer)
>> +		return 0;
> 
> The error code does not matter here and changing it makes the patch that
> bit much more noisier than it needs to be. So just leave it as:

Doing this will return an error from cxl_pci probe thereby failing the 
device node creation in FW-First AER/DPC. I cannot think of other places 
where we reference the device node in FW-First mode but I have a place 
where this could potentially be a roadblock.

I'm trying to add trace events support for FW-First Protocol Errors. 
https://lore.kernel.org/linux-cxl/D9381C12-A585-4089-873B-3707C17823D3@fb.com/T/#mcaf8a78c1295372ab811be7e1ccb6a8a4d99f3e9

And we already have an existing trace_cxl_aer_correctable_error() and 
similarly for uncorrectable error for native protocol error reporting. I 
was trying to reuse the same function for fw-first as well. This 
function references cxl memory device node which will be NULL in 
FW-First if this returns an error.

I don't mind having a separate trace event function for FW-First mode as 
it would simplify things especially when dealing with RCH DP.. But there 
may be other potential places where we might reference this device node 
in FW-First. Please advice.

Thanks,
Smita

> 
> 	return -ENXIO;
> 
>>   
>>   	rc = pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &cap);
>>   	if (rc)
>> -- 
>> 2.17.1
>>
> 
> 
>