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

[Robert Richter]

On 20.07.23 11:31:15, Smita Koralahalli wrote:
> 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?

Looks good.

Thanks,

-Robert

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