Re: [PATCH v9] cxl: add RAS status unmasking for CXL

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

On 7/13/2023 2:21 PM, Dan Williams wrote:
> Smita Koralahalli wrote:
>> Hi all,
>>
>> I understand this has been in upstream already. But I have a slight
>> confusion on one of the checks been done here.
>>
>> On 2/21/2023 9:55 AM, Dave Jiang wrote:
>>> By default the CXL RAS mask registers bits are defaulted to 1's and
>>> suppress all error reporting. If the kernel has negotiated ownership
>>> of error handling for CXL then unmask the mask registers by writing 0s.
>>>
>>> PCI_EXP_DEVCTL capability is checked to see uncorrectable or correctable
>>> errors bits are set before unmasking the respective errors.
>>>
>>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>  # pci_regs.h
>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>
>>> ---
>>
>>> +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;
>>> +	u16 cap;
>>> +	int rc;
>>> +
>>> +	if (!cxlds->regs.ras) {
>>> +		dev_dbg(&pdev->dev, "No RAS registers.\n");
>>> +		return 0;
>>> +	}
>>> +
>>> +	/* BIOS has CXL error control */
>>> +	if (!host_bridge->native_cxl_error)
>>> +		return -EOPNOTSUPP;
>>
>> Why are we checking for native_cxl_error (native_cxl_error is CXL Memory
>> Error Reporting Control _OSC bit..) while unmasking RAS status?
>>
>> RAS registers will be reported on a protocol error and the protocol
>> error follows the PCIe AER. Should we check for AER _OSC instead of CXL
>> Memory Error _OSC?
>>
>> Because atleast on AMD systems we log RAS registers only on Protocol
>> errors and we use this CXL Memory _OSC knob to report component errors.
>> Is it same across everywhere? And there might be cases where protocol
>> error reporting might be native (PCIe AER) and component/memory can be
>> FW-First which fails this check..
> 
> I think that's reasonable, it just was not clear from the specification
> that CXL protocol errors are included in PCIe AER as far as _OSC is
> concerned because they are conveyed as "internal" errors.
> 
> So I believe it was an "abundance of caution" more than a requirement
> that Linux expects control of memory-errors before proceeding to touch
> the CXL RAS registers.

Got it. Also, are there any issues returning zero here, rather than an 
error code just like we are doing in cxl_event_config()?

The error code returned from this function wouldn't grant cxl control to 
OS (i.e fails to create device node /dev/cxl/mem0) which would confuse 
users when they are operating with native PCIe AER support.

Thanks
Smita