This patch set has various unrelated fixes in the dpio driver and the
dpaa2-console one.
Patches were applied on latest linux-next and formatted from there.
Diana Craciun (1):
soc: fsl: dpio: fix qbman alignment error in the virtualization
context
Ioana Ciornei (2):
soc: fsl: dpio: use an explicit NULL instead of 0
soc: fsl: dpio: fix kernel-doc warnings
Robert-Ionut Alexa (1):
soc: fsl: dpaa2-console: free buffer before returning from
dpaa2_console_read
Youri Querry (1):
soc: fsl: dpio: rename the enqueue descriptor variable
drivers/soc/fsl/dpaa2-console.c | 1 +
drivers/soc/fsl/dpio/dpio-service.c | 42 ++++++++--------
drivers/soc/fsl/dpio/qbman-portal.c | 76 ++++++++++++++---------------
drivers/soc/fsl/dpio/qbman-portal.h | 39 +++++++++------
4 files changed, 85 insertions(+), 73 deletions(-)
--
2.33.1
From: Diana Craciun <[email protected]>
When running as a guest, under KVM, the CENA region is mapped as device
memory, so uncacheable. When the memory is mapped as device memory, the
unaligned accesses are not allowed. Memcpy is optimized to transfer 8
bytes at a time regardless of the start address and might cause
alignment issues.
Signed-off-by: Diana Craciun <[email protected]>
Signed-off-by: Ioana Ciornei <[email protected]>
---
drivers/soc/fsl/dpio/qbman-portal.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/soc/fsl/dpio/qbman-portal.c b/drivers/soc/fsl/dpio/qbman-portal.c
index 3fd54611ed98..ef9cafd12534 100644
--- a/drivers/soc/fsl/dpio/qbman-portal.c
+++ b/drivers/soc/fsl/dpio/qbman-portal.c
@@ -679,9 +679,9 @@ int qbman_swp_enqueue_multiple_direct(struct qbman_swp *s,
for (i = 0; i < num_enqueued; i++) {
p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi & half_mask));
/* Skip copying the verb */
- memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
- memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)],
- &fd[i], sizeof(*fd));
+ memcpy_toio((__iomem void *)&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
+ memcpy_toio((__iomem void *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)],
+ &fd[i], sizeof(*fd));
eqcr_pi++;
}
@@ -763,9 +763,9 @@ int qbman_swp_enqueue_multiple_mem_back(struct qbman_swp *s,
for (i = 0; i < num_enqueued; i++) {
p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi & half_mask));
/* Skip copying the verb */
- memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
- memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)],
- &fd[i], sizeof(*fd));
+ memcpy_toio((__iomem void *)&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
+ memcpy_toio((__iomem void *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)],
+ &fd[i], sizeof(*fd));
eqcr_pi++;
}
@@ -837,9 +837,9 @@ int qbman_swp_enqueue_multiple_desc_direct(struct qbman_swp *s,
p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi & half_mask));
cl = (uint32_t *)(&d[i]);
/* Skip copying the verb */
- memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
- memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)],
- &fd[i], sizeof(*fd));
+ memcpy_toio((__iomem void *)&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
+ memcpy_toio((__iomem void *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)],
+ &fd[i], sizeof(*fd));
eqcr_pi++;
}
@@ -907,9 +907,9 @@ int qbman_swp_enqueue_multiple_desc_mem_back(struct qbman_swp *s,
p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi & half_mask));
cl = (uint32_t *)(&d[i]);
/* Skip copying the verb */
- memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
- memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)],
- &fd[i], sizeof(*fd));
+ memcpy_toio((__iomem void *)&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
+ memcpy_toio((__iomem void *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)],
+ &fd[i], sizeof(*fd));
eqcr_pi++;
}
--
2.33.1
> -----Original Message-----
> From: Ioana Ciornei <[email protected]>
> Sent: Monday, October 18, 2021 10:10 AM
> To: Leo Li <[email protected]>
> Cc: Youri Querry <[email protected]>; [email protected];
> Ioana Ciornei <[email protected]>
> Subject: [PATCH 0/5] soc: fsl: various fixes
>
> This patch set has various unrelated fixes in the dpio driver and the
> dpaa2-console one.
> Patches were applied on latest linux-next and formatted from there.
>
> Diana Craciun (1):
> soc: fsl: dpio: fix qbman alignment error in the virtualization
> context
>
> Ioana Ciornei (2):
> soc: fsl: dpio: use an explicit NULL instead of 0
> soc: fsl: dpio: fix kernel-doc warnings
>
> Robert-Ionut Alexa (1):
> soc: fsl: dpaa2-console: free buffer before returning from
> dpaa2_console_read
>
> Youri Querry (1):
> soc: fsl: dpio: rename the enqueue descriptor variable
Patch 4-5 are applied for fix.
Patch 1-2 are applied for next. Patch 3 cannot be applied cleanly, we probably can apply it after the changes in Linux-next get into the mainline.
Thanks.
>
> drivers/soc/fsl/dpaa2-console.c | 1 +
> drivers/soc/fsl/dpio/dpio-service.c | 42 ++++++++--------
> drivers/soc/fsl/dpio/qbman-portal.c | 76 ++++++++++++++---------------
> drivers/soc/fsl/dpio/qbman-portal.h | 39 +++++++++------
> 4 files changed, 85 insertions(+), 73 deletions(-)
>
> --
> 2.33.1
> -----Original Message-----
> From: Ioana Ciornei <[email protected]>
> Sent: Monday, October 18, 2021 10:11 AM
> To: Leo Li <[email protected]>
> Cc: Youri Querry <[email protected]>; [email protected];
> Diana Madalina Craciun <[email protected]>; Ioana Ciornei
> <[email protected]>
> Subject: [PATCH 4/5] soc: fsl: dpio: fix qbman alignment error in the
> virtualization context
>
> From: Diana Craciun <[email protected]>
>
> When running as a guest, under KVM, the CENA region is mapped as device
> memory, so uncacheable. When the memory is mapped as device memory,
> the unaligned accesses are not allowed. Memcpy is optimized to transfer 8
> bytes at a time regardless of the start address and might cause alignment
> issues.
>
> Signed-off-by: Diana Craciun <[email protected]>
> Signed-off-by: Ioana Ciornei <[email protected]>
We get the following feedback from Arnd about this patch. Could you respond to the comments?
"This patch looks very suspicious to me, I don't think it's generally safe to
use memcpy_toio() on a normal pointer, as the __iomem tokens may
be in a separate address range, even though this currently works
on arm64. Adding the (__iomem void *) cast without a comment that
explains why it's added seems similarly wrong, and finally the
changeset text does not seem to match what the code does:
According to the text, the pointer is to a virtual address mapped as
"device memory" (i.e. PROT_DEVICE_nGnRE or PROT_DEVICE_nGnRnE),
but the code suggests it's actually write-combining normal
(PROT_NORMAL_NC)."
> ---
> drivers/soc/fsl/dpio/qbman-portal.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/soc/fsl/dpio/qbman-portal.c
> b/drivers/soc/fsl/dpio/qbman-portal.c
> index 3fd54611ed98..ef9cafd12534 100644
> --- a/drivers/soc/fsl/dpio/qbman-portal.c
> +++ b/drivers/soc/fsl/dpio/qbman-portal.c
> @@ -679,9 +679,9 @@ int qbman_swp_enqueue_multiple_direct(struct
> qbman_swp *s,
> for (i = 0; i < num_enqueued; i++) {
> p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi &
> half_mask));
> /* Skip copying the verb */
> - memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
> - memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)],
> - &fd[i], sizeof(*fd));
> + memcpy_toio((__iomem void *)&p[1], &cl[1],
> EQ_DESC_SIZE_WITHOUT_FD - 1);
> + memcpy_toio((__iomem void
> *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)],
> + &fd[i], sizeof(*fd));
> eqcr_pi++;
> }
>
> @@ -763,9 +763,9 @@ int
> qbman_swp_enqueue_multiple_mem_back(struct qbman_swp *s,
> for (i = 0; i < num_enqueued; i++) {
> p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi &
> half_mask));
> /* Skip copying the verb */
> - memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
> - memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)],
> - &fd[i], sizeof(*fd));
> + memcpy_toio((__iomem void *)&p[1], &cl[1],
> EQ_DESC_SIZE_WITHOUT_FD - 1);
> + memcpy_toio((__iomem void
> *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)],
> + &fd[i], sizeof(*fd));
> eqcr_pi++;
> }
>
> @@ -837,9 +837,9 @@ int
> qbman_swp_enqueue_multiple_desc_direct(struct qbman_swp *s,
> p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi &
> half_mask));
> cl = (uint32_t *)(&d[i]);
> /* Skip copying the verb */
> - memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
> - memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)],
> - &fd[i], sizeof(*fd));
> + memcpy_toio((__iomem void *)&p[1], &cl[1],
> EQ_DESC_SIZE_WITHOUT_FD - 1);
> + memcpy_toio((__iomem void
> *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)],
> + &fd[i], sizeof(*fd));
> eqcr_pi++;
> }
>
> @@ -907,9 +907,9 @@ int
> qbman_swp_enqueue_multiple_desc_mem_back(struct qbman_swp *s,
> p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi &
> half_mask));
> cl = (uint32_t *)(&d[i]);
> /* Skip copying the verb */
> - memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
> - memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)],
> - &fd[i], sizeof(*fd));
> + memcpy_toio((__iomem void *)&p[1], &cl[1],
> EQ_DESC_SIZE_WITHOUT_FD - 1);
> + memcpy_toio((__iomem void
> *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)],
> + &fd[i], sizeof(*fd));
> eqcr_pi++;
> }
>
> --
> 2.33.1
Hi Leo,
Just drop this patch. I realize that the implementation is not correct.
The problem should be resolved (if possible) elsewhere.
I will open another thread on the relevant lists regarding this problem.
Diana
On 10/27/2021 2:47 AM, Leo Li wrote:
>
>> -----Original Message-----
>> From: Ioana Ciornei <[email protected]>
>> Sent: Monday, October 18, 2021 10:11 AM
>> To: Leo Li <[email protected]>
>> Cc: Youri Querry <[email protected]>; [email protected];
>> Diana Madalina Craciun <[email protected]>; Ioana Ciornei
>> <[email protected]>
>> Subject: [PATCH 4/5] soc: fsl: dpio: fix qbman alignment error in the
>> virtualization context
>>
>> From: Diana Craciun <[email protected]>
>>
>> When running as a guest, under KVM, the CENA region is mapped as device
>> memory, so uncacheable. When the memory is mapped as device memory,
>> the unaligned accesses are not allowed. Memcpy is optimized to transfer 8
>> bytes at a time regardless of the start address and might cause alignment
>> issues.
>>
>> Signed-off-by: Diana Craciun <[email protected]>
>> Signed-off-by: Ioana Ciornei <[email protected]>
> We get the following feedback from Arnd about this patch. Could you respond to the comments?
>
> "This patch looks very suspicious to me, I don't think it's generally safe to
> use memcpy_toio() on a normal pointer, as the __iomem tokens may
> be in a separate address range, even though this currently works
> on arm64. Adding the (__iomem void *) cast without a comment that
> explains why it's added seems similarly wrong, and finally the
> changeset text does not seem to match what the code does:
>
> According to the text, the pointer is to a virtual address mapped as
> "device memory" (i.e. PROT_DEVICE_nGnRE or PROT_DEVICE_nGnRnE),
> but the code suggests it's actually write-combining normal
> (PROT_NORMAL_NC)."
>
>> ---
>> drivers/soc/fsl/dpio/qbman-portal.c | 24 ++++++++++++------------
>> 1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/soc/fsl/dpio/qbman-portal.c
>> b/drivers/soc/fsl/dpio/qbman-portal.c
>> index 3fd54611ed98..ef9cafd12534 100644
>> --- a/drivers/soc/fsl/dpio/qbman-portal.c
>> +++ b/drivers/soc/fsl/dpio/qbman-portal.c
>> @@ -679,9 +679,9 @@ int qbman_swp_enqueue_multiple_direct(struct
>> qbman_swp *s,
>> for (i = 0; i < num_enqueued; i++) {
>> p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi &
>> half_mask));
>> /* Skip copying the verb */
>> - memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
>> - memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)],
>> - &fd[i], sizeof(*fd));
>> + memcpy_toio((__iomem void *)&p[1], &cl[1],
>> EQ_DESC_SIZE_WITHOUT_FD - 1);
>> + memcpy_toio((__iomem void
>> *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)],
>> + &fd[i], sizeof(*fd));
>> eqcr_pi++;
>> }
>>
>> @@ -763,9 +763,9 @@ int
>> qbman_swp_enqueue_multiple_mem_back(struct qbman_swp *s,
>> for (i = 0; i < num_enqueued; i++) {
>> p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi &
>> half_mask));
>> /* Skip copying the verb */
>> - memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
>> - memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)],
>> - &fd[i], sizeof(*fd));
>> + memcpy_toio((__iomem void *)&p[1], &cl[1],
>> EQ_DESC_SIZE_WITHOUT_FD - 1);
>> + memcpy_toio((__iomem void
>> *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)],
>> + &fd[i], sizeof(*fd));
>> eqcr_pi++;
>> }
>>
>> @@ -837,9 +837,9 @@ int
>> qbman_swp_enqueue_multiple_desc_direct(struct qbman_swp *s,
>> p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi &
>> half_mask));
>> cl = (uint32_t *)(&d[i]);
>> /* Skip copying the verb */
>> - memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
>> - memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)],
>> - &fd[i], sizeof(*fd));
>> + memcpy_toio((__iomem void *)&p[1], &cl[1],
>> EQ_DESC_SIZE_WITHOUT_FD - 1);
>> + memcpy_toio((__iomem void
>> *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)],
>> + &fd[i], sizeof(*fd));
>> eqcr_pi++;
>> }
>>
>> @@ -907,9 +907,9 @@ int
>> qbman_swp_enqueue_multiple_desc_mem_back(struct qbman_swp *s,
>> p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi &
>> half_mask));
>> cl = (uint32_t *)(&d[i]);
>> /* Skip copying the verb */
>> - memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
>> - memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)],
>> - &fd[i], sizeof(*fd));
>> + memcpy_toio((__iomem void *)&p[1], &cl[1],
>> EQ_DESC_SIZE_WITHOUT_FD - 1);
>> + memcpy_toio((__iomem void
>> *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)],
>> + &fd[i], sizeof(*fd));
>> eqcr_pi++;
>> }
>>
>> --
>> 2.33.1