On Thu, Jun 25, 2026 at 06:24:18PM +0000, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) > to consider: > > Pre-existing issues: > - [High] Untagged extents spanning different DC partitions and DAX regions > are incorrectly coalesced and assigned to the same region.
Fixed in patch 12. extract_tag_group() originally put all untagged extents into the same tag_group. This has been fixed to put each untagged extent into its own tag_group so they are not coalesced and assigned to the same region. > - [High] Fully duplicate extent groups are incorrectly rejected and omitted > from the device response. Duplicate false positive from patch 14 and 16. > -- > > commit c2cf18ae81b1c32953f39b06aeaa9e153f4ac609 > Author: Anisa Su <[email protected]> > > cxl/mem: Enforce tag-group semantics > > This commit completes tag-group validation. It adds sequence integrity > checks to verify that sharable partitions have dense shared_extn_seq > values, and verifies that tagged allocations do not span DC partitions. > > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > > index 14ba263044f08..7967b0db2c512 100644 > > --- a/drivers/cxl/core/mbox.c > > +++ b/drivers/cxl/core/mbox.c > > [ ... ] > > > +static int cxl_check_group_partition(struct cxl_memdev_state *mds, > > + const uuid_t *tag, > > + const struct list_head *group) > > +{ > > + struct device *dev = mds->cxlds.dev; > > + const struct cxl_dpa_partition *first_part = NULL; > > + u64 first_dpa = 0; > > + struct cxl_extent_list_node *pos; > > + > > + if (uuid_is_null(tag) || list_empty(group)) > > + return 0; > > [Severity: High] > This is a pre-existing issue, but does this explicit bypass for untagged > extents allow them to span different DC partitions? > > If a device sends multiple untagged DC add capacity events spanning > different partitions, cxl_add_pending() appears to extract them all into a > single group. > > Since this check returns 0 for null UUIDs, cxl_realize_group() processes them > together. Would cxlr_add_extent() then register them all under the DAX region > of the first extent, effectively merging extents from different partitions > into > the same region's resource tree? > > [ ... ] > > > static int cxl_realize_group(struct cxl_memdev_state *mds, const uuid_t > > *tag, > > - struct list_head *group, bool existing) > > + struct list_head *group, bool shareable, > > + bool existing) > > { > > struct device *dev = mds->cxlds.dev; > > struct cxl_extent_list_node *pos, *tmp; > > struct cxl_dc_tag_group *tag_group; > > int group_cnt = 0; > > + u16 seq_num; > > int rc; > > > > + seq_num = -1; > > list_for_each_entry_safe(pos, tmp, group, list) { > > - /* > > - * Pass the device-stamped 0-based shared_extn_seq through > > - * unchanged as the dax-side @seq_num (0..n-1). > > - */ > > - u16 seq = le16_to_cpu(pos->extent->shared_extn_seq); > > + if (shareable) > > + seq_num = le16_to_cpu(pos->extent->shared_extn_seq); > > + else > > + seq_num++; > > > > - if (cxl_add_extent(mds, pos->extent, seq) < 0) { > > + if (cxl_add_extent(mds, pos->extent, seq_num) < 0) { > > [Severity: High] > This is a pre-existing issue, but what happens if a device resends a tag group > where all extents have already been accepted? > > If cxl_add_extent() detects a duplicate, it returns 0 without calling > cxlr_add_extent(). If this happens for every extent in the group, > mds->add_ctx.group is never allocated and remains NULL. > > Does this cause the subsequent if (!tag_group) check in this function to drop > the group and return -ENXIO instead of returning group_cnt to acknowledge > the extents back to the device? > > > dev_dbg(dev, > > "Tag %pUb: failed to add extent DPA:%#llx > > LEN:%#llx\n", > > tag, > > @@ -1412,15 +1532,22 @@ static int cxl_realize_group(struct > > cxl_memdev_state *mds, const uuid_t *tag, > > -- > Sashiko AI review ยท > https://sashiko.dev/#/patchset/[email protected]?part=17
