Re: [PATCH v17 01/11] PCI/AER: Introduce AER-CXL Kfifo

[Bowman, Terry]

On 5/7/2026 12:53 PM, Jonathan Cameron wrote:
> On Tue, 5 May 2026 12:30:19 -0500
> Terry Bowman <terry.bowman@xxxxxxx> wrote:
> 
>> CXL virtual hierarchy (VH) native RAS handling for CXL Port devices will be
>> added soon. This requires a notification mechanism for the AER driver to
>> share the AER interrupt with the CXL driver. The CXL drivers use the
>> notification to handle and log the CXL RAS errors.
>>
>> Note, 'CXL protocol error' terminology refers to CXL VH and not CXL RCH
>> errors unless specifically noted going forward.
>>
>> Introduce a new file in the AER driver to handle the CXL protocol
>> errors: pci/pcie/aer_cxl_vh.c.
>>
>> Add a kfifo work queue to be used by the AER and CXL drivers. Multiple
>> AER IRQ worker threads can be running and enqueueing concurrently, so
>> include write path synchronization. Pack the kfifo, the spinlock, the
>> rwsem, and the work pointer into a single structure. Initialize the
>> kfifo with INIT_KFIFO() from a subsys_initcall so its mask, esize and
>> data fields are valid before any producer or consumer runs.
>>
>> Add CXL work queue handler registration functions in the AER driver.
>> Export them so the CXL driver can assign or clear the work handler.
>>
>> Introduce 'struct cxl_proto_err_work_data' to serve as the kfifo work
>> data. It contains a reference to the PCI error source device and the
>> error severity. The cxl_core driver uses this when dequeuing the work.
>>
>> Introduce cxl_forward_error() to add a given CXL protocol error to a
>> work structure and push it onto the AER-CXL kfifo. This function takes
>> a pci_dev_get() on the source device. The kfifo consumer is responsible
>> for the matching pci_dev_put() after dequeue. On enqueue failure
>> cxl_forward_error() does the put itself.
>>
>> Synchronize accesses to the work function pointer during registration,
>> deregistration, enqueue, and dequeue.
>>
>> handle_error_source() is intentionally not changed here. The is_cxl_error()
>> switch that routes errors to cxl_forward_error() is added in a later patch
>> together with the kfifo consumer registration. This way the producer and
>> consumer land in the same commit, so CXL errors are not silently dropped
>> during bisect.
>>
>> Also add MAINTAINERS entries for both drivers/pci/pcie/aer_cxl_vh.c
>> (new in this patch) and drivers/pci/pcie/aer_cxl_rch.c (already in tree
>> but previously unlisted) under the existing CXL entry. This way the CXL
>> maintainers are CC'd on changes to the AER-CXL bridging code.
>>
>> Co-developed-by: Dan Williams <djbw@xxxxxxxxxx>
>> Signed-off-by: Dan Williams <djbw@xxxxxxxxxx>
>> Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>
> 
> Sashiko did have one comment on what happens if there are multiple things
> in the kfifo and fn fails.  At that point I think we are in the all
> bets are off corner and stranding a driver is fine, but open to other opinions!
> 
> https://sashiko.dev/#/patchset/20260505173029.2718246-1-terry.bowman%40amd.com
> 
> So with that in mind
> 
> Reviewed-by: Jonathan Cameron <jic23@xxxxxxxxxx>
> 

Hi Jonathan,

I resolved this for next series by changing __cxl_proto_err_work_fn() to return void 
as the error case was unnecessary and only added complexity. 



>> diff --git a/drivers/pci/pcie/aer_cxl_vh.c b/drivers/pci/pcie/aer_cxl_vh.c
>> new file mode 100644
>> index 000000000000..c0fea2c2b9bc
>> --- /dev/null
>> +++ b/drivers/pci/pcie/aer_cxl_vh.c
> 
> 
>> +int for_each_cxl_proto_err(struct cxl_proto_err_work_data *wd,
>> +			   cxl_proto_err_fn_t fn)
>> +{
>> +	int rc;
>> +
>> +	guard(rwsem_read)(&cxl_proto_err_kfifo.rwsem);
>> +	while (kfifo_get(&cxl_proto_err_kfifo.fifo, wd)) {
>> +		rc = fn(wd);
>> +		pci_dev_put(wd->pdev);
>> +		if (rc)
>> +			return rc;
> This is where Sashiko complains. Specifically:
> "If the consumer callback fn() returns an error, does this early return
> strand the remaining items in the kfifo?
> Because cxl_forward_error() takes a pci_dev reference for each enqueued
> item, it looks like these stranded items might leak their pci_dev references
> and prevent clean unbinding or hot-unplug until a new error triggers the
> queue again."
> 
> I'd go with indeed it does, but there is no right thing to do here. I guess
> we could flush the kfifo and call pci_dev_put() on each of them, but that's horrible.
> Would basically mean calling the same stuff you have for cancelling outstanding
> entrees on exit().
> 
> 

Yes, that is an idea. But, until error discriminator is needed this can return void.
Clearing will be necessary but I think that will fit within the call path.

-Terry


>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_FOR_MODULES(for_each_cxl_proto_err, "cxl_core");