On Thu, 17 Oct 2024 16:15:03 -0500
Ira Weiny <ira.weiny@xxxxxxxxx> wrote:
> Jonathan Cameron wrote:
> > On Wed, 9 Oct 2024 14:49:09 -0500
> > Ira Weiny <ira.weiny@xxxxxxxxx> 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;
> > > }
> > > }
> > >
> > > - if (cnt == 0 || pl_index) {
> >
> > I thought this cnt == 0 check was to deal with the no valid
> > extents case where an empty reply is needed.
>
> Yes but the bug found by Ming needs to be handled too. I see Fan is also
> questioning this code.
>
> So... for clarity among all of us here is the new function. I'm not thrilled
> with the use of a goto but I think it is ok here.
Easy enough to avoid and I don't think it hurts readability much to do so.
Your code should work though.
>
> Ira
>
> 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;
> if (cnt == 0)
> goto send_zero_accepted;
> 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);
Why set this here - to me makes more sense to set it only once but I can
see the logic either way.
>
> 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;
>
> rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
> if (rc)
> return rc;
> cnt -= pl_index;
> pl_index = 0;
> }
> }
>
> if (!pl_index)
> return 0;
>
> send_zero_accepted:
> mbox_cmd = (struct cxl_mbox_cmd) {
> .opcode = opcode,
> .size_in = struct_size(response, extent_list,
> pl_index),
> .payload_in = response,
> };
>
> response->flags = 0;
> return cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
> }
Alternative form for what you have...
if (cnt != 0) { /* Something to send */
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) /* Space for more? */
continue;
/* Send what we have */
response->flags = 0;
if (pl_index < cnt)
response->flags &= CXL_DCD_EVENT_MORE;
mbox_cmd = (struct cxl_mbox_cmd) {
.opcode = opcode,
.size_in = struct_size(response, extent_list,
pl_index),
.payload_in = response,
};
rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
if (rc)
return rc;
cnt -= pl_index;
pl_index = 0;
}
if (!pl_index)
return 0;
}
/* Catch left overs + send if zero length */
response->flags = 0;
mbox_cmd = (struct cxl_mbox_cmd) {
.opcode = opcode,
.size_in = struct_size(response, extent_list, pl_index),
.payload_in = response,
};
return cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
}
>
