Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1553059940-127038-1-git-send-email-fei.yang@intel.com>
 <CALAqxLUW9XCq0WqTzpYGZ7B_4CVdsXY6t_dyuwhnOTJ7HajrwQ@mail.gmail.com> <02E7334B1630744CBDC55DA8586225837F7FB935@ORSMSX102.amr.corp.intel.com>
In-Reply-To: <02E7334B1630744CBDC55DA8586225837F7FB935@ORSMSX102.amr.corp.intel.com>
From:   John Stultz <john.stultz@linaro.org>
Date:   Wed, 20 Mar 2019 16:42:48 -0700
Message-ID: <CALAqxLXqPSt2X=vo1qsyz4mwPkKj9s2g2VSqQbrWUkG2KYSjaQ@mail.gmail.com>
Subject: Re: [PATCH V2] usb: gadget: f_fs: don't free buffer prematurely
To:     "Yang, Fei" <fei.yang@intel.com>
Cc:     Felipe Balbi <balbi@kernel.org>,
        Greg KH <gregkh@linuxfoundation.org>,
        "andrzej.p@collabora.com" <andrzej.p@collabora.com>,
        Vincent Pelletier <plr.vincent@gmail.com>,
        Linux USB List <linux-usb@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>, Josh Gao <jmgao@google.com>,
        Alistair Strachan <astrachan@google.com>,
        "Shen, JingX" <jingx.shen@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Mar 20, 2019 at 4:28 PM Yang, Fei <fei.yang@intel.com> wrote:
>
> > Hey Fei,
> >  So while this patch does resolve the issues I was seeing with mainline=
 kernels and recent changes to adbd,
> > Josh pointed out that it wouldn't resolve the issues I was seeing with =
older kernels which is slightly different (but still related to aio usage).
> >
> > On the older kernels I'm hitting scheduling while atomic on reboot, whi=
ch seems to be due to ffs_aio_cancel() taking a spinlock then calling usb_e=
p_dequeue() which might sleep.
> >
> > It seems a fix for this was tried earlier with d52e4d0c0c428 ("usb:
> > gadget: ffs: Fix BUG when userland exits with submitted AIO
> > transfers") which was then reverted by a9c859033f6e.
> >
> > Elsewhere it seems the ffs driver takes effort to drop any locks before=
 calling usb_ep_dequeue(), so this seems like
> > that should be addressed, but it also seems like recent change to the d=
wc3 driver has been made to avoid sleeping
> > in that path (see fec9095bdef4 ("usb: dwc3: gadget: remove wait_end_tra=
nsfer")), which may be why I'm not seeing
> > the problem with mainline (and your patch here, of coarse). But that al=
so doesn't clarify if its still a potential issue
> > w/ non-dwc3 platforms.
> >
> > So for older kernels, do you have a suggestion of which approach is adv=
ised?  Does usb_ep_dequeue need to avoid
> > sleeping or do we need to rework the ffs_aio_cancel logic?
>
> Are you seeing this issue with Android? When running adb reboot?
> I have tried 4.19 and 4.9 kernel with Android P-dessert on one of the Int=
el platforms, but no luck on reproducing the issue.

You probably need to be running AOSP/master to trigger this. The
changes which uncovered this landed just last week.

> I will get back to you if I could reproduce the issue. I'm afraid I won=
=E2=80=99t be able to do much by just looking at the code.

So as discussed in further mails, the main issue seems to be the dwc3
code was sleeping in its ep_dequeue logic, which isn't safe as
ffs_aio_cancel calls it while holding a spinlock. Upstream the dw3c
driver has been fixed, but -stable kernels still have the issue.

thanks
-john