On 10/10/2024 3:49 AM, Ira Weiny wrote: > Li, Ming4 wrote: >> On 10/8/2024 7:16 AM, ira.weiny@xxxxxxxxx wrote: >>> From: Navneet Singh <navneet.singh@xxxxxxxxx> >>> [snip] >>> +static int cxl_send_dc_response(struct cxl_memdev_state *mds, int opcode, >>> + struct xarray *extent_array, int cnt) >>> +{ >>> + struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox; >>> + struct cxl_mbox_dc_response *p; >>> + struct cxl_mbox_cmd mbox_cmd; >>> + struct cxl_extent *extent; >>> + unsigned long index; >>> + u32 pl_index; >>> + int rc; >>> + >>> + size_t pl_size = struct_size(p, extent_list, cnt); >>> + u32 max_extents = cnt; >>> + >>> + /* May have to use more bit on response. */ >>> + if (pl_size > cxl_mbox->payload_size) { >>> + max_extents = (cxl_mbox->payload_size - sizeof(*p)) / >>> + sizeof(struct updated_extent_list); >>> + pl_size = struct_size(p, extent_list, max_extents); >>> + } >>> + >>> + struct cxl_mbox_dc_response *response __free(kfree) = >>> + kzalloc(pl_size, GFP_KERNEL); >>> + if (!response) >>> + return -ENOMEM; >>> + >>> + pl_index = 0; >>> + xa_for_each(extent_array, index, extent) { >>> + >>> + response->extent_list[pl_index].dpa_start = extent->start_dpa; >>> + response->extent_list[pl_index].length = extent->length; >>> + pl_index++; >>> + response->extent_list_size = cpu_to_le32(pl_index); >>> + >>> + if (pl_index == max_extents) { >>> + mbox_cmd = (struct cxl_mbox_cmd) { >>> + .opcode = opcode, >>> + .size_in = struct_size(response, extent_list, >>> + pl_index), >>> + .payload_in = response, >>> + }; >>> + >>> + response->flags = 0; >>> + if (pl_index < cnt) >>> + response->flags &= CXL_DCD_EVENT_MORE; >> It should be 'response->flags |= CXL_DCD_EVENT_MORE' here. > Ah yea. Good catch. > >> Another issue is if 'cnt' is N times bigger than 'max_extents'(e,g. cnt=20, max_extents=10). all responses will be sent in this xa_for_each(), and CXL_DCD_EVENT_MORE will be set in the last response but it should not be set in these cases. >> > Ah yes. cnt must be decremented. As I looked at the patch just now the > > if (cnt == 0 || pl_index) > > ... seemed very wrong to me. That change masked this bug. > > This should fix it: > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index d66beec687a0..99200274dea8 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -1119,10 +1119,11 @@ static int cxl_send_dc_response(struct cxl_memdev_state *mds, int opcode, > if (rc) > return rc; > pl_index = 0; > + cnt -= pl_index; should update cnt before pl_index is reset to 0. the cnt is a one of parameters of cxl_send_dc_response(), that means the caller gives the value of cnt, is that possible if the size of extent_array is larger than cnt? Should skip remain extents in extent_array when cnt is equal to 0? > } > } > > - if (cnt == 0 || pl_index) { > + if (pl_index) { > mbox_cmd = (struct cxl_mbox_cmd) { > .opcode = opcode, > .size_in = struct_size(response, extent_list, > > > Thank you, and sorry again for missing your feedback. > > Ira > > [snip]