* [U-Boot] [PATCH v2] usb: dwc3: gadget: make cache-maintenance on event buffers more robust
@ 2017-04-05 20:25 Philipp Tomsich
2017-04-05 21:50 ` Marek Vasut
0 siblings, 1 reply; 2+ messages in thread
From: Philipp Tomsich @ 2017-04-05 20:25 UTC (permalink / raw)
To: u-boot
Merely using dma_alloc_coherent does not ensure that there is no stale
data left in the caches for the allocated DMA buffer (i.e. that the
affected cacheline may still be dirty).
The original code was doing the following (on AArch64, which
translates a 'flush' into a 'clean + invalidate'):
# during initialisation:
1. allocate buffers via memalign
=> buffers may still be modified (cached, dirty)
# during interrupt processing
2. clean + invalidate buffers
=> may commit stale data from a modified cacheline
3. read from buffers
This could lead to garbage info being written to buffers before
reading them during even-processing.
To make the event processing more robust, we use the following sequence
for the cache-maintenance:
# during initialisation:
1. allocate buffers via memalign
2. clean + invalidate buffers
(we only need the 'invalidate' part, but dwc3_flush_cache()
always performs a 'clean + invalidate')
# during interrupt processing
3. read the buffers
(we know these lines are not cached, due to the previous
invalidation and no other code touching them in-between)
4. clean + invalidate buffers
=> writes back any modification we may have made during event
processing and ensures that the lines are not in the cache
the next time we enter interrupt processing
Note that with the original sequence, we observe reproducible
(depending on the cache state: i.e. running dhcp/usb start before will
upset caches to get us around this) issues in the event processing (a
fatal synchronous abort in dwc3_gadget_uboot_handle_interrupt on the
first time interrupt handling is invoked) when running USB mass
storage emulation on our RK3399-Q7 with data-caches on.
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---
Changes in v2:
- Consistently use uintptr_t for dwc3_flush_cache(addr, length).
drivers/usb/dwc3/core.c | 2 ++
drivers/usb/dwc3/ep0.c | 10 +++++-----
drivers/usb/dwc3/gadget.c | 15 ++++++++-------
drivers/usb/dwc3/io.h | 2 +-
4 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index b2c7eb1..1cbf179 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -125,6 +125,8 @@ static struct dwc3_event_buffer *dwc3_alloc_one_event_buffer(struct dwc3 *dwc,
if (!evt->buf)
return ERR_PTR(-ENOMEM);
+ dwc3_flush_cache((uintptr_t)evt->buf, evt->length);
+
return evt;
}
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 12b133f..e61d980 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -81,8 +81,8 @@ static int dwc3_ep0_start_trans(struct dwc3 *dwc, u8 epnum, dma_addr_t buf_dma,
trb->ctrl |= (DWC3_TRB_CTRL_IOC
| DWC3_TRB_CTRL_LST);
- dwc3_flush_cache((long)buf_dma, len);
- dwc3_flush_cache((long)trb, sizeof(*trb));
+ dwc3_flush_cache((uintptr_t)buf_dma, len);
+ dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
if (chain)
return 0;
@@ -790,7 +790,7 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
if (!r)
return;
- dwc3_flush_cache((long)trb, sizeof(*trb));
+ dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
status = DWC3_TRB_SIZE_TRBSTS(trb->size);
if (status == DWC3_TRBSTS_SETUP_PENDING) {
@@ -821,7 +821,7 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
ur->actual += transferred;
trb++;
- dwc3_flush_cache((long)trb, sizeof(*trb));
+ dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
length = trb->size & DWC3_TRB_SIZE_MASK;
ep0->free_slot = 0;
@@ -831,7 +831,7 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
maxp);
transferred = min_t(u32, ur->length - transferred,
transfer_size - length);
- dwc3_flush_cache((long)dwc->ep0_bounce, DWC3_EP0_BOUNCE_SIZE);
+ dwc3_flush_cache((uintptr_t)dwc->ep0_bounce, DWC3_EP0_BOUNCE_SIZE);
memcpy(buf, dwc->ep0_bounce, transferred);
} else {
transferred = ur->length - length;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 1156662..e065c5a 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -244,7 +244,7 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
list_del(&req->list);
req->trb = NULL;
- dwc3_flush_cache((long)req->request.dma, req->request.length);
+ dwc3_flush_cache((uintptr_t)req->request.dma, req->request.length);
if (req->request.status == -EINPROGRESS)
req->request.status = status;
@@ -771,8 +771,8 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
trb->ctrl |= DWC3_TRB_CTRL_HWO;
- dwc3_flush_cache((long)dma, length);
- dwc3_flush_cache((long)trb, sizeof(*trb));
+ dwc3_flush_cache((uintptr_t)dma, length);
+ dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
}
/*
@@ -1769,7 +1769,7 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
slot %= DWC3_TRB_NUM;
trb = &dep->trb_pool[slot];
- dwc3_flush_cache((long)trb, sizeof(*trb));
+ dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
__dwc3_cleanup_done_trbs(dwc, dep, req, trb, event, status);
dwc3_gadget_giveback(dep, req, status);
@@ -2668,11 +2668,12 @@ void dwc3_gadget_uboot_handle_interrupt(struct dwc3 *dwc)
int i;
struct dwc3_event_buffer *evt;
+ dwc3_thread_interrupt(0, dwc);
+
+ /* Clean + Invalidate the buffers after touching them */
for (i = 0; i < dwc->num_event_buffers; i++) {
evt = dwc->ev_buffs[i];
- dwc3_flush_cache((long)evt->buf, evt->length);
+ dwc3_flush_cache((uintptr_t)evt->buf, evt->length);
}
-
- dwc3_thread_interrupt(0, dwc);
}
}
diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
index 0d9fa22..810980f 100644
--- a/drivers/usb/dwc3/io.h
+++ b/drivers/usb/dwc3/io.h
@@ -48,7 +48,7 @@ static inline void dwc3_writel(void __iomem *base, u32 offset, u32 value)
writel(value, base + offs);
}
-static inline void dwc3_flush_cache(int addr, int length)
+static inline void dwc3_flush_cache(uintptr_t addr, int length)
{
flush_dcache_range(addr, addr + ROUND(length, CACHELINE_SIZE));
}
--
1.9.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* [U-Boot] [PATCH v2] usb: dwc3: gadget: make cache-maintenance on event buffers more robust
2017-04-05 20:25 [U-Boot] [PATCH v2] usb: dwc3: gadget: make cache-maintenance on event buffers more robust Philipp Tomsich
@ 2017-04-05 21:50 ` Marek Vasut
0 siblings, 0 replies; 2+ messages in thread
From: Marek Vasut @ 2017-04-05 21:50 UTC (permalink / raw)
To: u-boot
On 04/05/2017 10:25 PM, Philipp Tomsich wrote:
> Merely using dma_alloc_coherent does not ensure that there is no stale
> data left in the caches for the allocated DMA buffer (i.e. that the
> affected cacheline may still be dirty).
>
> The original code was doing the following (on AArch64, which
> translates a 'flush' into a 'clean + invalidate'):
> # during initialisation:
> 1. allocate buffers via memalign
> => buffers may still be modified (cached, dirty)
> # during interrupt processing
> 2. clean + invalidate buffers
> => may commit stale data from a modified cacheline
> 3. read from buffers
>
> This could lead to garbage info being written to buffers before
> reading them during even-processing.
>
> To make the event processing more robust, we use the following sequence
> for the cache-maintenance:
> # during initialisation:
> 1. allocate buffers via memalign
> 2. clean + invalidate buffers
> (we only need the 'invalidate' part, but dwc3_flush_cache()
> always performs a 'clean + invalidate')
> # during interrupt processing
> 3. read the buffers
> (we know these lines are not cached, due to the previous
> invalidation and no other code touching them in-between)
> 4. clean + invalidate buffers
> => writes back any modification we may have made during event
> processing and ensures that the lines are not in the cache
> the next time we enter interrupt processing
>
> Note that with the original sequence, we observe reproducible
> (depending on the cache state: i.e. running dhcp/usb start before will
> upset caches to get us around this) issues in the event processing (a
> fatal synchronous abort in dwc3_gadget_uboot_handle_interrupt on the
> first time interrupt handling is invoked) when running USB mass
> storage emulation on our RK3399-Q7 with data-caches on.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
The patch is fine, but it should really be two patches, one which
changes the types and one which fixes the caches, so it's clearly
bisectable. Can you do a V3 ? Thanks
> ---
>
> Changes in v2:
> - Consistently use uintptr_t for dwc3_flush_cache(addr, length).
>
> drivers/usb/dwc3/core.c | 2 ++
> drivers/usb/dwc3/ep0.c | 10 +++++-----
> drivers/usb/dwc3/gadget.c | 15 ++++++++-------
> drivers/usb/dwc3/io.h | 2 +-
> 4 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index b2c7eb1..1cbf179 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -125,6 +125,8 @@ static struct dwc3_event_buffer *dwc3_alloc_one_event_buffer(struct dwc3 *dwc,
> if (!evt->buf)
> return ERR_PTR(-ENOMEM);
>
> + dwc3_flush_cache((uintptr_t)evt->buf, evt->length);
> +
> return evt;
> }
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 12b133f..e61d980 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -81,8 +81,8 @@ static int dwc3_ep0_start_trans(struct dwc3 *dwc, u8 epnum, dma_addr_t buf_dma,
> trb->ctrl |= (DWC3_TRB_CTRL_IOC
> | DWC3_TRB_CTRL_LST);
>
> - dwc3_flush_cache((long)buf_dma, len);
> - dwc3_flush_cache((long)trb, sizeof(*trb));
> + dwc3_flush_cache((uintptr_t)buf_dma, len);
> + dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
>
> if (chain)
> return 0;
> @@ -790,7 +790,7 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
> if (!r)
> return;
>
> - dwc3_flush_cache((long)trb, sizeof(*trb));
> + dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
>
> status = DWC3_TRB_SIZE_TRBSTS(trb->size);
> if (status == DWC3_TRBSTS_SETUP_PENDING) {
> @@ -821,7 +821,7 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
> ur->actual += transferred;
>
> trb++;
> - dwc3_flush_cache((long)trb, sizeof(*trb));
> + dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
> length = trb->size & DWC3_TRB_SIZE_MASK;
>
> ep0->free_slot = 0;
> @@ -831,7 +831,7 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
> maxp);
> transferred = min_t(u32, ur->length - transferred,
> transfer_size - length);
> - dwc3_flush_cache((long)dwc->ep0_bounce, DWC3_EP0_BOUNCE_SIZE);
> + dwc3_flush_cache((uintptr_t)dwc->ep0_bounce, DWC3_EP0_BOUNCE_SIZE);
> memcpy(buf, dwc->ep0_bounce, transferred);
> } else {
> transferred = ur->length - length;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 1156662..e065c5a 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -244,7 +244,7 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
>
> list_del(&req->list);
> req->trb = NULL;
> - dwc3_flush_cache((long)req->request.dma, req->request.length);
> + dwc3_flush_cache((uintptr_t)req->request.dma, req->request.length);
>
> if (req->request.status == -EINPROGRESS)
> req->request.status = status;
> @@ -771,8 +771,8 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
>
> trb->ctrl |= DWC3_TRB_CTRL_HWO;
>
> - dwc3_flush_cache((long)dma, length);
> - dwc3_flush_cache((long)trb, sizeof(*trb));
> + dwc3_flush_cache((uintptr_t)dma, length);
> + dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
> }
>
> /*
> @@ -1769,7 +1769,7 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
> slot %= DWC3_TRB_NUM;
> trb = &dep->trb_pool[slot];
>
> - dwc3_flush_cache((long)trb, sizeof(*trb));
> + dwc3_flush_cache((uintptr_t)trb, sizeof(*trb));
> __dwc3_cleanup_done_trbs(dwc, dep, req, trb, event, status);
> dwc3_gadget_giveback(dep, req, status);
>
> @@ -2668,11 +2668,12 @@ void dwc3_gadget_uboot_handle_interrupt(struct dwc3 *dwc)
> int i;
> struct dwc3_event_buffer *evt;
>
> + dwc3_thread_interrupt(0, dwc);
> +
> + /* Clean + Invalidate the buffers after touching them */
> for (i = 0; i < dwc->num_event_buffers; i++) {
> evt = dwc->ev_buffs[i];
> - dwc3_flush_cache((long)evt->buf, evt->length);
> + dwc3_flush_cache((uintptr_t)evt->buf, evt->length);
> }
> -
> - dwc3_thread_interrupt(0, dwc);
> }
> }
> diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
> index 0d9fa22..810980f 100644
> --- a/drivers/usb/dwc3/io.h
> +++ b/drivers/usb/dwc3/io.h
> @@ -48,7 +48,7 @@ static inline void dwc3_writel(void __iomem *base, u32 offset, u32 value)
> writel(value, base + offs);
> }
>
> -static inline void dwc3_flush_cache(int addr, int length)
> +static inline void dwc3_flush_cache(uintptr_t addr, int length)
> {
> flush_dcache_range(addr, addr + ROUND(length, CACHELINE_SIZE));
> }
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2017-04-05 21:50 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-05 20:25 [U-Boot] [PATCH v2] usb: dwc3: gadget: make cache-maintenance on event buffers more robust Philipp Tomsich
2017-04-05 21:50 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.