RE: [char-misc-next 2/2] mei: revamp mei extension header structure layout.

["Winkler, Tomas" <tomas.winkler@intel.com>]

> 
> On Wed, Jun 16, 2021 at 12:15:57AM +0300, Tomas Winkler wrote:
> > The mei extension header was build as array of flexible structures
> > which will not work if actually more headers are added
> 
> Why not?  What is wrong with what you currently have?
Because it is not possible to create array of flexible structures in C as far as I know. 

> And did you forget a '.' here?
Thanks will resend. 

> 
> > Use basic type u8 for the variable sized extension.
> > Define explicitly mei_ext_hdr_vtag structure.
> > Fix also mei_ext_next() function to point correctly to the end of the
> > header.
> >
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> >  drivers/misc/mei/client.c    | 16 +++++++++-------
> >  drivers/misc/mei/hw.h        | 28 ++++++++++++++++++++--------
> >  drivers/misc/mei/interrupt.c | 23 ++++++++++-------------
> >  3 files changed, 39 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
> > index 18e49479d8b0..96f4e59c32a5 100644
> > --- a/drivers/misc/mei/client.c
> > +++ b/drivers/misc/mei/client.c
> > @@ -1726,12 +1726,15 @@ int mei_cl_read_start(struct mei_cl *cl, size_t
> length, const struct file *fp)
> >  	return rets;
> >  }
> >
> > -static inline u8 mei_ext_hdr_set_vtag(struct mei_ext_hdr *ext, u8
> > vtag)
> > +static inline u8 mei_ext_hdr_set_vtag(void *ext, u8 vtag)
> >  {
> > -	ext->type = MEI_EXT_HDR_VTAG;
> > -	ext->ext_payload[0] = vtag;
> > -	ext->length = mei_data2slots(sizeof(*ext));
> > -	return ext->length;
> > +	struct mei_ext_hdr_vtag *vtag_hdr = ext;
> > +
> > +	vtag_hdr->hdr.type = MEI_EXT_HDR_VTAG;
> > +	vtag_hdr->hdr.length = mei_data2slots(sizeof(*vtag_hdr));
> > +	vtag_hdr->vtag = vtag;
> > +	vtag_hdr->reserved = 0;
> > +	return vtag_hdr->hdr.length;
> >  }
> >
> >  /**
> > @@ -1745,7 +1748,6 @@ static struct mei_msg_hdr
> > *mei_msg_hdr_init(const struct mei_cl_cb *cb)  {
> >  	size_t hdr_len;
> >  	struct mei_ext_meta_hdr *meta;
> > -	struct mei_ext_hdr *ext;
> >  	struct mei_msg_hdr *mei_hdr;
> >  	bool is_ext, is_vtag;
> >
> > @@ -1764,7 +1766,7 @@ static struct mei_msg_hdr
> > *mei_msg_hdr_init(const struct mei_cl_cb *cb)
> >
> >  	hdr_len += sizeof(*meta);
> >  	if (is_vtag)
> > -		hdr_len += sizeof(*ext);
> > +		hdr_len += sizeof(struct mei_ext_hdr_vtag);
> >
> >  setup_hdr:
> >  	mei_hdr = kzalloc(hdr_len, GFP_KERNEL); diff --git
> > a/drivers/misc/mei/hw.h b/drivers/misc/mei/hw.h index
> > b10606550613..dfd60c916da0 100644
> > --- a/drivers/misc/mei/hw.h
> > +++ b/drivers/misc/mei/hw.h
> > @@ -235,9 +235,8 @@ enum mei_ext_hdr_type {  struct mei_ext_hdr {
> >  	u8 type;
> >  	u8 length;
> > -	u8 ext_payload[2];
> > -	u8 hdr[];
> > -};
> > +	u8 data[];
> > +} __packed;
> 
> why packed?
It's an aligned structure but still It's HW interface. 
> 
> >
> >  /**
> >   * struct mei_ext_meta_hdr - extend header meta data @@ -250,8
> > +249,21 @@ struct mei_ext_meta_hdr {
> >  	u8 count;
> >  	u8 size;
> >  	u8 reserved[2];
> > -	struct mei_ext_hdr hdrs[];
> > -};
> > +	u8 hdrs[];
> > +} __packed;
> 
> Why packed?
Same here. 

> 
> > +
> > +/**
> > + * struct mei_ext_hdr_vtag - extend header for vtag
> > + *
> > + * @hdr: standard extend header
> > + * @vtag: virtual tag
> > + * @reserved: reserved
> > + */
> > +struct mei_ext_hdr_vtag {
> > +	struct mei_ext_hdr hdr;
> > +	u8 vtag;
> > +	u8 reserved;
> > +} __packed;
> 
> Why packed?
> 
> These are not being read directly from hardware are they?
They are.

Thanks
Tomas