| Message ID | 20240528115802.3122955-1-arnd@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] HID: intel-ish-hid: fix cache management mistake | expand |
>-----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;
>-----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
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
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; }