Re: [PATCH v1 04/14] cxl/mem: Introduce DC event handlers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] cxl/mem: incomplete chain abort on staging failure in handle_dcd_chain_event()
- [Medium] cxl/mem: missing chain abort in cxl_handle_dcd_event_records()
--

commit 5ef1f10166c3a30ee58534de97a6b9ec5412cf04
Author: Ira Weiny <iweiny@kernel.org>

cxl/mem: Introduce DC event handlers

This commit adds the event handling skeleton for Dynamic Capacity (DC) Add
and Release event chains in the CXL mailbox driver. Chained records with the
CXL_DCD_EVENT_MORE flag set are staged in mds->dc_chain until the closing
record arrives, at which point Add chains are refused with an empty response
and Release chains are acknowledged.

> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 13a68f5cef692..3acf5ace48dbc 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
[ ... ]
> +/*
> + * Drop the open chain.  An add chain is refused with an empty response so
> + * the device can reclaim the offered capacity; a release chain is simply
> + * dropped, the capacity stays with the host and the device may retry.
> + */
> +static void cxl_dc_chain_abort(struct cxl_memdev_state *mds)
> +{
> +	struct cxl_dc_event_chain *chain = &mds->dc_chain;
> +	struct device *dev = mds->cxlds.dev;
> +
> +	if (chain->event_type == DCD_ADD_CAPACITY &&
> +	    cxl_send_dc_response(mds, CXL_MBOX_OP_ADD_DC_RESPONSE,
> +				 &chain->pending_extents, 0))
> +		dev_dbg(dev, "Failed to refuse add chain\n");
> +
> +	clear_pending_extents(mds);
> +}
> +
> +static int stage_pending_extent(struct list_head *pending,
> +				struct cxl_extent *to_add)
> +{
> +	struct cxl_extent_list_node *node = kzalloc(sizeof(*node), GFP_KERNEL);
> +	struct cxl_extent *extent;
> +
> +	if (!node)
> +		return -ENOMEM;
> +	extent = kmemdup(to_add, sizeof(*extent), GFP_KERNEL);
> +	if (!extent) {
> +		kfree(node);
> +		return -ENOMEM;
> +	}
> +
> +	node->extent = extent;
> +	list_add_tail(&node->list, pending);
> +	return 0;
> +}
> +
> +static int handle_dcd_chain_event(struct cxl_memdev_state *mds,
> +				  struct cxl_event_dcd *event)
> +{
> +	struct cxl_dc_event_chain *chain = &mds->dc_chain;
> +	struct device *dev = mds->cxlds.dev;
> +	int rc;
> +
> +	/* Chained records share one event type (CXL r4.0 Table 8-229) */
> +	if (list_empty(&chain->pending_extents)) {
> +		chain->event_type = event->event_type;
> +	} else if (chain->event_type != event->event_type) {
> +		dev_warn_ratelimited(dev, "DC %s record inside an open %s chain; dropping the chain\n",
> +				     cxl_dcd_evt_type_str(event->event_type),
> +				     cxl_dcd_evt_type_str(chain->event_type));
> +		cxl_dc_chain_abort(mds);
> +		chain->event_type = event->event_type;
> +	}
> +
> +	rc = stage_pending_extent(&chain->pending_extents, &event->extent);
> +	if (rc) {
> +		cxl_dc_chain_abort(mds);
> +		return rc;
> +	}

[Severity: Medium]
If stage_pending_extent() fails with -ENOMEM on a non-terminal record in a
chain (where event->flags has CXL_DCD_EVENT_MORE set), what prevents the
remaining records of that aborted chain from being processed as a new chain?

cxl_dc_chain_abort() sends CXL_MBOX_OP_ADD_DC_RESPONSE (for DCD_ADD_CAPACITY)
and empties chain->pending_extents via clear_pending_extents(). However,
cxl_mem_get_records_log() continues iterating through payload->records[i] and
passing subsequent records of the same chain into
cxl_handle_dcd_event_records() -> handle_dcd_chain_event().

Because handle_dcd_chain_event() checks list_empty(&chain->pending_extents) to
determine whether a chain is open, the next record in the aborted chain sees an
empty list and starts staging the remainder of the chain as a fresh operation.
When the closing record (More=0) arrives, a DCD_RELEASE_CAPACITY chain will
send CXL_MBOX_OP_RELEASE_DC for only the trailing subset of extents instead of
dropping the chain, and a DCD_ADD_CAPACITY chain will send a duplicate
CXL_MBOX_OP_ADD_DC_RESPONSE.

Should struct cxl_dc_event_chain track an aborted state until the closing
record with CXL_DCD_EVENT_MORE cleared is consumed?

> +
> +	if (event->flags & CXL_DCD_EVENT_MORE) {
> +		dev_dbg(dev, "more bit set; delay processing of extent\n");
> +		return 0;
> +	}
> +
> +	/* Nothing is tracked yet: refuse every add, acknowledge every release */
> +	if (event->event_type == DCD_ADD_CAPACITY)
> +		rc = cxl_send_dc_response(mds, CXL_MBOX_OP_ADD_DC_RESPONSE,
> +					  &chain->pending_extents, 0);
> +	else
> +		rc = cxl_send_dc_response(mds, CXL_MBOX_OP_RELEASE_DC,
> +					  &chain->pending_extents,
> +					  list_count_nodes(&chain->pending_extents));
> +	clear_pending_extents(mds);
> +	return rc;
> +}
> +
> +static void cxl_handle_dcd_event_records(struct cxl_memdev_state *mds,
> +					 struct cxl_event_record_raw *raw_rec)
> +{
> +	struct cxl_event_dcd *event = &raw_rec->event.dcd;
> +	struct cxl_extent *extent = &event->extent;
> +	struct device *dev = mds->cxlds.dev;
> +	uuid_t *id = &raw_rec->id;
> +	int rc;
> +
> +	if (!uuid_equal(id, &CXL_EVENT_DC_EVENT_UUID))
> +		return;
> +
> +	dev_dbg(dev, "DCD event %s : DPA:%#llx LEN:%#llx\n",
> +		cxl_dcd_evt_type_str(event->event_type),
> +		le64_to_cpu(extent->start_dpa), le64_to_cpu(extent->length));
> +
> +	switch (event->event_type) {
> +	case DCD_ADD_CAPACITY:
> +	case DCD_RELEASE_CAPACITY:
> +		rc = handle_dcd_chain_event(mds, event);
> +		break;
> +	case DCD_FORCED_CAPACITY_RELEASE:
> +		dev_err_ratelimited(dev, "Forced release event ignored.\n");
> +		rc = 0;
> +		break;
> +	case DCD_REGION_CONFIGURATION_UPDATED:
> +		dev_dbg(dev, "Region configuration updated event ignored.\n");
> +		rc = 0;
> +		break;
> +	default:
> +		rc = -EINVAL;
> +		break;
> +	}

[Severity: Medium]
What happens if a DCD_FORCED_CAPACITY_RELEASE,
DCD_REGION_CONFIGURATION_UPDATED, unknown event_type, or non-DC UUID record
arrives in the DCD log while mds->dc_chain.pending_extents is non-empty?

Currently, the check for an open chain is only performed inside
handle_dcd_chain_event(), which is only called for DCD_ADD_CAPACITY and
DCD_RELEASE_CAPACITY. If another event record arrives in the middle of an open
chain, cxl_handle_dcd_event_records() does not call cxl_dc_chain_abort(mds).

As a result, an interrupted Add chain is not refused when broken by the
intervening record, and if the next chain shares the same event_type as the
interrupted one, handle_dcd_chain_event() sees chain->event_type ==
event->event_type and merges the stale extents from the broken chain into the
new chain.

Should cxl_handle_dcd_event_records() abort any open chain when receiving
these non-chain records?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104230445.66625-1-anisa.su@samsung.com?part=4