From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S970664AbeEXPDA (ORCPT ); Thu, 24 May 2018 11:03:00 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:55244 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S970650AbeEXPC6 (ORCPT ); Thu, 24 May 2018 11:02:58 -0400 Date: Thu, 24 May 2018 11:02:57 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Martin Liu cc: gregkh@linuxfoundation.org, , , Subject: Re: [RFC] driver core: don't hold dev's parent lock when using async probe In-Reply-To: <20180524140021.GA214888@google.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 24 May 2018, Martin Liu wrote: > On Tue, May 22, 2018 at 01:09:44PM -0400, Alan Stern wrote: > > On Tue, 22 May 2018, martin_liu wrote: > > > > > not sure if we still need 'bf74ad5bc417 ("[PATCH] Hold the > > > device's parent's lock during probe and remove")' since it has > > > been there over 10 years. If we still need it and hard to fix it > > > , the simple way is to find a place not to allow USB subsystem > > > drivers to have async probe capability. Any suggestion is welcome. > > > > I don't think the "allows_async_probing" attribute is the best way to > > attack this. Some other approach, like a special-purpose flag, might > > be better. > > > > Yes, USB still needs to have parent's locks held during probing. > > Here's the reason. A USB device can have multiple interfaces, each > > bound to its own driver. A driver may sometimes need to issue a reset, > > but in USB there's no way to reset a single interface. Only the entire > > device can be reset, and of course this affects all the interfaces. > > Therefore a driver needs to acquire the device lock before it can issue > > a reset. > > > > The problem is that the driver's thread may already hold the device > > lock. During a normal probe sequence, for example, the interfaces get > > probed by the hub driver while it owns the device lock. But for probes > > under other circumstances (for example, if the user writes to the > > driver's "bind" attribute in sysfs), the device lock might not be held. > > > > A driver cannot tell these two cases apart. The only way to make it > > work all the time is to have the caller _always_ hold the device lock > > while the driver is probed (or the removed, for that matter). > > > > Alan Stern > > Thanks for the reply and more detail about the backgroud. I'd like to > have a conclusion about it. Please kindly correct me if my understanding > is wrong. Regarding to the "special-purpose flag", do you mean we could > find a place in USB subsystem to have the flag set (not sure if it's > easy to find it). Driver core would be base on the flag to decide if we > need to hold the device's parent's lock. Yes, except that the flag would not be in the USB subsystem. It would be in the device, device_type, or bus_type structure, so that the driver core could access it. Alan Stern From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [RFC] driver core: don't hold dev's parent lock when using async probe From: Alan Stern Message-Id: Date: Thu, 24 May 2018 11:02:57 -0400 (EDT) To: Martin Liu Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, jenhaochen@google.com List-ID: T24gVGh1LCAyNCBNYXkgMjAxOCwgTWFydGluIExpdSB3cm90ZToKCj4gT24gVHVlLCBNYXkgMjIs IDIwMTggYXQgMDE6MDk6NDRQTSAtMDQwMCwgQWxhbiBTdGVybiB3cm90ZToKPiA+IE9uIFR1ZSwg MjIgTWF5IDIwMTgsIG1hcnRpbl9saXUgd3JvdGU6Cj4gPiAKPiA+ID4gbm90IHN1cmUgaWYgd2Ug c3RpbGwgbmVlZCAnYmY3NGFkNWJjNDE3ICgiW1BBVENIXSBIb2xkIHRoZQo+ID4gPiBkZXZpY2Un cyBwYXJlbnQncyBsb2NrIGR1cmluZyBwcm9iZSBhbmQgcmVtb3ZlIiknIHNpbmNlIGl0IGhhcwo+ ID4gPiBiZWVuIHRoZXJlIG92ZXIgMTAgeWVhcnMuIElmIHdlIHN0aWxsIG5lZWQgaXQgYW5kIGhh cmQgdG8gZml4IGl0Cj4gPiA+ICwgdGhlIHNpbXBsZSB3YXkgaXMgdG8gZmluZCBhIHBsYWNlIG5v dCB0byBhbGxvdyBVU0Igc3Vic3lzdGVtCj4gPiA+IGRyaXZlcnMgdG8gaGF2ZSBhc3luYyBwcm9i ZSBjYXBhYmlsaXR5LiBBbnkgc3VnZ2VzdGlvbiBpcyB3ZWxjb21lLgo+ID4gCj4gPiBJIGRvbid0 IHRoaW5rIHRoZSAiYWxsb3dzX2FzeW5jX3Byb2JpbmciIGF0dHJpYnV0ZSBpcyB0aGUgYmVzdCB3 YXkgdG8gCj4gPiBhdHRhY2sgdGhpcy4gIFNvbWUgb3RoZXIgYXBwcm9hY2gsIGxpa2UgYSBzcGVj aWFsLXB1cnBvc2UgZmxhZywgbWlnaHQgCj4gPiBiZSBiZXR0ZXIuCj4gPiAKPiA+IFllcywgVVNC IHN0aWxsIG5lZWRzIHRvIGhhdmUgcGFyZW50J3MgbG9ja3MgaGVsZCBkdXJpbmcgcHJvYmluZy4g IAo+ID4gSGVyZSdzIHRoZSByZWFzb24uICBBIFVTQiBkZXZpY2UgY2FuIGhhdmUgbXVsdGlwbGUg aW50ZXJmYWNlcywgZWFjaAo+ID4gYm91bmQgdG8gaXRzIG93biBkcml2ZXIuICBBIGRyaXZlciBt YXkgc29tZXRpbWVzIG5lZWQgdG8gaXNzdWUgYSByZXNldCwKPiA+IGJ1dCBpbiBVU0IgdGhlcmUn cyBubyB3YXkgdG8gcmVzZXQgYSBzaW5nbGUgaW50ZXJmYWNlLiAgT25seSB0aGUgZW50aXJlCj4g PiBkZXZpY2UgY2FuIGJlIHJlc2V0LCBhbmQgb2YgY291cnNlIHRoaXMgYWZmZWN0cyBhbGwgdGhl IGludGVyZmFjZXMuICAKPiA+IFRoZXJlZm9yZSBhIGRyaXZlciBuZWVkcyB0byBhY3F1aXJlIHRo ZSBkZXZpY2UgbG9jayBiZWZvcmUgaXQgY2FuIGlzc3VlCj4gPiBhIHJlc2V0Lgo+ID4gCj4gPiBU aGUgcHJvYmxlbSBpcyB0aGF0IHRoZSBkcml2ZXIncyB0aHJlYWQgbWF5IGFscmVhZHkgaG9sZCB0 aGUgZGV2aWNlCj4gPiBsb2NrLiAgRHVyaW5nIGEgbm9ybWFsIHByb2JlIHNlcXVlbmNlLCBmb3Ig ZXhhbXBsZSwgdGhlIGludGVyZmFjZXMgZ2V0Cj4gPiBwcm9iZWQgYnkgdGhlIGh1YiBkcml2ZXIg d2hpbGUgaXQgb3ducyB0aGUgZGV2aWNlIGxvY2suICBCdXQgZm9yIHByb2Jlcwo+ID4gdW5kZXIg b3RoZXIgY2lyY3Vtc3RhbmNlcyAoZm9yIGV4YW1wbGUsIGlmIHRoZSB1c2VyIHdyaXRlcyB0byB0 aGUKPiA+IGRyaXZlcidzICJiaW5kIiBhdHRyaWJ1dGUgaW4gc3lzZnMpLCB0aGUgZGV2aWNlIGxv Y2sgbWlnaHQgbm90IGJlIGhlbGQuCj4gPiAKPiA+IEEgZHJpdmVyIGNhbm5vdCB0ZWxsIHRoZXNl IHR3byBjYXNlcyBhcGFydC4gIFRoZSBvbmx5IHdheSB0byBtYWtlIGl0Cj4gPiB3b3JrIGFsbCB0 aGUgdGltZSBpcyB0byBoYXZlIHRoZSBjYWxsZXIgX2Fsd2F5c18gaG9sZCB0aGUgZGV2aWNlIGxv Y2sKPiA+IHdoaWxlIHRoZSBkcml2ZXIgaXMgcHJvYmVkIChvciB0aGUgcmVtb3ZlZCwgZm9yIHRo YXQgbWF0dGVyKS4KPiA+IAo+ID4gQWxhbiBTdGVybgo+IAo+IFRoYW5rcyBmb3IgdGhlIHJlcGx5 IGFuZCBtb3JlIGRldGFpbCBhYm91dCB0aGUgYmFja2dyb3VkLiBJJ2QgbGlrZSB0bwo+IGhhdmUg YSBjb25jbHVzaW9uIGFib3V0IGl0LiBQbGVhc2Uga2luZGx5IGNvcnJlY3QgbWUgaWYgbXkgdW5k ZXJzdGFuZGluZwo+IGlzIHdyb25nLiBSZWdhcmRpbmcgdG8gdGhlICJzcGVjaWFsLXB1cnBvc2Ug ZmxhZyIsIGRvIHlvdSBtZWFuIHdlIGNvdWxkCj4gZmluZCBhIHBsYWNlIGluIFVTQiBzdWJzeXN0 ZW0gdG8gaGF2ZSB0aGUgZmxhZyBzZXQgKG5vdCBzdXJlIGlmIGl0J3MKPiBlYXN5IHRvIGZpbmQg aXQpLiBEcml2ZXIgY29yZSB3b3VsZCBiZSBiYXNlIG9uIHRoZSBmbGFnIHRvIGRlY2lkZSBpZiB3 ZQo+IG5lZWQgdG8gaG9sZCB0aGUgZGV2aWNlJ3MgcGFyZW50J3MgbG9jay4KClllcywgZXhjZXB0 IHRoYXQgdGhlIGZsYWcgd291bGQgbm90IGJlIGluIHRoZSBVU0Igc3Vic3lzdGVtLiAgSXQgd291 bGQgCmJlIGluIHRoZSBkZXZpY2UsIGRldmljZV90eXBlLCBvciBidXNfdHlwZSBzdHJ1Y3R1cmUs IHNvIHRoYXQgdGhlIApkcml2ZXIgY29yZSBjb3VsZCBhY2Nlc3MgaXQuCgpBbGFuIFN0ZXJuCi0t LQpUbyB1bnN1YnNjcmliZSBmcm9tIHRoaXMgbGlzdDogc2VuZCB0aGUgbGluZSAidW5zdWJzY3Jp YmUgbGludXgtdXNiIiBpbgp0aGUgYm9keSBvZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZnZXIu a2VybmVsLm9yZwpNb3JlIG1ham9yZG9tbyBpbmZvIGF0ICBodHRwOi8vdmdlci5rZXJuZWwub3Jn L21ham9yZG9tby1pbmZvLmh0bWwK