Re: [PATCH v11 10/31] cxl/mem: Configure dynamic capacity interrupts

On Thu, Jun 25, 2026 at 06:14:41PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] An unhandled DCD event status bit will cause an infinite loop in the `cxl_event_thread` IRQ handler if DCD processing is disabled dynamically.
Fixed

> - [High] The driver fails to perform an initial drain of the DCD event log when the BIOS has control of standard CXL errors (`!native_cxl`), stranding pre-existing events.
Fixed

> --
> 
> commit 4e2d7455b6a9875d8ee530fc91ae3a8b65741caa
> Author: Ira Weiny <[email protected]>
> 
> cxl/mem: Configure dynamic capacity interrupts
> 
> Dynamic Capacity Devices (DCD) support extent change notifications
> through the event log mechanism.  The interrupt mailbox commands were
> extended in CXL 3.1 to support these notifications.  Firmware can't
> configure DCD events to be FW controlled but can retain control of
> memory events.
> 
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index 8d12c684d6700..95a4bf7c1e466 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> [ ... ]
> > @@ -632,6 +657,30 @@ static int cxl_event_irqsetup(struct cxl_memdev_state *mds,
> >  	return 0;
> >  }
> >  
> > +static int cxl_irqsetup(struct cxl_memdev_state *mds,
> > +			struct cxl_event_interrupt_policy *policy,
> > +			bool native_cxl)
> > +{
> > +	struct cxl_dev_state *cxlds = &mds->cxlds;
> > +	int rc;
> > +
> > +	if (native_cxl) {
> > +		rc = cxl_event_irqsetup(mds, policy);
> > +		if (rc)
> > +			return rc;
> > +	}
> > +
> > +	if (cxl_dcd_supported(mds)) {
> > +		rc = cxl_event_req_irq(cxlds, policy->dcd_settings);
> > +		if (rc) {
> > +			dev_err(cxlds->dev, "Failed to get interrupt for DCD event log\n");
> > +			cxl_disable_dcd(mds);
> 
> [Severity: High]
> If cxl_event_req_irq() fails here, cxl_disable_dcd() is called, but the
> device is already programmed to generate DCD interrupts.
> 
> When a DCD event interrupt fires, cxl_event_thread() will read the status
> and retain the CXLDEV_EVENT_STATUS_DCD bit because the mask includes it.
> However, cxl_mem_get_event_records() skips draining the log when
> cxl_dcd_supported() is false:
> 
> drivers/cxl/core/mbox.c:cxl_mem_get_event_records() {
>     ...
>     if (cxl_dcd_supported(mds) && (status & CXLDEV_EVENT_STATUS_DCD))
>         cxl_mem_get_records_log(mds, CXL_EVENT_TYPE_DCD);
>     ...
> }
> 
> Will this cause cxl_event_thread() to spin in an infinite loop since the
> status bit is never cleared by draining the log?
> 

The fix is in cxl_event_thread(). Instead of setting the event drain
mask based only on the value of native_cxl:

mask = host_bridge->native_cxl_error ? CXLDEV_EVENT_STATUS_ALL
                                     : CXLDEV_EVENT_STATUS_DCD;

we additionally check cxl_dcd_supported(), because the native_cxl value
only determines if the standard logs are owned by firmware. DCD event
logs are always OS owned.

So the mask becomes:

u32 mask = 0;

if (host_bridge->native_cxl_error)
        mask |= CXLDEV_EVENT_STATUS_ALL & ~CXLDEV_EVENT_STATUS_DCD;
if (cxl_dcd_supported(mds))
        mask |= CXLDEV_EVENT_STATUS_DCD;

This gets put in a new helper function cxl_event_drain_mask() because
the second bug below uses it too.

The helper is called from cxl_event_thread():
static irqreturn_t cxl_event_thread(int irq, void *id)
{
	struct cxl_dev_id *dev_id = id;
	struct cxl_dev_state *cxlds = dev_id->cxlds;
	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
	struct pci_host_bridge *host_bridge =
		pci_find_host_bridge(to_pci_dev(cxlds->dev)->bus);
	u32 mask = cxl_event_drain_mask(host_bridge, mds);
...

So if dcd has been disabled afterwards, DCD events are still cleared.

> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> [ ... ]
> > @@ -687,11 +744,16 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge,
> >  	if (rc)
> >  		return rc;
> >  
> > -	rc = cxl_event_irqsetup(mds, &policy);
> > +	rc = cxl_irqsetup(mds, &policy, native_cxl);
> >  	if (rc)
> >  		return rc;
> >  
> > -	cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL);
> > +	if (native_cxl)
> > +		cxl_mem_get_event_records(mds, CXLDEV_EVENT_STATUS_ALL);
> 
> [Severity: High]
> If the BIOS has control of standard CXL errors (!native_cxl), does this skip
> the initial drain of the DCD event log?
> 
> Since the driver owns the DCD log regardless of native_cxl, it appears that
> pre-existing DCD events might be left in the log, potentially preventing
> subsequent edge-triggered interrupts from firing.
> 

Fixed by calling previous defined helper cxl_event_drain_mask()
which also checks if dcd is enabled.

So if (native_cxl)
	cxl_mem_get_event_records()

is replaced by

status = cxl_event_drain_mask(host_bridge, mds);
if (status)
	cxl_mem_get_event_records(mds, status);

> > +
> > +	dev_dbg(mds->cxlds.dev, "Event config : %s DCD %s\n",
> > +		native_cxl ? "OS" : "BIOS",
> > +		cxl_dcd_supported(mds) ? "supported" : "not supported");
> >  
> >  	return 0;
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10