Linux Documentation
 help / color / mirror / Atom feed
From: "Bowman, Terry" <terry.bowman@amd.com>
To: Lukas Wunner <lukas@wunner.de>
Cc: Jonathan Cameron <jic23@kernel.org>,
	Dave Jiang <dave.jiang@intel.com>,
	Alison Schofield <alison.schofield@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	Davidlohr Bueso <dave@stgolabs.net>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Dan Williams <djbw@kernel.org>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	Jonathan Corbet <corbet@lwn.net>,
	linux-cxl@vger.kernel.org, Tony Luck <tony.luck@intel.com>,
	Borislav Petkov <bp@alien8.de>, Hanjun Guo <guohanjun@huawei.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Shuai Xue <xueshuai@linux.alibaba.com>,
	Len Brown <lenb@kernel.org>, Ira Weiny <iweiny@kernel.org>,
	Li Ming <ming.li@zohomail.com>,
	Shuah Khan <skhan@linuxfoundation.org>,
	Ben Cheatham <Benjamin.Cheatham@amd.com>,
	Richard Cheng <icheng@nvidia.com>,
	Robert Richter <rrichter@amd.com>,
	linux-pci@vger.kernel.org, linux-acpi@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v20 0/9] Enable CXL PCIe Port Protocol Error handling and logging
Date: Wed, 9 Sep 2026 15:31:54 -0500	[thread overview]
Message-ID: <c6b62614-fcc8-48aa-b65e-b3c26c8579a7@amd.com> (raw)
In-Reply-To: <aqGDV2GQEqxg_1gE@wunner.de>

On 9/9/2026 11:03 AM, Lukas Wunner wrote:
> On Wed, Sep 02, 2026 at 08:39:24AM -0500, Terry Bowman wrote:
>> Today the kernel handles native CXL.cachemem RAS only for Endpoints and
>> Restricted CXL Host (RCH) Downstream Ports. Root Ports, Upstream Switch
>> Ports, and Downstream Switch Ports are uncovered. This series introduces
>> a unified CXL protocol error path for all CXL device types, in both VH
>> and RCH topologies.
> 
> I'm trying to make sense of the existing code.  I know this wasn't
> introduced by you (but by Robert in 0a867568bb0d), but you've moved
> this code around recently and are extending it in this series.
> 
> Errors of the RCH downstream port are signaled as internal errors
> of an RCEC on the root bus.  Their handling was plumbed into:
> 
> aer_process_err_devices()
>   handle_error_source()
>     cxl_rch_handle_error()
> 

cxl_rch_handle_error() is used in the RCH and RCD error handling path. VH errors 
are handled through the call to cxl_forward_errors(). Both, RCH/RCD and VH errors
use the kfifo for handling in cxl_core.

> First of all, handle_error_source() calls pci_aer_handle_error(),
> which (for Fatal Errors) will issue an FLR of the RCEC.  Note that
> Uncorrectable Internal Errors have Fatal severity by default
> (PCIe r7.0 sec 7.8.4.4).
> 
> If the Uncorrectable Internal Error was the only error, then why is the
> RCEC being reset?  It's just serving as a conduit to inform that there
> are CXL errors at the downstream port.  There's no reason at all to
> issue an FLR to the RCEC in that case.
> 

RCEC FLR doesn't serve a purpose here. This likely needs a check to skip the FLR
if the device is a CXL RCEC and the error is internal, indicating a CXL RAS error
at the downstream CXL device.

> Second, cxl_rch_handle_error() then walks all the RCiEPs reporting to
> the RCEC.  This also looks weird to me.  Can there ever be more than
> one RCiEP?  I think not, but maybe I'm missing something.  If there's
> only ever a single RCiEP reporting to the RCEC, why perform a walk?
> 

The RCEC can have more than one associated RCiEP. The associated set is carried 
in the RCEC Endpoint Association DVSEC, which is what pcie_walk_rcec() iterates. 

> What we actually want to do is retrieve the CXL errors from the
> downstream port's RCRB, but this is done in a fairly roundabout way:
> cxl_rch_handle_error() walks the RCiEPs (aka RCDs), invokes the
> ->error_detected() callback for each, which is cxl_error_detected().
> That will then call cxl_handle_rdport_errors() to find the RCH
> downstream port to which the RCD is attached.
> 
> Isn't there a simpler way to find the RCH downstream port from which
> the error originated?  Why do we have to go through the RCDs?
> 

Unfortunately there isn't a better approach. The CXL spec (Chapter 12) outlines 
2 procedures for RCH/RCD protocol error reporting. The standard approach is 
implemented and follows the logic of searching all CXL RCiEP's (RCDs) 
associated with the reporting RCEC.

A second approach defined by the CXL spec named the RCEC Downstream Port 
Association Structure (RDPAS) was submitted for review by Dave Jiang but did 
not offer obvious improvement as it didn't eliminate the search iterations.
Also, its an optionally supported platform device hardware making it "not 
required". 
https://lore.kernel.org/linux-cxl/20260618170723.2010490-1-dave.jiang@intel.com/

> Also, putting this in cxl_error_detected() has a weird side effect:
> The function is also invoked when the RCD upstream port experienced
> an error.  But because the retrieval, reporting and handling of
> RCH downstream port errors was put into this function, those errors
> are reported and handled as a side effect of RCD upstream port errors.
> What sense does this make?
> 

The side effect stems from the RCH Downstream Port being implemented as 
an RCRB with no BDF or PCI visibility. Because the DP cannot be addressed 
directly, software discovers it via the downstream RCD.

Per CXL r4.0 §12.2.1.1, an RCH Downstream Port has no BDF of its own.
The RCEC Error Source Identification register logs the RCEC's own BDF 
"because the RCH Downstream Port is not associated with one," and the 
spec states outright that "the RCEC Error Source Identification register 
is insufficient for identifying the error source." Software must therefore 
either follow PCIe rules to inspect the RCEC-associated RCiEPs, or use 
the optional RDPAS structure (§9.18.1.5) "if present." Because RDPAS is 
optional, the RCiEP walk is the baseline discovery path, and it is why 
the DP is reached via its associated RCD rather than addressed directly.


> The commit message of 0a867568bb0d provides the following hint as
> to why this approach was chosen:
> 
>     The reason for choosing this implementation is that the AER service
>     driver claims the RCEC device, but does not allow it to register a
>     custom specific handler to support CXL. Connecting the RCEC hard-wired
>     with a CXL handler does not work, as the CXL subsystem might not be
>     present all the time.
> 
> So I think the point may have been to make this work even if the cxl
> module is not loaded?  Is that all?  Is that the only reason?
> We have try_module_get() and symbol_get() helpers.  You could just
> call those prior to invoking functions implemented by the cxl module
> from the AER driver.  There's plenty of precedent for that in the kernel.
> 

We we were looking for a callback that fit the current PCI error handling
but also supported handling errors detected in the RCH RCRB. The move
to use the kfifo removes the module dependency and timing constraint.
I'd prefer that over symbol_get()/try_module_get() here: it avoids a 
per-error symbol lookup in the error path and gives explicit ordering against 
AER recovery via cxl_proto_err_wait_for_empty(). But, I'm open to it if you 
see a concrete advantage.

> The problem is that the present approach is fairly complex and has
> side effects which make it difficult to understand and reason about
> the error handling.  In my view, this should be cleaned up first
> before bolting more functionality on top of it.
> 
> Thanks,
> 
> Lukas


I understand the concern. The RCEC-reset-on-conduit-error issue is
self-contained and worth fixing. I'll post it as a standalone patch.
                                                                                                                                                        
For the broader restructuring, this is already on the serie's coversheet TODO 
list ("Move RCH traversing for handling from AER driver into CXL driver"): the 
plan is to move the RCH/RCD searching into cxl_core, after the error is forwarded 
via the kfifo, so discovery lives in the CXL subsystem rather than the AER driver. 
I'd like to keep this series focused on landing the unified CXL protocol-error 
path and take that up as follow-up work. Is that acceptable, or is there a 
specific change you'd want to see land first?

Thanks,
Terry


      reply	other threads:[~2026-09-09 20:32 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-02 13:39 [PATCH v20 0/9] Enable CXL PCIe Port Protocol Error handling and logging Terry Bowman
2026-09-02 13:39 ` [PATCH v20 1/9] PCI/AER: Introduce AER-CXL protocol error kfifo Terry Bowman
2026-09-02 20:57   ` Cheatham, Benjamin
2026-09-08  0:51   ` Jonathan Cameron
2026-09-09 15:38     ` Bowman, Terry
2026-09-09 22:02       ` Jonathan Cameron
2026-09-10 14:57         ` Bowman, Terry
2026-09-02 13:39 ` [PATCH v20 2/9] PCI: Establish common CXL Port protocol error flow Terry Bowman
2026-09-02 20:57   ` Cheatham, Benjamin
2026-09-10 16:55     ` Bowman, Terry
2026-09-08  0:57   ` Jonathan Cameron
2026-09-02 13:39 ` [PATCH v20 3/9] cxl/ras: Handle RCH correctable and uncorrectable errors in one pass Terry Bowman
2026-09-02 20:57   ` Cheatham, Benjamin
2026-09-08  1:06   ` Jonathan Cameron
2026-09-02 13:39 ` [PATCH v20 4/9] cxl/pci: Thread port and dport through RAS handling helpers Terry Bowman
2026-09-02 20:57   ` Cheatham, Benjamin
2026-09-09 14:42     ` Bowman, Terry
2026-09-09 15:21     ` Bowman, Terry
2026-09-08 17:47   ` Jonathan Cameron
2026-09-02 13:39 ` [PATCH v20 5/9] cxl: Update CXL Endpoint AER handler Terry Bowman
2026-09-02 20:57   ` Cheatham, Benjamin
2026-09-02 13:39 ` [PATCH v20 6/9] PCI: Cache PCI DSN into pci_dev->dsn during probe Terry Bowman
2026-09-02 20:57   ` Cheatham, Benjamin
2026-09-09 15:16   ` Lukas Wunner
2026-09-02 13:39 ` [PATCH v20 7/9] cxl: Add port and dport identifiers to CXL AER trace events Terry Bowman
2026-09-08 18:13   ` Jonathan Cameron
2026-09-02 13:39 ` [PATCH v20 8/9] PCI/CXL: Mask/Unmask CXL protocol errors Terry Bowman
2026-09-02 20:57   ` Cheatham, Benjamin
2026-09-02 13:39 ` [PATCH v20 9/9] Documentation: cxl: Document CXL protocol error handling Terry Bowman
2026-09-08 18:39   ` Jonathan Cameron
2026-09-10 15:19     ` Bowman, Terry
2026-09-09 16:03 ` [PATCH v20 0/9] Enable CXL PCIe Port Protocol Error handling and logging Lukas Wunner
2026-09-09 20:31   ` Bowman, Terry [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c6b62614-fcc8-48aa-b65e-b3c26c8579a7@amd.com \
    --to=terry.bowman@amd.com \
    --cc=Benjamin.Cheatham@amd.com \
    --cc=alison.schofield@intel.com \
    --cc=bhelgaas@google.com \
    --cc=bp@alien8.de \
    --cc=corbet@lwn.net \
    --cc=dave.jiang@intel.com \
    --cc=dave@stgolabs.net \
    --cc=djbw@kernel.org \
    --cc=guohanjun@huawei.com \
    --cc=icheng@nvidia.com \
    --cc=iweiny@kernel.org \
    --cc=jic23@kernel.org \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-cxl@vger.kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lukas@wunner.de \
    --cc=mchehab@kernel.org \
    --cc=ming.li@zohomail.com \
    --cc=rafael@kernel.org \
    --cc=rrichter@amd.com \
    --cc=skhan@linuxfoundation.org \
    --cc=tony.luck@intel.com \
    --cc=vishal.l.verma@intel.com \
    --cc=xueshuai@linux.alibaba.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox