From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com ([192.55.52.88]:35241 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751316AbcCIHVF (ORCPT ); Wed, 9 Mar 2016 02:21:05 -0500 From: Felipe Balbi To: "Felipe F. Tonello" , linux-usb@vger.kernel.org Cc: "# v4 . 4+" Subject: Re: [PATCH] usb: gadget: f_midi: added spinlock on transmit function In-Reply-To: <1457468507-25622-1-git-send-email-eu@felipetonello.com> References: <1457468507-25622-1-git-send-email-eu@felipetonello.com> Date: Wed, 09 Mar 2016 09:20:14 +0200 Message-ID: <874mcgglsh.fsf@intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" Sender: stable-owner@vger.kernel.org List-ID: --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, "Felipe F. Tonello" writes: > [ text/plain ] > Since f_midi_transmit is called by both ALSA and USB sub-systems, it can > potentially cause a race condition between both calls because f_midi_tran= smit > is not reentrant nor thread-safe. This is due to an implementation detail= that > the transmit function looks for the next available usb request from the f= ifo > and only enqueues it if there is data to send, otherwise just re-uses it.= So, > if both ALSA and USB frameworks calls this function at the same time, > kfifo_seek() will return the same usb_request, which will cause a race > condition. > > To solve this problem a syncronization mechanism is necessary. In this ca= se it > is used a spinlock since f_midi_transmit is also called by usb_request->c= omplete > callback in interrupt context. > > Cc: # v4.4+ this should be v4.5+ $ git describe e1e3d7ec5da3 v4.4-rc5-59-ge1e3d7ec5da3 > @@ -651,17 +653,22 @@ static void f_midi_transmit(struct f_midi *midi) > { > struct usb_ep *ep =3D midi->in_ep; > int ret; > + unsigned long flags; >=20=20 > /* We only care about USB requests if IN endpoint is enabled */ > if (!ep || !ep->enabled) > goto drop_out; >=20=20 > + spin_lock_irqsave(&midi->transmit_lock, flags); > + > do { > ret =3D f_midi_do_transmit(midi, ep); you wrote this commit on top of 'next' right ? I know that because of this call to f_midi_do_transmit() here which is introduced by commit aac887442d5e ("usb: gadget: f_midi: move some of f_midi_transmit to separate func") which is not in Linus' tree yet. This prevents me from taking this patch during current -rc. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW386vAAoJEIaOsuA1yqREDU8P/1UTnoRAIZhG5iAQYIBSsTpN JYYnyJdksGdvo2Ynx0q3/UsEMWKwDVFnBfWDVfooKawU640KZ0/+6Y6equ/Dg5uj 3CR/sgf0T+7ya1t7vYdF9urvFbI9Vhl4FXtoGfdhWMJImCKBGVqxHP4D5W9d7yp7 8oAnU/jmL6A9SxBa8ePh4EoNsFrWjC0mfVkRr/rXUOKmmQTccmvC5hrTgb+WuYfz +eHkDMLqqSDKlx2AbIT8I4U/VLjYHyErnwP4iSzUpPx/JP0vL2WS2QkGy6KnNqPf mz259iwltz/E8qHTc8/XFkQkSGvksbIE2pkqX9+4seOBzs7NBJnYgehDpQ2Ag+DV 1pdoq8xOhIGQDDxMdA8hNZVa5r5Jyl9ErEKDuNVwoBZLJtSO35PiiHoH2U4LcBIb OYC8RGmJoMVbxdoBGX7BfzdJxUar0IXf1V01SKhkq/FhVLX7tUnDK3Exq2PLpWX5 cddcjs9f8+jUZ2Ted/FA6+jhAtV6opNUoSsOFF9d5c6FikVmyfATDJcJt4QQ7xOC bWW9nBzxW9FAUaypAcBMjyx8zG+mJMWHUprvyJlImiXnXqZn/9Pjl4coX5oG/WiI UApwCO4FHUQWacK76uIDswaNqYzNjH0LCIQhBRSggoxE4vHM2axq4VidMCZ164zD q/8AwC9YpBNd2ret/fXW =zzaR -----END PGP SIGNATURE----- --=-=-=--