From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com ([192.55.52.120]:62957 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751137AbdEPIJp (ORCPT ); Tue, 16 May 2017 04:09:45 -0400 From: Felipe Balbi To: Thinh Nguyen , linux-usb@vger.kernel.org, Philipp Gesang , Ingo Molnar Cc: John Youn , Alan Stern , stable@vger.kernel.org Subject: Re: [PATCH v2 2/2] usb: gadget: f_mass_storage: Serialize wake and sleep execution In-Reply-To: References: <9b652321e763734cb23bd065a8daed4ddb406249.1494546251.git.thinhn@synopsys.com> Date: Tue, 16 May 2017 11:09:24 +0300 Message-ID: <87tw4lw7d7.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: stable-owner@vger.kernel.org List-ID: --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Thinh Nguyen writes: > f_mass_storage has a memorry barrier issue with the sleep and wake > functions that can cause a deadlock. This results in intermittent hangs > during MSC file transfer. The host will reset the device after receiving > no response to resume the transfer. This issue is seen when dwc3 is > processing 2 transfer-in-progress events at the same time, invoking > completion handlers for CSW and CBW. Also this issue occurs depending on > the system timing and latency. > > To increase the chance to hit this issue, you can force dwc3 driver to > wait and process those 2 events at once by adding a small delay (~100us) > in dwc3_check_event_buf() whenever the request is for CSW and read the > event count again. Avoid debugging with printk and ftrace as extra > delays and memory barrier will mask this issue. > > Scenario which can lead to failure: > ----------------------------------- > 1) The main thread sleeps and waits for the next command in > get_next_command(). > 2) bulk_in_complete() wakes up main thread for CSW. > 3) bulk_out_complete() tries to wake up the running main thread for CBW. > 4) thread_wakeup_needed is not loaded with correct value in > sleep_thread(). > 5) Main thread goes to sleep again. > > The pattern is shown below. Note the 2 critical variables. > * common->thread_wakeup_needed > * bh->state > > CPU 0 (sleep_thread) CPU 1 (wakeup_thread) > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > bh->state =3D BH_STATE_FULL; > smp_wmb(); > thread_wakeup_needed =3D 0; thread_wakeup_needed =3D 1; > smp_rmb(); > if (bh->state !=3D BH_STATE_FULL) > sleep again ... > > As pointed out by Alan Stern, this is an R-pattern issue. The issue can > be seen when there are two wakeups in quick succession. The > thread_wakeup_needed can be overwritten in sleep_thread, and the read of > the bh->state maybe reordered before the write to thread_wakeup_needed. > > This patch applies full memory barrier smp_mb() in both sleep_thread() > and wakeup_thread() to ensure the order which the thread_wakeup_needed > and bh->state are written and loaded. > > However, a better solution in the future would be to use wait_queue > method that takes care of managing memory barrier between waker and > waiter. > > Cc: stable@vger.kernel.org > Signed-off-by: Thinh Nguyen Alan, just to make sure you're okay with this patch. Can I have your acked-by? =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAlkas7QACgkQzL64meEa mQbMshAA3S+tr/47ZlroHmOQ0+0ReKsQgNHKkJO381Alkyq1c15LP2QPGQ5Xs+Va jY3SRAPnc/u10zu74SOuzbLGgPC4eQdloziciZM7WupiNgr8H6DNoWX1TOJid5Ki NfCnxw9eQiRr6ivMOqRtHIV51HokcgKB57JlnLMQjmjuws/0z4YsCPzfmC+3u2Gp GY5xqVPpkkC29TckiUneTTkkkwgljY2liWDg/UTtF1B9gS7SqMnxPyLnyz0x8TF3 Cu1CZw7MWuDth7hrhYESDB8QP5GXNuV3TBTs4BWGmR6vLo9nuQtJk/WWA6/fHN0m xjkz+XI7zVRKPEitLZJul4/0j8q7ZD2VcvIL1MyEYLxpHp0yR9sHTabZ+9E7N8/4 sZhyhXt79mURYyjWxRie8JYjq7luPueJ7n0aJJnOic3Z75JLbT5tICRAsWwQVtBw gL4wkxAPHWNDMtEFFDflJ9738sVBGqeK7cZc4eT0ZxgMlRuZpayMKi+3mUOyClUm 8+8uXvgfTNZw1uuFS9rOebEmV+6tIIvjaLp10+5PcnkkI39iE7DIia2+fIGTWOLi ydjydYmtUGZest4YknJY7dorufsG9bOi0IfJ/Svjq6PROecpxvdFyPNC0+x/JlAp H9Iv9dzcYUo1xNt/Fv/RMSDKcc4LDO512+ZubkkfyItBh1QVhOs= =ep1y -----END PGP SIGNATURE----- --=-=-=--