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