Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754726AbcL0MQ6 (ORCPT ); Tue, 27 Dec 2016 07:16:58 -0500 Received: from mail-oi0-f50.google.com ([209.85.218.50]:36661 "EHLO mail-oi0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754025AbcL0MQu (ORCPT ); Tue, 27 Dec 2016 07:16:50 -0500 MIME-Version: 1.0 In-Reply-To: <87wpel1vac.fsf@linux.intel.com> References: <0d79eb1f34e409749a136173b68f365b545c4789.1482738764.git.baolin.wang@linaro.org> <87wpel1vac.fsf@linux.intel.com> From: Baolin Wang Date: Tue, 27 Dec 2016 20:16:49 +0800 Message-ID: Subject: Re: [PATCH] usb: dwc3: gadget: Avoid race between dwc3 interrupt handler and irq thread handler To: Felipe Balbi Cc: Janusz Dziedzic , Greg KH , USB , LKML , Linaro Kernel Mailman List , Mark Brown Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2991 Lines: 94 Hi, On 27 December 2016 at 19:11, Felipe Balbi wrote: > > Hi, > > Baolin Wang writes: >> Hi, >> >> On 27 December 2016 at 18:52, Janusz Dziedzic wrote: >>> 2016-12-26 9:01 GMT+01:00 Baolin Wang : >>>> On some platfroms(like x86 platform), when one core is running the USB gadget >>>> irq thread handler by dwc3_thread_interrupt(), meanwhile another core also can >>>> respond other interrupts from dwc3 controller and modify the event buffer by >>>> dwc3_interrupt() function, that will cause getting the wrong event count in >>>> irq thread handler to make the USB function abnormal. >>>> >>>> We should add spin_lock/unlock() in dwc3_check_event_buf() to avoid this race. >>>> >>> Interesting, I always think we mask interrupt in dwc3_interrupt() by setting >>> DWC3_GEVNTSIZ_INTMASK >>> And unmask interrupt when we end dwc3_thread_interrupt(). >>> >>> So, we shouldn't get any IRQ from HW during dwc3_thread_interrupt(), >>> or I miss something? >>> Do you have some traces that indicate this masking will not work correctly? >> >> Yes, but we just masked the interrupts described in DEVTEN register, >> and we did not mask all the interrupts, like the endpoint command >> complete event, transfer complete event and so on, so we can still get >> interrupts. > > not true, we masked interrupts for the entire event buffer: Yes, you are right and I missed that. I should reproduce this problem and analyse the real reason. > >> static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt) >> { >> struct dwc3 *dwc = evt->dwc; >> u32 count; >> u32 reg; >> >> if (pm_runtime_suspended(dwc->dev)) { >> pm_runtime_get(dwc->dev); >> disable_irq_nosync(dwc->irq_gadget); >> dwc->pending_events = true; >> return IRQ_HANDLED; >> } >> >> count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0)); >> count &= DWC3_GEVNTCOUNT_MASK; >> if (!count) >> return IRQ_NONE; >> >> evt->count = count; >> evt->flags |= DWC3_EVENT_PENDING; >> >> /* Mask interrupt */ >> reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0)); >> reg |= DWC3_GEVNTSIZ_INTMASK; > > See here ?!? > >> dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), reg); >> >> return IRQ_WAKE_THREAD; >> } > >>> BTW, what value you get when problem occured, 0xFFFC? >> >> Yes, something like this, the event count become huge. > > please send us tracepoint data. You probably need to compress > it. Something like 256k of trace data is probably enough, so: > > # mkdir -p /t > # mount -t tracefs none /t > # cd /t > # echo 256 > buffer_size_kb > # echo 1 > events/dwc3/enable > # echo 0 > events/dwc3/dwc3_readl/enable > # echo 0 > events/dwc3/dwc3_writel/enable > > (reproduce) > > # cp /t/trace /path/to/non-volatile/media/trace.txt Okay, I try to do that. Thanks. -- Baolin.wang Best Regards