From: Arnd Bergmann <[email protected]>
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 <[email protected]>
---
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
>-----Original Message-----
>From: Arnd Bergmann <[email protected]>
>Sent: Tuesday, May 28, 2024 7:58 PM
>To: Srinivas Pandruvada <[email protected]>; Jiri Kosina
><[email protected]>; Benjamin Tissoires <[email protected]>; Zhang, Lixu
><[email protected]>
>Cc: Arnd Bergmann <[email protected]>; [email protected]; linux-
>[email protected]
>Subject: [PATCH 1/2] HID: intel-ish-hid: fix cache management mistake
>
>From: Arnd Bergmann <[email protected]>
>
>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 <[email protected]>
>---
>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
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
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
>-----Original Message-----
>From: srinivas pandruvada <[email protected]>
>Sent: Thursday, May 30, 2024 6:25 AM
>To: Arnd Bergmann <[email protected]>; Zhang, Lixu <[email protected]>;
>Arnd Bergmann <[email protected]>; Jiri Kosina <[email protected]>; Benjamin
>Tissoires <[email protected]>
>Cc: [email protected]; [email protected]
>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
>-----Original Message-----
>From: Zhang, Lixu
>Sent: Thursday, May 30, 2024 3:42 PM
>To: srinivas pandruvada <[email protected]>; Arnd
>Bergmann <[email protected]>; Arnd Bergmann <[email protected]>; Jiri Kosina
><[email protected]>; Benjamin Tissoires <[email protected]>; Xu, Even
><[email protected]>
>Cc: [email protected]; [email protected]
>Subject: RE: [PATCH 1/2] HID: intel-ish-hid: fix cache management mistake
>
>>-----Original Message-----
>>From: srinivas pandruvada <[email protected]>
>>Sent: Thursday, May 30, 2024 6:25 AM
>>To: Arnd Bergmann <[email protected]>; Zhang, Lixu <[email protected]>;
>>Arnd Bergmann <[email protected]>; Jiri Kosina <[email protected]>;
>>Benjamin Tissoires <[email protected]>
>>Cc: [email protected]; [email protected]
>>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