Li, Ming4 wrote:
> On 10/8/2024 7:16 AM, ira.weiny@xxxxxxxxx wrote:
> > From: Navneet Singh <navneet.singh@xxxxxxxxx>
> >
[snip]
> >
> > Signed-off-by: Navneet Singh <navneet.singh@xxxxxxxxx>
> > Co-developed-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> >
> Hi Ira,
>
> I guess you missed my comments for V3, I comment it again for this patch.
Apologies. Yes I totally missed your reply. :-(
>
> > +static bool extents_contain(struct cxl_dax_region *cxlr_dax,
> > + struct cxl_endpoint_decoder *cxled,
> > + struct range *new_range)
> > +{
> > + struct device *extent_device;
> > + struct match_data md = {
> > + .cxled = cxled,
> > + .new_range = new_range,
> > + };
> > +
> > + extent_device = device_find_child(&cxlr_dax->dev, &md, match_contains);
> > + if (!extent_device)
> > + return false;
> > +
> > + put_device(extent_device);
> could use __free(put_device) to drop this 'put_device(extent_device)'
Yep.
> > + return true;
> > +}
> [...]
> > +static bool extents_overlap(struct cxl_dax_region *cxlr_dax,
> > + struct cxl_endpoint_decoder *cxled,
> > + struct range *new_range)
> > +{
> > + struct device *extent_device;
> > + struct match_data md = {
> > + .cxled = cxled,
> > + .new_range = new_range,
> > + };
> > +
> > + extent_device = device_find_child(&cxlr_dax->dev, &md, match_overlaps);
> > + if (!extent_device)
> > + return false;
> > +
> > + put_device(extent_device);
> Same as above.
Done.
> > + return true;
> > +}
> > +
> [...]
> > +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;
}
}
- 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]
