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

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

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.


...alan


On Thu, 2022-06-09 at 16:19 -0700, Ceraolo Spurio, Daniele wrote:
> From: Tomas Winkler <tomas.winkler@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.

> +		}
> +
> +		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;
>  };
>