> -----Original Message-----
> From: Teres Alexis, Alan Previn <alan.previn.teres.alexis@xxxxxxxxx>
> Sent: Thursday, August 04, 2022 01:08
> To: Ceraolo Spurio, Daniele <daniele.ceraolospurio@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Winkler, Tomas <tomas.winkler@xxxxxxxxx>; Lubart, Vitaly
> <vitaly.lubart@xxxxxxxxx>
> Subject: Re: [PATCH 02/15] mei: add support to GSC extended header
>
> I was informed by Daniele that the MEI team had done the functional
> reviews as part of their differentiated "Signed-of- by" process and so i was
> asked to only do a at the surface code-syntax / code-structure review.
>
> That said i did find one issue farther below.
>
> I'm also compelled to add the following nit, but this is about prior code, not
> new code in these patches so you can choose to ignore this: In
> mei_cl_irq_write, mei_cl_write and mei_cl_irq_read_msg functions, there
> are multiple blocks of code that reference header-index, header-lenght,
> buffer-data, buffer-size, dr-slots and other ptr + sizing related variables in
> different layers to decide validity of data, validity of size, ability for splitting
> data or extending via dma-rings and other code flows I can't really make out.
> It would be nice to have separate helpers with self-explanatory names and
> added comments about what these blocks of code are trying to do and how
> they interact with the e2e flow of sending data or receiving data via the irq
> and message lists.
Thanks for the input, will try to address that comment, in later patches.
>
>
> ...alan
>
>
> On Thu, 2022-06-09 at 16:19 -0700, Ceraolo Spurio, Daniele wrote:
> > From: Tomas Winkler <tomas.winkler@xxxxxxxxx>
> >
> > GSC extend header is of variable size and data is provided in a sgl
> > list inside the header and not in the data buffers, need to enable the
> > path.
> >
> > diff --git a/drivers/misc/mei/interrupt.c
> > b/drivers/misc/mei/interrupt.c index 0706322154cb..0a0e984e5673 100644
> > --- a/drivers/misc/mei/interrupt.c
> > +++ b/drivers/misc/mei/interrupt.c
> > @@ -98,9 +98,12 @@ static int mei_cl_irq_read_msg(struct mei_cl *cl,
> > struct mei_device *dev = cl->dev;
> > struct mei_cl_cb *cb;
> >
> > + struct mei_ext_hdr_vtag *vtag_hdr = NULL;
> > + struct mei_ext_hdr_gsc_f2h *gsc_f2h = NULL;
> > +
> > size_t buf_sz;
> > u32 length;
> > - int ext_len;
> > + u32 ext_len;
> >
> > length = mei_hdr->length;
> > ext_len = 0;
> > @@ -122,18 +125,24 @@ static int mei_cl_irq_read_msg(struct mei_cl *cl,
> > }
> >
> > if (mei_hdr->extended) {
> > - struct mei_ext_hdr *ext;
> > - struct mei_ext_hdr_vtag *vtag_hdr = NULL;
> > -
> > - ext = mei_ext_begin(meta);
> > + struct mei_ext_hdr *ext = mei_ext_begin(meta);
> > do {
> > switch (ext->type) {
> > case MEI_EXT_HDR_VTAG:
> > vtag_hdr = (struct mei_ext_hdr_vtag *)ext;
> > break;
> > + case MEI_EXT_HDR_GSC:
> > + gsc_f2h = (struct mei_ext_hdr_gsc_f2h
> *)ext;
> > + cb->ext_hdr = kzalloc(sizeof(*gsc_f2h),
> GFP_KERNEL);
> > + if (!cb->ext_hdr) {
> > + cb->status = -ENOMEM;
> > + goto discard;
> > + }
> > + break;
> > case MEI_EXT_HDR_NONE:
> > fallthrough;
> > default:
> > + cl_err(dev, cl, "unknown extended
> header\n");
> > cb->status = -EPROTO;
> > break;
> > }
> > @@ -157,6 +168,28 @@ static int mei_cl_irq_read_msg(struct mei_cl *cl,
> > cb->vtag = vtag_hdr->vtag;
> > }
> >
> > + if (gsc_f2h) {
> > + u32 ext_hdr_len = mei_ext_hdr_len(&gsc_f2h->hdr);
> > +
> > + if (!dev->hbm_f_gsc_supported) {
> > + cl_err(dev, cl, "gsc extended header is not
> supported\n");
> > + cb->status = -EPROTO;
> > + goto discard;
> >
> Alan: It looks to me that code jump where "discard" begins, puts cb back into
> a linked list for future re-use.
> However, it doesn't free cb->ext_hdr (or at least from what i can tell). Thus,
> if we already allocated cb->ext_hdr (from above when "case
> MEI_EXT_HDR_GSC:" is true, and if we end up discarding here or in the
> following few lines, then we may end up leaking memory if we dont free cb-
> >ext_hdr between discard and next reuse.
The cb is not reused, it is put on the completion list, all completed cbs freed via
mei_io_cb_free() function which also frees the ext_hdr.
> > + }
> > +
> > + if (length) {
> > + cl_err(dev, cl, "no data allowed in cb with gsc\n");
> > + cb->status = -EPROTO;
> > + goto discard;
> > + }
> > + if (ext_hdr_len > sizeof(*gsc_f2h)) {
> > + cl_err(dev, cl, "gsc extended header is too big %u\n",
> ext_hdr_len);
> > + cb->status = -EPROTO;
> > + goto discard;
> > + }
> > + memcpy(cb->ext_hdr, gsc_f2h, ext_hdr_len);
> > + }
> > +
> > if (!mei_cl_is_connected(cl)) {
> > cl_dbg(dev, cl, "not connected\n");
> > cb->status = -ENODEV;
> > diff --git a/drivers/misc/mei/mei_dev.h b/drivers/misc/mei/mei_dev.h
> > index 7c508bca9a00..862190b297aa 100644
> > --- a/drivers/misc/mei/mei_dev.h
> > +++ b/drivers/misc/mei/mei_dev.h
> > @@ -211,6 +211,7 @@ struct mei_cl_cb {
> > int status;
> > u32 internal:1;
> > u32 blocking:1;
> > + struct mei_ext_hdr *ext_hdr;
> > };
> >
