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
