Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754183AbcL0LGv (ORCPT ); Tue, 27 Dec 2016 06:06:51 -0500 Received: from mail-oi0-f50.google.com ([209.85.218.50]:33797 "EHLO mail-oi0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751902AbcL0LGr (ORCPT ); Tue, 27 Dec 2016 06:06:47 -0500 MIME-Version: 1.0 In-Reply-To: References: <0d79eb1f34e409749a136173b68f365b545c4789.1482738764.git.baolin.wang@linaro.org> From: Baolin Wang Date: Tue, 27 Dec 2016 19:06:46 +0800 Message-ID: Subject: Re: [PATCH] usb: dwc3: gadget: Avoid race between dwc3 interrupt handler and irq thread handler To: Janusz Dziedzic Cc: Felipe Balbi , 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: 2745 Lines: 84 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. > > BTW, what value you get when problem occured, 0xFFFC? Yes, something like this, the event count become huge. > > BR > Janusz > >> Signed-off-by: Baolin Wang >> --- >> drivers/usb/dwc3/gadget.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 6785595..1a1e1f4 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2894,10 +2894,13 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt) >> return IRQ_HANDLED; >> } >> >> + spin_lock(&dwc->lock); >> count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0)); >> count &= DWC3_GEVNTCOUNT_MASK; >> - if (!count) >> + if (!count) { >> + spin_unlock(&dwc->lock); >> return IRQ_NONE; >> + } >> >> evt->count = count; >> evt->flags |= DWC3_EVENT_PENDING; >> @@ -2914,6 +2917,7 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt) >> memcpy(evt->cache, evt->buf, count - amount); >> >> dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count); >> + spin_unlock(&dwc->lock); >> >> return IRQ_WAKE_THREAD; >> } >> -- >> 1.7.9.5 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-usb" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > Janusz Dziedzic -- Baolin.wang Best Regards