Jonathan Cameron wrote:
> 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]
> >
[snip]
> >
> > 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.
I disagree... See below.
>
> 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.
I put it here to group it with the changing of pl_index. It is extra work.
Since I'm resending I'll make the quick change.
>
> >
> > 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...
Sure but lots of indentation on the common path which I have grown
to avoid... :-/
Looking at this fresh... A helper function works best.
static int send_one_response(struct cxl_mailbox *cxl_mbox,
struct cxl_mbox_dc_response *response,
int opcode, u32 extent_list_size, u8 flags)
{
struct cxl_mbox_cmd mbox_cmd = (struct cxl_mbox_cmd) {
.opcode = opcode,
.size_in = struct_size(response, extent_list, extent_list_size),
.payload_in = response,
};
response->extent_list_size = cpu_to_le32(extent_list_size);
response->flags = flags;
return cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
}
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_extent *extent;
unsigned long index;
u32 pl_index;
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;
if (cnt == 0)
return send_one_response(cxl_mbox, response, opcode, 0, 0);
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++;
if (pl_index == max_extents) {
u8 flags = 0;
int rc;
if (pl_index < cnt)
flags &= CXL_DCD_EVENT_MORE;
rc = send_one_response(cxl_mbox, response, opcode,
pl_index, flags);
if (rc)
return rc;
cnt -= pl_index;
pl_index = 0;
}
}
if (!pl_index) /* nothing more to do */
return 0;
return send_one_response(cxl_mbox, response, opcode, pl_index, 0);
}
