[PATCH 1/2] HID: intel-ish-hid: fix cache management mistake

[PATCH 1/2] HID: intel-ish-hid: fix cache management mistake

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The low-level cache operation on a coherent buffer is incorrect,
as coherent DMA memory may not actually be cached. Instead, use a DMA
barrier that ensures that the data is visible to the DMA master before
the address is and move the memcpy() before the reference.

Fixes: 579a267e4617 ("HID: intel-ish-hid: Implement loading firmware from host feature")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
I noticed this one while looking at the bug that was fixed in
236049723826 ("HID: intel-ish-hid: Fix build error for COMPILE_TEST")
---
 drivers/hid/intel-ish-hid/ishtp/loader.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ishtp/loader.c b/drivers/hid/intel-ish-hid/ishtp/loader.c
index 2785b04a2f5a..062d1b25eaa7 100644
--- a/drivers/hid/intel-ish-hid/ishtp/loader.c
+++ b/drivers/hid/intel-ish-hid/ishtp/loader.c
@@ -33,7 +33,6 @@
 
 #define dev_fmt(fmt) "ISH loader: " fmt
 
-#include <linux/cacheflush.h>
 #include <linux/container_of.h>
 #include <linux/dev_printk.h>
 #include <linux/dma-mapping.h>
@@ -175,10 +174,11 @@ static int prepare_dma_bufs(struct ishtp_device *dev,
 			return -ENOMEM;
 
 		fragment->fragment_tbl[i].ddr_adrs = cpu_to_le64(dma_addr);
+
+		memcpy(dma_bufs[i], ish_fw->data + offset, fragment->fragment_tbl[i].length);
+		dma_wmb();
 		fragment->fragment_tbl[i].length = clamp(ish_fw->size - offset, 0, fragment_size);
 		fragment->fragment_tbl[i].fw_off = offset;
-		memcpy(dma_bufs[i], ish_fw->data + offset, fragment->fragment_tbl[i].length);
-		clflush_cache_range(dma_bufs[i], fragment_size);
 
 		offset += fragment->fragment_tbl[i].length;
 	}
-- 
2.39.2

[PATCH 2/2] HID: intel-ish-hid: fix endian-conversion

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The newly added file causes a ton of sparse warnings about the
incorrect use of __le32 and similar types:

drivers/hid/intel-ish-hid/ishtp/loader.c:179:17: warning: cast from restricted __le32
drivers/hid/intel-ish-hid/ishtp/loader.c:182:50: warning: incorrect type in assignment (different base types)
drivers/hid/intel-ish-hid/ishtp/loader.c:182:50:    expected restricted __le32 [usertype] length
drivers/hid/intel-ish-hid/ishtp/loader.c:182:50:    got restricted __le16 [usertype]
drivers/hid/intel-ish-hid/ishtp/loader.c:183:50: warning: incorrect type in assignment (different base types)
drivers/hid/intel-ish-hid/ishtp/loader.c:183:50:    expected restricted __le32 [usertype] fw_off
drivers/hid/intel-ish-hid/ishtp/loader.c:183:50:    got restricted __le16 [usertype]

Add the necessary conversions and use temporary variables where appropriate
to avoid converting back.

Fixes: 579a267e4617 ("HID: intel-ish-hid: Implement loading firmware from host feature")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
I noticed this one while looking at the bug that was fixed in
236049723826 ("HID: intel-ish-hid: Fix build error for COMPILE_TEST")
---
 drivers/hid/intel-ish-hid/ishtp/loader.c | 69 +++++++++++++-----------
 drivers/hid/intel-ish-hid/ishtp/loader.h | 33 +++++++-----
 2 files changed, 58 insertions(+), 44 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ishtp/loader.c b/drivers/hid/intel-ish-hid/ishtp/loader.c
index 062d1b25eaa7..1d4cb99d2130 100644
--- a/drivers/hid/intel-ish-hid/ishtp/loader.c
+++ b/drivers/hid/intel-ish-hid/ishtp/loader.c
@@ -83,8 +83,8 @@ static int loader_write_message(struct ishtp_device *dev, void *buf, int len)
 static int loader_xfer_cmd(struct ishtp_device *dev, void *req, int req_len,
 			   void *resp, int resp_len)
 {
-	struct loader_msg_header *req_hdr = req;
-	struct loader_msg_header *resp_hdr = resp;
+	union loader_msg_header req_hdr;
+	union loader_msg_header resp_hdr;
 	struct device *devc = dev->devc;
 	int rv;
 
@@ -92,34 +92,37 @@ static int loader_xfer_cmd(struct ishtp_device *dev, void *req, int req_len,
 	dev->fw_loader_rx_size = resp_len;
 
 	rv = loader_write_message(dev, req, req_len);
+	req_hdr.val32 = le32_to_cpup(req);
+
 	if (rv < 0) {
-		dev_err(devc, "write cmd %u failed:%d\n", req_hdr->command, rv);
+		dev_err(devc, "write cmd %u failed:%d\n", req_hdr.command, rv);
 		return rv;
 	}
 
 	/* Wait the ACK */
 	wait_event_interruptible_timeout(dev->wait_loader_recvd_msg, dev->fw_loader_received,
 					 ISHTP_LOADER_TIMEOUT);
+	resp_hdr.val32 = le32_to_cpup(resp);
 	dev->fw_loader_rx_size = 0;
 	dev->fw_loader_rx_buf = NULL;
 	if (!dev->fw_loader_received) {
-		dev_err(devc, "wait response of cmd %u timeout\n", req_hdr->command);
+		dev_err(devc, "wait response of cmd %u timeout\n", req_hdr.command);
 		return -ETIMEDOUT;
 	}
 
-	if (!resp_hdr->is_response) {
-		dev_err(devc, "not a response for %u\n", req_hdr->command);
+	if (!resp_hdr.is_response) {
+		dev_err(devc, "not a response for %u\n", req_hdr.command);
 		return -EBADMSG;
 	}
 
-	if (req_hdr->command != resp_hdr->command) {
-		dev_err(devc, "unexpected cmd response %u:%u\n", req_hdr->command,
-			resp_hdr->command);
+	if (req_hdr.command != resp_hdr.command) {
+		dev_err(devc, "unexpected cmd response %u:%u\n", req_hdr.command,
+			resp_hdr.command);
 		return -EBADMSG;
 	}
 
-	if (resp_hdr->status) {
-		dev_err(devc, "cmd %u failed %u\n", req_hdr->command, resp_hdr->status);
+	if (resp_hdr.status) {
+		dev_err(devc, "cmd %u failed %u\n", req_hdr.command, resp_hdr.status);
 		return -EIO;
 	}
 
@@ -162,25 +165,30 @@ static void release_dma_bufs(struct ishtp_device *dev,
 static int prepare_dma_bufs(struct ishtp_device *dev,
 			    const struct firmware *ish_fw,
 			    struct loader_xfer_dma_fragment *fragment,
-			    void **dma_bufs, u32 fragment_size)
+			    void **dma_bufs, u32 fragment_size, u32 fragment_count)
 {
 	dma_addr_t dma_addr;
 	u32 offset = 0;
+	u32 length;
 	int i;
 
 	for (i = 0; i < fragment->fragment_cnt && offset < ish_fw->size; i++) {
 		dma_bufs[i] = dma_alloc_coherent(dev->devc, fragment_size, &dma_addr, GFP_KERNEL);
+		dma_bufs[i] = dma_alloc_coherent(dev->devc, fragment_size,
+						 &dma, GFP_KERNEL);
 		if (!dma_bufs[i])
 			return -ENOMEM;
 
 		fragment->fragment_tbl[i].ddr_adrs = cpu_to_le64(dma_addr);
 
-		memcpy(dma_bufs[i], ish_fw->data + offset, fragment->fragment_tbl[i].length);
+		memcpy(dma_bufs[i], ish_fw->data + offset, le32_to_cpu(fragment->fragment_tbl[i].length));
 		dma_wmb();
-		fragment->fragment_tbl[i].length = clamp(ish_fw->size - offset, 0, fragment_size);
-		fragment->fragment_tbl[i].fw_off = offset;
+		fragment->fragment_tbl[i].ddr_adrs = cpu_to_le64(dma);
+		length = clamp(ish_fw->size - offset, 0, fragment_size);
+		fragment->fragment_tbl[i].length = cpu_to_le32(length);
+		fragment->fragment_tbl[i].fw_off = cpu_to_le32(offset);
 
-		offset += fragment->fragment_tbl[i].length;
+		offset += length;
 	}
 
 	return 0;
@@ -208,17 +216,17 @@ void ishtp_loader_work(struct work_struct *work)
 {
 	DEFINE_RAW_FLEX(struct loader_xfer_dma_fragment, fragment, fragment_tbl, FRAGMENT_MAX_NUM);
 	struct ishtp_device *dev = container_of(work, struct ishtp_device, work_fw_loader);
-	struct loader_xfer_query query = {
-		.header.command = LOADER_CMD_XFER_QUERY,
-	};
-	struct loader_start start = {
-		.header.command = LOADER_CMD_START,
-	};
+	union loader_msg_header query_hdr = { .command = LOADER_CMD_XFER_QUERY, };
+	union loader_msg_header start_hdr = { .command = LOADER_CMD_START, };
+	union loader_msg_header fragment_hdr = { .command = LOADER_CMD_XFER_FRAGMENT, };
+	struct loader_xfer_query query = { .header = cpu_to_le32(query_hdr.val32), };
+	struct loader_start start = { .header = cpu_to_le32(start_hdr.val32), };
 	union loader_recv_message recv_msg;
 	char *filename = dev->driver_data->fw_filename;
 	const struct firmware *ish_fw;
 	void *dma_bufs[FRAGMENT_MAX_NUM] = {};
 	u32 fragment_size;
+	u32 fragment_count;
 	int retry = ISHTP_LOADER_RETRY_TIMES;
 	int rv;
 
@@ -228,23 +236,24 @@ void ishtp_loader_work(struct work_struct *work)
 		return;
 	}
 
-	fragment->fragment.header.command = LOADER_CMD_XFER_FRAGMENT;
-	fragment->fragment.xfer_mode = LOADER_XFER_MODE_DMA;
-	fragment->fragment.is_last = 1;
-	fragment->fragment.size = ish_fw->size;
+	fragment->fragment.header = cpu_to_le32(fragment_hdr.val32);;
+	fragment->fragment.xfer_mode = cpu_to_le32(LOADER_XFER_MODE_DMA);
+	fragment->fragment.is_last = cpu_to_le32(1);
+	fragment->fragment.size = cpu_to_le32(ish_fw->size);
 	/* Calculate the size of a single DMA fragment */
 	fragment_size = PFN_ALIGN(DIV_ROUND_UP(ish_fw->size, FRAGMENT_MAX_NUM));
 	/* Calculate the count of DMA fragments */
-	fragment->fragment_cnt = DIV_ROUND_UP(ish_fw->size, fragment_size);
+	fragment_count = DIV_ROUND_UP(ish_fw->size, fragment_size);
+	fragment->fragment_cnt = cpu_to_le32(fragment_count);
 
-	rv = prepare_dma_bufs(dev, ish_fw, fragment, dma_bufs, fragment_size);
+	rv = prepare_dma_bufs(dev, ish_fw, fragment, dma_bufs, fragment_size, fragment_count);
 	if (rv) {
 		dev_err(dev->devc, "prepare DMA buffer failed.\n");
 		goto out;
 	}
 
 	do {
-		query.image_size = ish_fw->size;
+		query.image_size = cpu_to_le32(ish_fw->size);
 		rv = loader_xfer_cmd(dev, &query, sizeof(query), recv_msg.raw_data,
 				     sizeof(struct loader_xfer_query_ack));
 		if (rv)
@@ -257,7 +266,7 @@ void ishtp_loader_work(struct work_struct *work)
 			recv_msg.query_ack.version_build);
 
 		rv = loader_xfer_cmd(dev, fragment,
-				     struct_size(fragment, fragment_tbl, fragment->fragment_cnt),
+				     struct_size(fragment, fragment_tbl, fragment_count),
 				     recv_msg.raw_data, sizeof(struct loader_xfer_fragment_ack));
 		if (rv)
 			continue; /* try again if failed */
diff --git a/drivers/hid/intel-ish-hid/ishtp/loader.h b/drivers/hid/intel-ish-hid/ishtp/loader.h
index 7aa45ebc3f7b..308b96085a4d 100644
--- a/drivers/hid/intel-ish-hid/ishtp/loader.h
+++ b/drivers/hid/intel-ish-hid/ishtp/loader.h
@@ -30,19 +30,23 @@ struct work_struct;
 #define LOADER_XFER_MODE_DMA BIT(0)
 
 /**
- * struct loader_msg_header - ISHTP firmware loader message header
+ * union loader_msg_header - ISHTP firmware loader message header
  * @command: Command type
  * @is_response: Indicates if the message is a response
  * @has_next: Indicates if there is a next message
  * @reserved: Reserved for future use
  * @status: Status of the message
- */
-struct loader_msg_header {
-	__le32 command:7;
-	__le32 is_response:1;
-	__le32 has_next:1;
-	__le32 reserved:15;
-	__le32 status:8;
+ * @val32: entire header as a 32-bit value
+ */
+union loader_msg_header {
+	struct {
+		__u32 command:7;
+		__u32 is_response:1;
+		__u32 has_next:1;
+		__u32 reserved:15;
+		__u32 status:8;
+	};
+	__u32 val32;
 };
 
 /**
@@ -51,7 +55,7 @@ struct loader_msg_header {
  * @image_size: Size of the image
  */
 struct loader_xfer_query {
-	struct loader_msg_header header;
+	__le32 header;
 	__le32 image_size;
 };
 
@@ -103,7 +107,7 @@ struct loader_capability {
  * @capability: Loader capability
  */
 struct loader_xfer_query_ack {
-	struct loader_msg_header header;
+	__le32 header;
 	__le16 version_major;
 	__le16 version_minor;
 	__le16 version_hotfix;
@@ -122,7 +126,7 @@ struct loader_xfer_query_ack {
  * @is_last: Is last
  */
 struct loader_xfer_fragment {
-	struct loader_msg_header header;
+	__le32 header;
 	__le32 xfer_mode;
 	__le32 offset;
 	__le32 size;
@@ -134,7 +138,7 @@ struct loader_xfer_fragment {
  * @header: Header of the message
  */
 struct loader_xfer_fragment_ack {
-	struct loader_msg_header header;
+	__le32 header;
 };
 
 /**
@@ -170,7 +174,7 @@ struct loader_xfer_dma_fragment {
  * @header: Header of the message
  */
 struct loader_start {
-	struct loader_msg_header header;
+	__le32 header;
 };
 
 /**
@@ -178,10 +182,11 @@ struct loader_start {
  * @header: Header of the message
  */
 struct loader_start_ack {
-	struct loader_msg_header header;
+	__le32 header;
 };
 
 union loader_recv_message {
+	__le32 header;
 	struct loader_xfer_query_ack query_ack;
 	struct loader_xfer_fragment_ack fragment_ack;
 	struct loader_start_ack start_ack;
-- 
2.39.2

Re: [PATCH 2/2] HID: intel-ish-hid: fix endian-conversion

[Arnd Bergmann]

On Tue, May 28, 2024, at 13:57, Arnd Bergmann wrote:

> 
>  	for (i = 0; i < fragment->fragment_cnt && offset < ish_fw->size; i++) 
> {
>  		dma_bufs[i] = dma_alloc_coherent(dev->devc, fragment_size, 
> &dma_addr, GFP_KERNEL);
> +		dma_bufs[i] = dma_alloc_coherent(dev->devc, fragment_size,
> +						 &dma, GFP_KERNEL);
>  		if (!dma_bufs[i])
>  			return -ENOMEM;
> 


Sorry about this one, the duplicate linen was an incorrect
rebase. I've fixed this one locally but did not resend the
series since I'm still waiting for other review comments.

    Arnd

Re: [PATCH 2/2] HID: intel-ish-hid: fix endian-conversion

[kernel test robot]

Hi Arnd,

kernel test robot noticed the following build errors:

[auto build test ERROR on hid/for-next]
[also build test ERROR on next-20240528]
[cannot apply to linus/master v6.10-rc1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Arnd-Bergmann/HID-intel-ish-hid-fix-endian-conversion/20240528-200100
base:   https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
patch link:    https://lore.kernel.org/r/20240528115802.3122955-2-arnd%40kernel.org
patch subject: [PATCH 2/2] HID: intel-ish-hid: fix endian-conversion
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20240529/202405290420.DtwUdg3b-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240529/202405290420.DtwUdg3b-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405290420.DtwUdg3b-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/hid/intel-ish-hid/ishtp/loader.c: In function 'prepare_dma_bufs':
>> drivers/hid/intel-ish-hid/ishtp/loader.c:178:51: error: 'dma' undeclared (first use in this function); did you mean 'cma'?
     178 |                                                  &dma, GFP_KERNEL);
         |                                                   ^~~
         |                                                   cma
   drivers/hid/intel-ish-hid/ishtp/loader.c:178:51: note: each undeclared identifier is reported only once for each function it appears in


vim +178 drivers/hid/intel-ish-hid/ishtp/loader.c

   154	
   155	/**
   156	 * prepare_dma_bufs() - Prepare the DMA buffer for transferring firmware fragments
   157	 * @dev: The ISHTP device
   158	 * @ish_fw: The ISH firmware
   159	 * @fragment: The ISHTP firmware fragment descriptor
   160	 * @dma_bufs: The array of DMA fragment buffers
   161	 * @fragment_size: The size of a single DMA fragment
   162	 *
   163	 * Return: 0 on success, negative error code on failure
   164	 */
   165	static int prepare_dma_bufs(struct ishtp_device *dev,
   166				    const struct firmware *ish_fw,
   167				    struct loader_xfer_dma_fragment *fragment,
   168				    void **dma_bufs, u32 fragment_size, u32 fragment_count)
   169	{
   170		dma_addr_t dma_addr;
   171		u32 offset = 0;
   172		u32 length;
   173		int i;
   174	
   175		for (i = 0; i < fragment->fragment_cnt && offset < ish_fw->size; i++) {
   176			dma_bufs[i] = dma_alloc_coherent(dev->devc, fragment_size, &dma_addr, GFP_KERNEL);
   177			dma_bufs[i] = dma_alloc_coherent(dev->devc, fragment_size,
 > 178							 &dma, GFP_KERNEL);
   179			if (!dma_bufs[i])
   180				return -ENOMEM;
   181	
   182			fragment->fragment_tbl[i].ddr_adrs = cpu_to_le64(dma_addr);
   183	
   184			memcpy(dma_bufs[i], ish_fw->data + offset, le32_to_cpu(fragment->fragment_tbl[i].length));
   185			dma_wmb();
   186			fragment->fragment_tbl[i].ddr_adrs = cpu_to_le64(dma);
   187			length = clamp(ish_fw->size - offset, 0, fragment_size);
   188			fragment->fragment_tbl[i].length = cpu_to_le32(length);
   189			fragment->fragment_tbl[i].fw_off = cpu_to_le32(offset);
   190	
   191			offset += length;
   192		}
   193	
   194		return 0;
   195	}
   196	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] HID: intel-ish-hid: fix endian-conversion

[kernel test robot]

Hi Arnd,

kernel test robot noticed the following build errors:

[auto build test ERROR on hid/for-next]
[also build test ERROR on next-20240528]
[cannot apply to linus/master v6.10-rc1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Arnd-Bergmann/HID-intel-ish-hid-fix-endian-conversion/20240528-200100
base:   https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
patch link:    https://lore.kernel.org/r/20240528115802.3122955-2-arnd%40kernel.org
patch subject: [PATCH 2/2] HID: intel-ish-hid: fix endian-conversion
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20240529/202405290447.n14W21hZ-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240529/202405290447.n14W21hZ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405290447.n14W21hZ-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/hid/intel-ish-hid/ishtp/loader.c:178:9: error: use of undeclared identifier 'dma'
     178 |                                                  &dma, GFP_KERNEL);
         |                                                   ^
   drivers/hid/intel-ish-hid/ishtp/loader.c:186:52: error: use of undeclared identifier 'dma'
     186 |                 fragment->fragment_tbl[i].ddr_adrs = cpu_to_le64(dma);
         |                                                                  ^
   include/linux/byteorder/generic.h:86:21: note: expanded from macro 'cpu_to_le64'
      86 | #define cpu_to_le64 __cpu_to_le64
         |                     ^
   2 errors generated.


vim +/dma +178 drivers/hid/intel-ish-hid/ishtp/loader.c

   154	
   155	/**
   156	 * prepare_dma_bufs() - Prepare the DMA buffer for transferring firmware fragments
   157	 * @dev: The ISHTP device
   158	 * @ish_fw: The ISH firmware
   159	 * @fragment: The ISHTP firmware fragment descriptor
   160	 * @dma_bufs: The array of DMA fragment buffers
   161	 * @fragment_size: The size of a single DMA fragment
   162	 *
   163	 * Return: 0 on success, negative error code on failure
   164	 */
   165	static int prepare_dma_bufs(struct ishtp_device *dev,
   166				    const struct firmware *ish_fw,
   167				    struct loader_xfer_dma_fragment *fragment,
   168				    void **dma_bufs, u32 fragment_size, u32 fragment_count)
   169	{
   170		dma_addr_t dma_addr;
   171		u32 offset = 0;
   172		u32 length;
   173		int i;
   174	
   175		for (i = 0; i < fragment->fragment_cnt && offset < ish_fw->size; i++) {
   176			dma_bufs[i] = dma_alloc_coherent(dev->devc, fragment_size, &dma_addr, GFP_KERNEL);
   177			dma_bufs[i] = dma_alloc_coherent(dev->devc, fragment_size,
 > 178							 &dma, GFP_KERNEL);
   179			if (!dma_bufs[i])
   180				return -ENOMEM;
   181	
   182			fragment->fragment_tbl[i].ddr_adrs = cpu_to_le64(dma_addr);
   183	
   184			memcpy(dma_bufs[i], ish_fw->data + offset, le32_to_cpu(fragment->fragment_tbl[i].length));
   185			dma_wmb();
   186			fragment->fragment_tbl[i].ddr_adrs = cpu_to_le64(dma);
   187			length = clamp(ish_fw->size - offset, 0, fragment_size);
   188			fragment->fragment_tbl[i].length = cpu_to_le32(length);
   189			fragment->fragment_tbl[i].fw_off = cpu_to_le32(offset);
   190	
   191			offset += length;
   192		}
   193	
   194		return 0;
   195	}
   196	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

RE: [PATCH 2/2] HID: intel-ish-hid: fix endian-conversion

[Zhang, Lixu]

>-----Original Message-----
>From: Arnd Bergmann <arnd@kernel.org>
>Sent: Tuesday, May 28, 2024 7:58 PM
>To: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>; Jiri Kosina
><jikos@kernel.org>; Benjamin Tissoires <bentiss@kernel.org>; Zhang, Lixu
><lixu.zhang@intel.com>
>Cc: Arnd Bergmann <arnd@arndb.de>; linux-input@vger.kernel.org; linux-
>kernel@vger.kernel.org
>Subject: [PATCH 2/2] HID: intel-ish-hid: fix endian-conversion
>
>From: Arnd Bergmann <arnd@arndb.de>
>
>The newly added file causes a ton of sparse warnings about the incorrect use of
>__le32 and similar types:
>
>drivers/hid/intel-ish-hid/ishtp/loader.c:179:17: warning: cast from restricted
>__le32
>drivers/hid/intel-ish-hid/ishtp/loader.c:182:50: warning: incorrect type in
>assignment (different base types)
>drivers/hid/intel-ish-hid/ishtp/loader.c:182:50:    expected restricted __le32
>[usertype] length
>drivers/hid/intel-ish-hid/ishtp/loader.c:182:50:    got restricted __le16
>[usertype]
>drivers/hid/intel-ish-hid/ishtp/loader.c:183:50: warning: incorrect type in
>assignment (different base types)
>drivers/hid/intel-ish-hid/ishtp/loader.c:183:50:    expected restricted __le32
>[usertype] fw_off
>drivers/hid/intel-ish-hid/ishtp/loader.c:183:50:    got restricted __le16
>[usertype]
>
>Add the necessary conversions and use temporary variables where appropriate
>to avoid converting back.
>
>Fixes: 579a267e4617 ("HID: intel-ish-hid: Implement loading firmware from
>host feature")
>Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>---
>I noticed this one while looking at the bug that was fixed in
>236049723826 ("HID: intel-ish-hid: Fix build error for COMPILE_TEST")
>---
> drivers/hid/intel-ish-hid/ishtp/loader.c | 69 +++++++++++++-----------
>drivers/hid/intel-ish-hid/ishtp/loader.h | 33 +++++++-----
> 2 files changed, 58 insertions(+), 44 deletions(-)
>
>diff --git a/drivers/hid/intel-ish-hid/ishtp/loader.c b/drivers/hid/intel-ish-
>hid/ishtp/loader.c
>index 062d1b25eaa7..1d4cb99d2130 100644
>--- a/drivers/hid/intel-ish-hid/ishtp/loader.c
>+++ b/drivers/hid/intel-ish-hid/ishtp/loader.c
...

>@@ -162,25 +165,30 @@ static void release_dma_bufs(struct ishtp_device
>*dev,  static int prepare_dma_bufs(struct ishtp_device *dev,
> 			    const struct firmware *ish_fw,
> 			    struct loader_xfer_dma_fragment *fragment,
>-			    void **dma_bufs, u32 fragment_size)
>+			    void **dma_bufs, u32 fragment_size, u32
>fragment_count)
See below.

> {
> 	dma_addr_t dma_addr;
> 	u32 offset = 0;
>+	u32 length;
> 	int i;
>
> 	for (i = 0; i < fragment->fragment_cnt && offset < ish_fw->size; i++) {
You added a parameter fragment_count, but you didn't use it. Did you intend to use it here?

Thanks,
Lixu
> 		dma_bufs[i] = dma_alloc_coherent(dev->devc, fragment_size,
>&dma_addr, GFP_KERNEL);
>+		dma_bufs[i] = dma_alloc_coherent(dev->devc, fragment_size,
>+						 &dma, GFP_KERNEL);
> 		if (!dma_bufs[i])
> 			return -ENOMEM;

Re: [PATCH 2/2] HID: intel-ish-hid: fix endian-conversion

[Arnd Bergmann]

On Wed, May 29, 2024, at 09:05, Zhang, Lixu wrote:

>>
>> 	for (i = 0; i < fragment->fragment_cnt && offset < ish_fw->size; i++) {
> You added a parameter fragment_count, but you didn't use it. Did you 
> intend to use it here?
>

My mistake, that was again broken in my incorrect
rebase.

     Arnd

Re: [PATCH 2/2] HID: intel-ish-hid: fix endian-conversion

[srinivas pandruvada]

On Wed, 2024-05-29 at 15:18 +0200, Arnd Bergmann wrote:
> On Wed, May 29, 2024, at 09:05, Zhang, Lixu wrote:
> 
> > > 
> > > 	for (i = 0; i < fragment->fragment_cnt && offset <
> > > ish_fw->size; i++) {
> > You added a parameter fragment_count, but you didn't use it. Did
> > you 
> > intend to use it here?
> > 
> 
> My mistake, that was again broken in my incorrect
> rebase.
> 
Do you have updated patch? Lixu can try and make sure that the
functionality is not broken by changes.

Thanks,
Srinivas

>      Arnd

Re: [PATCH 2/2] HID: intel-ish-hid: fix endian-conversion

[Arnd Bergmann]

On Fri, May 31, 2024, at 05:49, srinivas pandruvada wrote:
> On Wed, 2024-05-29 at 15:18 +0200, Arnd Bergmann wrote:
>> On Wed, May 29, 2024, at 09:05, Zhang, Lixu wrote:
>> 
>> > > 
>> > > 	for (i = 0; i < fragment->fragment_cnt && offset <
>> > > ish_fw->size; i++) {
>> > You added a parameter fragment_count, but you didn't use it. Did
>> > you 
>> > intend to use it here?
>> > 
>> 
>> My mistake, that was again broken in my incorrect
>> rebase.
>> 
> Do you have updated patch? Lixu can try and make sure that the
> functionality is not broken by changes.
>

I've sent v2 now, hopefully addressing all the issues
of the first version. I've dropped the other patch now,
though I still think something is wrong there.

    Arnd

RE: [PATCH 1/2] HID: intel-ish-hid: fix cache management mistake

[Zhang, Lixu]

>-----Original Message-----
>From: Arnd Bergmann <arnd@kernel.org>
>Sent: Tuesday, May 28, 2024 7:58 PM
>To: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>; Jiri Kosina
><jikos@kernel.org>; Benjamin Tissoires <bentiss@kernel.org>; Zhang, Lixu
><lixu.zhang@intel.com>
>Cc: Arnd Bergmann <arnd@arndb.de>; linux-input@vger.kernel.org; linux-
>kernel@vger.kernel.org
>Subject: [PATCH 1/2] HID: intel-ish-hid: fix cache management mistake
>
>From: Arnd Bergmann <arnd@arndb.de>
>
>The low-level cache operation on a coherent buffer is incorrect, as coherent
>DMA memory may not actually be cached. Instead, use a DMA barrier that
>ensures that the data is visible to the DMA master before the address is and
>move the memcpy() before the reference.
>
>Fixes: 579a267e4617 ("HID: intel-ish-hid: Implement loading firmware from
>host feature")
>Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>---
>I noticed this one while looking at the bug that was fixed in
>236049723826 ("HID: intel-ish-hid: Fix build error for COMPILE_TEST")
>---
> drivers/hid/intel-ish-hid/ishtp/loader.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/hid/intel-ish-hid/ishtp/loader.c b/drivers/hid/intel-ish-
>hid/ishtp/loader.c
>index 2785b04a2f5a..062d1b25eaa7 100644
>--- a/drivers/hid/intel-ish-hid/ishtp/loader.c
>+++ b/drivers/hid/intel-ish-hid/ishtp/loader.c
>@@ -33,7 +33,6 @@
>
> #define dev_fmt(fmt) "ISH loader: " fmt
>
>-#include <linux/cacheflush.h>
> #include <linux/container_of.h>
> #include <linux/dev_printk.h>
> #include <linux/dma-mapping.h>
>@@ -175,10 +174,11 @@ static int prepare_dma_bufs(struct ishtp_device
>*dev,
> 			return -ENOMEM;
>
> 		fragment->fragment_tbl[i].ddr_adrs =
>cpu_to_le64(dma_addr);
>+
>+		memcpy(dma_bufs[i], ish_fw->data + offset, fragment-
>>fragment_tbl[i].length);
fragment->fragment_tbl[i].length was used before assignment.
 
>+		dma_wmb();
I tested it on the platform, but it didn't wok.

Thanks,
Lixu

> 		fragment->fragment_tbl[i].length = clamp(ish_fw->size -
>offset, 0, fragment_size);
> 		fragment->fragment_tbl[i].fw_off = offset;
>-		memcpy(dma_bufs[i], ish_fw->data + offset, fragment-
>>fragment_tbl[i].length);
>-		clflush_cache_range(dma_bufs[i], fragment_size);
>
> 		offset += fragment->fragment_tbl[i].length;
> 	}
>--
>2.39.2

Re: [PATCH 1/2] HID: intel-ish-hid: fix cache management mistake

[Arnd Bergmann]

On Wed, May 29, 2024, at 08:46, Zhang, Lixu wrote:
>>
>>-#include <linux/cacheflush.h>
>> #include <linux/container_of.h>
>> #include <linux/dev_printk.h>
>> #include <linux/dma-mapping.h>
>>@@ -175,10 +174,11 @@ static int prepare_dma_bufs(struct ishtp_device
>>*dev,
>> 			return -ENOMEM;
>>
>> 		fragment->fragment_tbl[i].ddr_adrs =
>>cpu_to_le64(dma_addr);
>>+
>>+		memcpy(dma_bufs[i], ish_fw->data + offset, fragment-
>>>fragment_tbl[i].length);
> fragment->fragment_tbl[i].length was used before assignment.
> 
>>+		dma_wmb();
> I tested it on the platform, but it didn't wok.
>

What behavior do you see instead? If the manual cache
flush works around a bug, that would indicate that the
device itself is not coherent and the dma_alloc_coherent()
in the architecture is broken.

     Arnd

Re: [PATCH 1/2] HID: intel-ish-hid: fix cache management mistake

[srinivas pandruvada]

On Wed, 2024-05-29 at 09:06 +0200, Arnd Bergmann wrote:
> On Wed, May 29, 2024, at 08:46, Zhang, Lixu wrote:
> > > 
> > > -#include <linux/cacheflush.h>
> > > #include <linux/container_of.h>
> > > #include <linux/dev_printk.h>
> > > #include <linux/dma-mapping.h>
> > > @@ -175,10 +174,11 @@ static int prepare_dma_bufs(struct
> > > ishtp_device
> > > *dev,
> > > 			return -ENOMEM;
> > > 
> > > 		fragment->fragment_tbl[i].ddr_adrs =
> > > cpu_to_le64(dma_addr);
> > > +
> > > +		memcpy(dma_bufs[i], ish_fw->data + offset,
> > > fragment-
> > > > fragment_tbl[i].length);
> > fragment->fragment_tbl[i].length was used before assignment.
> > 
> > > +		dma_wmb();
> > I tested it on the platform, but it didn't wok.
> > 
> 
> What behavior do you see instead? If the manual cache
> flush works around a bug, that would indicate that the
> device itself is not coherent and the dma_alloc_coherent()
> in the architecture is broken.

Lixu,

What happens if you remove manual cache flush in your code?
It is possible that boot loader at this time not ready to do fully
coherent ops.

Thanks,
Srinivas



> 
>      Arnd

RE: [PATCH 1/2] HID: intel-ish-hid: fix cache management mistake

[Zhang, Lixu]

>-----Original Message-----
>From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
>Sent: Thursday, May 30, 2024 6:25 AM
>To: Arnd Bergmann <arnd@arndb.de>; Zhang, Lixu <lixu.zhang@intel.com>;
>Arnd Bergmann <arnd@kernel.org>; Jiri Kosina <jikos@kernel.org>; Benjamin
>Tissoires <bentiss@kernel.org>
>Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH 1/2] HID: intel-ish-hid: fix cache management mistake
>
>On Wed, 2024-05-29 at 09:06 +0200, Arnd Bergmann wrote:
>> On Wed, May 29, 2024, at 08:46, Zhang, Lixu wrote:
>> > >
>> >
>> > > +		dma_wmb();
>> > I tested it on the platform, but it didn't wok.
>> >
>>
>> What behavior do you see instead? 
Hi Arnd, please refer to the information below.

>> If the manual cache flush works
>> around a bug, that would indicate that the device itself is not
>> coherent and the dma_alloc_coherent() in the architecture is broken.
>
>Lixu,
>
>What happens if you remove manual cache flush in your code?
When the driver side sends the next start command, it receives an error response, which is likely because the bootloader failed to verify the firmware image.

Thanks,
Lixu

>It is possible that boot loader at this time not ready to do fully coherent ops.
>
>Thanks,
>Srinivas
>
>>      Arnd

RE: [PATCH 1/2] HID: intel-ish-hid: fix cache management mistake

[Zhang, Lixu]

>-----Original Message-----
>From: Zhang, Lixu
>Sent: Thursday, May 30, 2024 3:42 PM
>To: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>; Arnd
>Bergmann <arnd@arndb.de>; Arnd Bergmann <arnd@kernel.org>; Jiri Kosina
><jikos@kernel.org>; Benjamin Tissoires <bentiss@kernel.org>; Xu, Even
><even.xu@intel.com>
>Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: RE: [PATCH 1/2] HID: intel-ish-hid: fix cache management mistake
>
>>-----Original Message-----
>>From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
>>Sent: Thursday, May 30, 2024 6:25 AM
>>To: Arnd Bergmann <arnd@arndb.de>; Zhang, Lixu <lixu.zhang@intel.com>;
>>Arnd Bergmann <arnd@kernel.org>; Jiri Kosina <jikos@kernel.org>;
>>Benjamin Tissoires <bentiss@kernel.org>
>>Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
>>Subject: Re: [PATCH 1/2] HID: intel-ish-hid: fix cache management
>>mistake
>>
>>On Wed, 2024-05-29 at 09:06 +0200, Arnd Bergmann wrote:
>>> On Wed, May 29, 2024, at 08:46, Zhang, Lixu wrote:
>>> > >
>>> >
>>> > > +		dma_wmb();
>>> > I tested it on the platform, but it didn't wok.
>>> >
>>>
>>> What behavior do you see instead?
>Hi Arnd, please refer to the information below.
>
>>> If the manual cache flush works
>>> around a bug, that would indicate that the device itself is not
>>> coherent and the dma_alloc_coherent() in the architecture is broken.
>>
Hi Arnd,

Flush cache is necessary for some performance reason on this device.

Thanks,
Lixu