From mboxrd@z Thu Jan 1 00:00:00 1970 From: Felipe Balbi Date: Wed, 05 Apr 2017 11:18:21 +0300 Subject: [U-Boot] [PATCH] usb: dwc3: gadget: make cache-maintenance on event buffers more robust In-Reply-To: References: <1491241798-37084-1-git-send-email-philipp.tomsich@theobroma-systems.com> <8bb1f19c-a7f5-bbc8-3a0d-6c2de73a7410@denx.de> <52CB490A-0557-4C75-99B9-B9137D85C789@theobroma-systems.com> Message-ID: <87wpazgtde.fsf@linux.intel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de Hi, Marek Vasut writes: >>>> 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 >>>> >>>> --- >>>> >>>> drivers/usb/dwc3/core.c | 2 ++ >>>> drivers/usb/dwc3/gadget.c | 5 +++-- >>>> 2 files changed, 5 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>> index b2c7eb1..f58c7ba 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((long)evt->buf, evt->length); >>>> + >>> >>> Is the length aligned ? If not, you will get cache alignment warning. >>> Also, address should be uintptr_t to avoid 32/64 bit issues . >> >> The length is a well-known value and aligned (it expands to PAGE_SIZE in the end). > > Uh, the event buffer is 4k ? That's quite big, but OK. it really isn't when you're dealing with LPM. I've seen 4k cause overflow events before. >> Good point on the “long”, especially as I just copied this from other occurences and it’s consistently wrong throughout DWC3 in U-Boot: > > Hrm, I thought the driver was ported over from Linux, so is this broken > in Linux too ? haven't seen a problem in almost 6 years dealing with this IP. -- balbi