From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1762443AbdAEJ2e (ORCPT ); Thu, 5 Jan 2017 04:28:34 -0500 Received: from mga07.intel.com ([134.134.136.100]:27114 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161140AbdAEJ21 (ORCPT ); Thu, 5 Jan 2017 04:28:27 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,320,1477983600"; d="asc'?scan'208";a="805371753" From: Felipe Balbi To: Baolin Wang Cc: Janusz Dziedzic , Greg KH , USB , LKML , Linaro Kernel Mailman List , Mark Brown Subject: Re: [PATCH] usb: dwc3: gadget: Avoid race between dwc3 interrupt handler and irq thread handler In-Reply-To: References: <0d79eb1f34e409749a136173b68f365b545c4789.1482738764.git.baolin.wang@linaro.org> <87wpel1vac.fsf@linux.intel.com> <8737h0z5kc.fsf@linux.intel.com> Date: Thu, 05 Jan 2017 11:26:40 +0200 Message-ID: <878tqpx3fz.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Baolin Wang writes: >>>>>>> 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 th= e USB gadget >>>>>>>>> irq thread handler by dwc3_thread_interrupt(), meanwhile another = core also can >>>>>>>>> respond other interrupts from dwc3 controller and modify the even= t buffer by >>>>>>>>> dwc3_interrupt() function, that will cause getting the wrong even= t count in >>>>>>>>> irq thread handler to make the USB function abnormal. >>>>>>>>> >>>>>>>>> We should add spin_lock/unlock() in dwc3_check_event_buf() to avo= id 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 c= orrectly? >>>>>>> >>>>>>> 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 *e= vt) >>>>>>> { >>>>>>> struct dwc3 *dwc =3D 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 =3D true; >>>>>>> return IRQ_HANDLED; >>>>>>> } >>>>>>> >>>>>>> count =3D dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0)); >>>>>>> count &=3D DWC3_GEVNTCOUNT_MASK; >>>>>>> if (!count) >>>>>>> return IRQ_NONE; >>>>>>> >>>>>>> evt->count =3D count; >>>>>>> evt->flags |=3D DWC3_EVENT_PENDING; >>>>>>> >>>>>>> /* Mask interrupt */ >>>>>>> reg =3D dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0)); >>>>>>> reg |=3D 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. IOW, you sent me a patch to be integrated in the tree which everybody in the whole world uses and you didn't even test anything in that very tree? How am I supposed to trust you're sending me tested patches from now on? Clearly you have no empathy for those working countless hours to keep this stable and working. If you're ready to send me a completely untested patch and claim that it's fixing a race condition you have never seen for yourself, it becomes difficult to trust any patches you're sending me. You should know better. Your employer has people on staff who are able to clarify all these "details". You should never, ever send me patches like this again. Mark, himself, has a long track record of contributing to upstream development; maybe you should have a discussion with him offline about this. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAlhuEVAACgkQzL64meEa mQbV3w//WIBCDJhGRvueeh2esQT01vRaDYwUfByirpdEhAhkpjuYaUSv8w8J1v3k nSMvo5FfX91PnKR8UOcbpdeu3oC0tCtha8cOch0t0sreE/pJTlDJAY+YpgIyNUO/ FbcDpLaTvZxzRaCEuEL97WFGUO3xR7N5tmmGUDZnmw8q4mnifdJ0CXs0/jBn5oej pyweouRX4fYjbIbUVirf6bixBRGpqSb39nT2QD4N+VZH0vCWmhEH3esYrOXU34ol Pq91hRJKlJTfjyop2TZk42x2yPpQ1BNDaDjmg8ZTO1u+9l4xSEgNvI0rxKZ/VwSu OOcmMPDteKta4xQBJz08O3hznMezvDG2uh2ZMv/S/4R8UxVQbF/HEmbrm1gDYHHU njH7Cl3rooww3yzO5NqUEqid2aIW2weeqDCHu+AuKXR4EeOZnaTIyhyTBHdxaaZ+ 0b4a5bQ1V91SrjRCwEWR+wngayf3Wxytkd5r5n38PHWR33rQNz0GHJnnSOExhiYA qkRvOQdf/+M7tUOD1x7mkb6oKQ3HkLqDJJ9/XGIcd5ELOjrgNgsJu/BbCSnHSrvo iiBvzDdI1YegHWVnq+1dav/rnyqgtDasx6S7FKgWE/tf0n1ebE1aXXUjSgDz9R63 BsaInQdCHpGjsxbCPENWLo8ZAwt1eHO4SI28+bIOCrQFUaXsQRQ= =pdjz -----END PGP SIGNATURE----- --=-=-=--