Re: [PATCH 2/2] cxl/pci: Fix appropriate checking for _OSC while handling CXL RAS registers

[Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>]

On 7/20/2023 6:07 AM, Robert Richter wrote:
> Smita,
> 
> On 19.07.23 15:30:25, Smita Koralahalli wrote:
>> On 7/19/2023 1:39 PM, Sathyanarayanan Kuppuswamy wrote:
>>>
>>>
>>> On 7/19/23 12:23 PM, 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.
>>>
>>> Does it fix any issue? If yes, please include that in the commit log.
>>
>> Yes, this fix actually makes Protocol Error handling independent of
>> Component/Memory Error handling.
>>
>> We observed that OS was not able to handle the protocol errors ("i.e unable
>> to reference to the cxl device node") with native AER support. The reason
>> being Memory/Component Error handling was under FW control.
>>
>> Since the RAS registers are tied to protocol errors, I think there is no
>> reason that memory error reporting being in fw control or os control should
>> be a roadblock in handling RAS registers or accessing the cxl device node by
>> OS.
>>
>>>
>>> Since you are removing some change, maybe it needs Fixes: tag?
>>
>> Missed this. Thanks!
>>
>> Fixes: 248529edc86f ("cxl: add RAS status unmasking for CXL")
> 
> the fix must be isolated to this patch (for automated backports) and
> you need to remove the dependency to the first patch then. So swap
> them and ... see below.
> 
>>
>> Will include in v2.
>>
>> Thanks,
>> Smita
>>
>>>>
>>>> [1] Compute Express Link (CXL) Specification, Revision 3.1, Aug 1 2022.
>>>>
>>>> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
>>>> ---
>>>>    drivers/cxl/pci.c | 7 +++----
>>>>    1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>>>> index 1cb1494c28fe..44a21ab7add5 100644
>>>> --- a/drivers/cxl/pci.c
>>>> +++ b/drivers/cxl/pci.c
>>>> @@ -529,7 +529,6 @@ static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>>>>    static int cxl_pci_ras_unmask(struct pci_dev *pdev)
>>>>    {
>>>> -	struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus);
>>>>    	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
>>>>    	void __iomem *addr;
>>>>    	u32 orig_val, val, mask;
>>>> @@ -541,9 +540,9 @@ static int cxl_pci_ras_unmask(struct pci_dev *pdev)
>>>>    		return 0;
>>>>    	}
>>>> -	/* BIOS has CXL error control */
>>>> -	if (!host_bridge->native_cxl_error)
> 
> For the fix, you could replace that with:
> 
> 	if (!host_bridge->native_aer) ...

Yeah I tried something like:
+	if (!pdev->aer_cap &&
+	    !(pcie_ports_native || host_bridge->native_aer))
+		return 0;

But then pcie_ports_native needed to be exported as well. So better just 
keep the check to !host_bridge->native_aer and return zero in first 
patch, EXPORT to second and replacing host_bridge->native_aer with 
pcie_aer_is_native() in third?

Thanks,
Smita

> 
>>>> -		return -ENXIO;
>>>> +	/* BIOS has PCIe AER error control */
>>>> +	if (!pcie_aer_is_native(pdev))
>>>> +		return 0;
> 
> ... and replace it with this function here in the patch where
> pcie_aer_is_native() is exported (or in a 3rd patch).
> 
> -Robert
> 
>>>>    	rc = pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &cap);
>>>>    	if (rc)
>>>
>>