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.toms...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
