[Intel-gfx] [PATCH 02/15] mei: add support to GSC extended header

[Winkler, Tomas]

> -----Original Message-----
> From: Teres Alexis, Alan Previn <alan.previn.teres.alexis at intel.com>
> Sent: Thursday, August 04, 2022 01:08
> To: Ceraolo Spurio, Daniele <daniele.ceraolospurio at intel.com>; intel-
> gfx at lists.freedesktop.org
> Cc: Winkler, Tomas <tomas.winkler at intel.com>; Lubart, Vitaly
> <vitaly.lubart at intel.com>
> 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 at intel.com>
> >
> > 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;
> >  };
> >