Re: [PATCH v11 12/31] cxl/mem: Set up framework for handling DC Events

On Thu, Jun 25, 2026 at 06:12:18PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Missing DCD response on memory allocation failure leaves the device state machine permanently blocked while unconditionally clearing the event log.
fixed

> - [Medium] Stack-allocation optimization in `memdev_release_extent()` is defeated by an internal dynamic allocation in `cxl_send_dc_response()`.
false pos

> - [Medium] Invalid `__counted_by` attribute on a little-endian struct member breaks bounds checking on big-endian architectures.
fixed

> --
> 
> commit f5333550094fcaacb4e98dbf0c270b32ec17b506
> Author: Ira Weiny <[email protected]>
> 
> cxl/mem: Set up framework for handling DC Events
>     
> Adds the support for receiving DC event records but defers
> the real add/release logic to subsequent commits. Simply refuse all
> extents for DC_ADD and ack all DC_RELEASE events for now. Forced
> release is currently unsupported.
> 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> 
> [ ... ]
> 
> > +static void memdev_release_extent(struct cxl_memdev_state *mds, struct range *range)
> > +{
> > +	struct device *dev = mds->cxlds.dev;
> > +	struct cxl_extent extent = {
> > +		.start_dpa = cpu_to_le64(range->start),
> > +		.length = cpu_to_le64(range_len(range)),
> > +	};
> > +	struct cxl_extent_list_node node = { .extent = &extent };
> > +	LIST_HEAD(extent_list);
> > +
> > +	dev_dbg(dev, "Release response dpa %pra\n", range);
> > +
> > +	/*
> > +	 * Stack-allocated instead of kzalloc'ed to avoid potential -ENOMEM,
> > +	 * which would prevent sending the release DC rsp.
> > +	 */
> > +	list_add_tail(&node.list, &extent_list);
> > +	if (cxl_send_dc_response(mds, CXL_MBOX_OP_RELEASE_DC, &extent_list, 1))
> 
> [Severity: Medium]
> The comment mentions stack-allocating to avoid -ENOMEM so we can ensure the
> release DC response is sent.
> 
> However, cxl_send_dc_response() uses kzalloc() to allocate the
> cxl_mbox_dc_response message:
> 
>     struct cxl_mbox_dc_response *response __free(kfree) =
>                         kzalloc(pl_size, GFP_KERNEL);
> 
> Doesn't this dynamic allocation defeat the optimization, since memory
> pressure could still cause kzalloc() to fail, preventing the release
> response from being sent?
> 
No.

In cxl_send_dc_response(), we send an empty rsp if kzalloc() fails.

struct cxl_mbox_dc_response *response __free(kfree) =
						kzalloc(pl_size, GFP_KERNEL);
if (!response) {
	/* Refuse all extents rather than stall the device. */
	struct cxl_mbox_dc_response empty = { };
	return send_one_response(cxl_mbox, &empty, opcode, 0, 0);
}

Originally, this function kzalloc'ed the node that was the input for
cxl_send_dc_response. If that failed, we would never reach
cxl_dc_response(), and no rsp would be sent, which is why it was changed
to stack allocation.

Anyway the comment is deleted since it seems misleading and stack-allocating
two small local structs for a single extent is the obvious choice and does need
extra justification. There is no issue of correctness.

> > +		dev_dbg(dev, "Failed to release %pra\n", range);
> > +}
> 
> [ ... ]
> 
> > +static int handle_add_event(struct cxl_memdev_state *mds,
> > +			    struct cxl_event_dcd *event)
> > +{
> > +	struct device *dev = mds->cxlds.dev;
> > +	int rc;
> > +
> > +	rc = add_to_pending_list(&mds->add_ctx.pending_extents, &event->extent);
> > +	if (rc) {
> > +		clear_pending_extents(mds);
> > +		return rc;
> > +	}
> 
> [Severity: High]
> If add_to_pending_list() fails and returns -ENOMEM, we return early without
> sending the required CXL_MBOX_OP_ADD_DC_RESPONSE command.
> 
> Since cxl_handle_dcd_event_records() drops the error and the event is cleared
> from the device log, will this leave the device state machine permanently
> blocked waiting for a response that never arrives?
> 
Fixed.

This only applies in the case for a More chain with a single event
record. So the first and only event record has More=0 and the 20s
watchdog is never armed. Otherwise, the watchdog would catch it.

However, we may as well send an empty response anyway for both cases. So
the fix is:

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;
}

> > +
> > +	if (event->flags & CXL_DCD_EVENT_MORE) {
> 
> [ ... ]
> 
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> 
> [ ... ]
> 
> > +/*
> > + * Add Dynamic Capacity Response
> > + * CXL rev 3.1 section 8.2.9.9.9.3; Table 8-168 & Table 8-169
> > + */
> > +struct cxl_mbox_dc_response {
> > +	__le32 extent_list_size;
> > +	u8 flags;
> > +	u8 reserved[3];
> > +	struct updated_extent_list {
> > +		__le64 dpa_start;
> > +		__le64 length;
> > +		u8 reserved[8];
> > +	} __packed extent_list[] __counted_by(extent_list_size);
> 
> [Severity: Medium]
> Should this use __counted_by_le() instead of __counted_by()?
> 
> Since extent_list_size is explicitly little-endian (__le32), using
> __counted_by() on big-endian architectures will result in the bounds checking
> instrumentation reading an inflated integer value, leading to out-of-bounds
> errors or kernel panics.
> 
Fixed.

Also updated spec reference to r4.0 Section 8.2.10.9.9.3; Table 8-350 & Table 8-351

> > +} __packed;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12