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

[Smita Koralahalli]

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@xxxxxxx>
> > > > ---
> > > >    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)