[char-misc-next 1/2] mei: fix kdoc in the driver

[char-misc-next 1/2] mei: fix kdoc in the driver

[Tomas Winkler]

From: Tamar Mashiah <tamar.mashiah@intel.com>

Over time the functions were renamed,
but this was not always reflected in kdoc, fix that.

Signed-off-by: Tamar Mashiah <tamar.mashiah@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/misc/mei/bus-fixup.c | 2 +-
 drivers/misc/mei/client.c    | 6 +++---
 drivers/misc/mei/hbm.c       | 2 +-
 drivers/misc/mei/hw-me.c     | 4 ++--
 drivers/misc/mei/main.c      | 2 +-
 drivers/misc/mei/pci-txe.c   | 2 +-
 6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/mei/bus-fixup.c b/drivers/misc/mei/bus-fixup.c
index d8e760b11ae3..67844089db21 100644
--- a/drivers/misc/mei/bus-fixup.c
+++ b/drivers/misc/mei/bus-fixup.c
@@ -498,7 +498,7 @@ static struct mei_fixup {
 };
 
 /**
- * mei_cldev_fixup - run fixup handlers
+ * mei_cl_bus_dev_fixup - run fixup handlers
  *
  * @cldev: me client device
  */
diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
index 2cc370adb238..18e49479d8b0 100644
--- a/drivers/misc/mei/client.c
+++ b/drivers/misc/mei/client.c
@@ -326,7 +326,7 @@ void mei_io_cb_free(struct mei_cl_cb *cb)
 }
 
 /**
- * mei_tx_cb_queue - queue tx callback
+ * mei_tx_cb_enqueue - queue tx callback
  *
  * Locking: called under "dev->device_lock" lock
  *
@@ -2250,7 +2250,7 @@ static void mei_cl_dma_free(struct mei_cl *cl)
 }
 
 /**
- * mei_cl_alloc_and_map - send client dma map request
+ * mei_cl_dma_alloc_and_map - send client dma map request
  *
  * @cl: host client
  * @fp: pointer to file structure
@@ -2349,7 +2349,7 @@ int mei_cl_dma_alloc_and_map(struct mei_cl *cl, const struct file *fp,
 }
 
 /**
- * mei_cl_unmap_and_free - send client dma unmap request
+ * mei_cl_dma_unmap - send client dma unmap request
  *
  * @cl: host client
  * @fp: pointer to file structure
diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c
index d0277c7fed10..99b5c1ecc444 100644
--- a/drivers/misc/mei/hbm.c
+++ b/drivers/misc/mei/hbm.c
@@ -853,7 +853,7 @@ static int mei_hbm_add_single_tx_flow_ctrl_creds(struct mei_device *dev,
 }
 
 /**
- * mei_hbm_cl_flow_control_res - flow control response from me
+ * mei_hbm_cl_tx_flow_ctrl_creds_res - flow control response from me
  *
  * @dev: the device structure
  * @fctrl: flow control response bus message
diff --git a/drivers/misc/mei/hw-me.c b/drivers/misc/mei/hw-me.c
index cda0829ac589..d3a6c0728645 100644
--- a/drivers/misc/mei/hw-me.c
+++ b/drivers/misc/mei/hw-me.c
@@ -1380,7 +1380,7 @@ static bool mei_me_fw_type_nm(const struct pci_dev *pdev)
 	.quirk_probe = mei_me_fw_type_nm
 
 /**
- * mei_me_fw_sku_sps_4() - check for sps 4.0 sku
+ * mei_me_fw_type_sps_4() - check for sps 4.0 sku
  *
  * Read ME FW Status register to check for SPS Firmware.
  * The SPS FW is only signaled in the PCI function 0.
@@ -1405,7 +1405,7 @@ static bool mei_me_fw_type_sps_4(const struct pci_dev *pdev)
 	.quirk_probe = mei_me_fw_type_sps_4
 
 /**
- * mei_me_fw_sku_sps() - check for sps sku
+ * mei_me_fw_type_sps() - check for sps sku
  *
  * Read ME FW Status register to check for SPS Firmware.
  * The SPS FW is only signaled in pci function 0
diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c
index 9001c45f6fc4..786f7c8f7f61 100644
--- a/drivers/misc/mei/main.c
+++ b/drivers/misc/mei/main.c
@@ -1102,7 +1102,7 @@ static ssize_t dev_state_show(struct device *device,
 static DEVICE_ATTR_RO(dev_state);
 
 /**
- * dev_set_devstate: set to new device state and notify sysfs file.
+ * mei_set_devstate: set to new device state and notify sysfs file.
  *
  * @dev: mei_device
  * @state: new device state
diff --git a/drivers/misc/mei/pci-txe.c b/drivers/misc/mei/pci-txe.c
index 4bf26ce61044..aec0483b8e72 100644
--- a/drivers/misc/mei/pci-txe.c
+++ b/drivers/misc/mei/pci-txe.c
@@ -156,7 +156,7 @@ static int mei_txe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 }
 
 /**
- * mei_txe_remove - Device Shutdown Routine
+ * mei_txe_shutdown- Device Shutdown Routine
  *
  * @pdev: PCI device structure
  *
-- 
2.31.1

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

[Tomas Winkler]

The mei extension header was build as array of flexible structures
which will not work if actually more headers are added
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;
 
 /**
  * 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;
+
+/**
+ * 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;
 
 /*
  * Extended header iterator functions
@@ -266,7 +278,7 @@ struct mei_ext_meta_hdr {
  */
 static inline struct mei_ext_hdr *mei_ext_begin(struct mei_ext_meta_hdr *meta)
 {
-	return meta->hdrs;
+	return (struct mei_ext_hdr *)meta->hdrs;
 }
 
 /**
@@ -284,7 +296,7 @@ static inline bool mei_ext_last(struct mei_ext_meta_hdr *meta,
 }
 
 /**
- *mei_ext_next - following extended header on the TLV list
+ * mei_ext_next - following extended header on the TLV list
  *
  * @ext: current extend header
  *
@@ -295,7 +307,7 @@ static inline bool mei_ext_last(struct mei_ext_meta_hdr *meta,
  */
 static inline struct mei_ext_hdr *mei_ext_next(struct mei_ext_hdr *ext)
 {
-	return (struct mei_ext_hdr *)(ext->hdr + (ext->length * 4));
+	return (struct mei_ext_hdr *)((u8 *)ext + (ext->length * 4));
 }
 
 /**
diff --git a/drivers/misc/mei/interrupt.c b/drivers/misc/mei/interrupt.c
index aab3ebfa9fc4..a67f4f2d33a9 100644
--- a/drivers/misc/mei/interrupt.c
+++ b/drivers/misc/mei/interrupt.c
@@ -123,13 +123,13 @@ 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 = NULL;
+		struct mei_ext_hdr_vtag *vtag_hdr = NULL;
 
 		ext = mei_ext_begin(meta);
 		do {
 			switch (ext->type) {
 			case MEI_EXT_HDR_VTAG:
-				vtag = ext;
+				vtag_hdr = (struct mei_ext_hdr_vtag *)ext;
 				break;
 			case MEI_EXT_HDR_NONE:
 				fallthrough;
@@ -141,20 +141,20 @@ static int mei_cl_irq_read_msg(struct mei_cl *cl,
 			ext = mei_ext_next(ext);
 		} while (!mei_ext_last(meta, ext));
 
-		if (!vtag) {
+		if (!vtag_hdr) {
 			cl_dbg(dev, cl, "vtag not found in extended header.\n");
 			cb->status = -EPROTO;
 			goto discard;
 		}
 
-		cl_dbg(dev, cl, "vtag: %d\n", vtag->ext_payload[0]);
-		if (cb->vtag && cb->vtag != vtag->ext_payload[0]) {
+		cl_dbg(dev, cl, "vtag: %d\n", vtag_hdr->vtag);
+		if (cb->vtag && cb->vtag != vtag_hdr->vtag) {
 			cl_err(dev, cl, "mismatched tag: %d != %d\n",
-			       cb->vtag, vtag->ext_payload[0]);
+			       cb->vtag, vtag_hdr->vtag);
 			cb->status = -EPROTO;
 			goto discard;
 		}
-		cb->vtag = vtag->ext_payload[0];
+		cb->vtag = vtag_hdr->vtag;
 	}
 
 	if (!mei_cl_is_connected(cl)) {
@@ -331,7 +331,6 @@ int mei_irq_read_handler(struct mei_device *dev,
 	struct mei_ext_meta_hdr *meta_hdr = NULL;
 	struct mei_cl *cl;
 	int ret;
-	u32 ext_meta_hdr_u32;
 	u32 hdr_size_left;
 	u32 hdr_size_ext;
 	int i;
@@ -367,14 +366,12 @@ int mei_irq_read_handler(struct mei_device *dev,
 
 	if (mei_hdr->extended) {
 		if (!dev->rd_msg_hdr[1]) {
-			ext_meta_hdr_u32 = mei_read_hdr(dev);
-			dev->rd_msg_hdr[1] = ext_meta_hdr_u32;
+			dev->rd_msg_hdr[1] = mei_read_hdr(dev);
 			dev->rd_msg_hdr_count++;
 			(*slots)--;
-			dev_dbg(dev->dev, "extended header is %08x\n",
-				ext_meta_hdr_u32);
+			dev_dbg(dev->dev, "extended header is %08x\n", dev->rd_msg_hdr[1]);
 		}
-		meta_hdr = ((struct mei_ext_meta_hdr *)dev->rd_msg_hdr + 1);
+		meta_hdr = ((struct mei_ext_meta_hdr *)&dev->rd_msg_hdr[1]);
 		if (check_add_overflow((u32)sizeof(*meta_hdr),
 				       mei_slots2data(meta_hdr->size),
 				       &hdr_size_ext)) {
-- 
2.31.1

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

[Greg Kroah-Hartman]

On Wed, Jun 16, 2021 at 12:15:57AM +0300, Tomas Winkler wrote:
> @@ -284,7 +296,7 @@ static inline bool mei_ext_last(struct mei_ext_meta_hdr *meta,
>  }
>  
>  /**
> - *mei_ext_next - following extended header on the TLV list
> + * mei_ext_next - following extended header on the TLV list
>   *
>   * @ext: current extend header
>   *

This change belongs in the previous patch, not this one.

Please fix up.

thanks,

greg k-h

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

[Greg Kroah-Hartman]

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?

And did you forget a '.' here?

> 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?

>  
>  /**
>   * 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?

> +
> +/**
> + * 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?

thanks,

greg k-h

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

[Winkler, Tomas]

> 
> 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