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. - [High] Watchdog timeout clears the pending list without transitioning to a discard state, leading to a double-response protocol violation if the chain completes. --
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? 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? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
