From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S967613AbdAECHa (ORCPT ); Wed, 4 Jan 2017 21:07:30 -0500 Received: from mail-oi0-f49.google.com ([209.85.218.49]:35349 "EHLO mail-oi0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934627AbdAECHX (ORCPT ); Wed, 4 Jan 2017 21:07:23 -0500 MIME-Version: 1.0 In-Reply-To: <8737h0z5kc.fsf@linux.intel.com> References: <0d79eb1f34e409749a136173b68f365b545c4789.1482738764.git.baolin.wang@linaro.org> <87wpel1vac.fsf@linux.intel.com> <8737h0z5kc.fsf@linux.intel.com> From: Baolin Wang Date: Thu, 5 Jan 2017 10:07:22 +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 On 3 January 2017 at 20:33, Felipe Balbi wrote: > > Hi, > > Baolin Wang writes: >> On 28 December 2016 at 20:30, Janusz Dziedzic wrote: >>> 2016-12-27 13:16 GMT+01:00 Baolin Wang : >>>> 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. >>>>> >>> Probably you have little bit different code than current community >>> version (depends how your PM works). >>> >>> This is possible when we write: >>> dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 0); >>> And after that >>> dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 4); >>> >>> After that we will get 0xFFFC (-4). >>> >>> Possible races: >>> 1) dwc3_event_buffers_setup/dwc3_event_buffers_cleanup - write 0 >>> 2) dwc3_thread - write 4 >>> >>> While [1] could be called in PM work or UM context (init in Android >>> case) spin_lock_irqsave() will only disable local irqs and still we >>> could get IRQ on different core, next update evt->count and run >>> thread... >> >> Yeah, that's the possible races. > > and you have triggered this with mailine? How? We don't write to GEVNT* > registers from PM code and we only allow runtime_suspend with cable > dettached. Sorry for late reply since I am busy on other things. I just agreed with the possible races pointed by Janusz. I need to look at if these are happened on my platform and also I found some out of tree code which will clean the GEVNTCOUNT register when stop the gadget. I will check the mainline kernel and resend new patch if I make this problem clearly. Anyway thanks for your help and suggestion. -- Baolin.wang Best Regards