Re: [PATCH v11 13/31] cxl/mem: Add 20 second timeout for stalled DC_ADD_CAPACITY chains

On Thu, Jun 25, 2026 at 06:15:43PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Error paths in handle_add_event fail to abort the DCD event chain properly, leading to a stalled device, leaked watchdog timers, and partial chain acceptance.
Fixed.

> - [High] Watchdog timeout clears the pending list without transitioning to a discard state, leading to a double-response protocol violation if the chain completes.

Unlikely and also a spec issue.

> --
> 
> commit f4b4a95fd0c56d91f06baa4811e9a05ea46012ee
> Author: Anisa Su <[email protected]>
> 
> cxl/mem: Add 20 second timeout for stalled DC_ADD_CAPACITY chains
> 
> A DC_ADD_CAPACITY event can span multiple event records. This patch adds a
> delayed-work watchdog to prevent indefinitely stalled chains and enforces a
> maximum limit on the number of pending extents.
> 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 7dd40fb8d613c..4e887b5cdc3ed 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> 
> [ ... ]
> 
> > @@ -1208,15 +1208,78 @@ static void clear_pending_extents(void *_mds)
> >  
> >  	list_for_each_entry_safe(pos, tmp, &mds->add_ctx.pending_extents, list)
> >  		delete_extent_node(pos);
> > +	mds->add_ctx.nr_pending = 0;
> >  	mds->add_ctx.group = NULL;
> >  }
> 
> [ ... ]
> 
> > +static void cxl_dc_add_timeout(struct work_struct *work)
> > +{
> > +	struct pending_add_ctx *ctx = container_of(to_delayed_work(work),
> > +						   struct pending_add_ctx,
> > +						   timeout_work);
> 
> [ ... ]
> 
> > +	if (cxl_send_dc_response(mds, CXL_MBOX_OP_ADD_DC_RESPONSE,
> > +				 &ctx->pending_extents, 0))
> > +		dev_dbg(dev, "Failed to send empty ADD_DC_RESPONSE on timeout\n");
> > +
> > +	clear_pending_extents(mds);
> > +	ctx->armed = false;
> > +}
> 
> [Severity: High]
> If the device is just slow and later sends the remainder of the chain
> (including the closing More=0 event), will the driver treat the delayed
> events as a fresh chain?
> 
It will, but the kernel has no way of discerning the remainder of an
aborted chain from a new chain.

And this would only occur if the device exceeds the 20s timeout in the
first place, which is unlikely. If this needs to be fixed, a More chain
ID would have to be added to the spec.

> Because cxl_dc_add_timeout() resets ctx->armed = false and nr_pending = 0
> without marking the chain as discarded, it appears handle_add_event() would
> process a delayed tail of the chain as a new chain.
> 
> When the More=0 event is processed, wouldn't the driver issue a second
> ADD_DC_RESPONSE for this single event chain, violating the CXL protocol?
> 
> [ ... ]
> 
> > @@ -1239,10 +1303,20 @@ static int add_to_pending_list(struct list_head *pending_list,
> >  static int handle_add_event(struct cxl_memdev_state *mds,
> >  			    struct cxl_event_dcd *event)
> >  {
> > +	struct pending_add_ctx *ctx = &mds->add_ctx;
> >  	struct device *dev = mds->cxlds.dev;
> >  	int rc;
> >  
> > -	rc = add_to_pending_list(&mds->add_ctx.pending_extents, &event->extent);
> > +	guard(mutex)(&ctx->lock);
> > +
> > +	if (ctx->nr_pending >= CXL_DC_MAX_PENDING_EXTENTS) {
> > +		dev_warn(dev, "DC add chain exceeds %u extents; dropping (firmware bug)\n",
> > +			 CXL_DC_MAX_PENDING_EXTENTS);
> > +		clear_pending_extents(mds);
> > +		return -ENOSPC;
> > +	}
> > +
> > +	rc = add_to_pending_list(&ctx->pending_extents, &event->extent);
> >  	if (rc) {
> >  		clear_pending_extents(mds);
> >  		return rc;
> 
> [Severity: High]
> Do these early error returns fail to abort the event chain properly?
> 
> If the limit is exceeded or memory allocation fails, the code calls
> clear_pending_extents() and returns an error without sending an
> ADD_DC_RESPONSE.
> 
> If this happens on the first extent, wouldn't the device never receive a
> response and stall indefinitely?
> 
> If it occurs on a subsequent extent, does this leave the watchdog timer
> armed to fire spuriously later?
> 
> Additionally, because clear_pending_extents() resets nr_pending to 0, if
> the device continues sending More=1 events for the same chain, would the
> driver incorrectly process them as a new chain and potentially accept the
> tail of a chain after dropping the head?
> 
Fixed.

rc = add_to_pending_list(&ctx->pending_extents, &event->extent);
if (rc) {
	clear_pending_extents(mds);
	/*
	 * If More chain only has 1 record and errors before watchdog is
	 * armed, cxl_add_pending will never be reached, so no response
	 * is sent. Return an empty response. Also sends empty response
	 * for multi-record chain pre-emptively rather than waiting for
	 * watchdog to trigger.
	 */
	if (!(event->flags & CXL_DCD_EVENT_MORE)) {
		ctx->armed = false;
		cancel_delayed_work(&ctx->timeout_work);
		if (cxl_send_dc_response(mds, CXL_MBOX_OP_ADD_DC_RESPONSE,
					 &ctx->pending_extents, 0))
			dev_dbg(dev, "Failed to refuse chain after staging error\n");
	}
	return rc;
}

> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13