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: [2/2] usb: dwc3: of_simple: don't call pm_runtime_set_active() From: Johan Hovold Message-Id: <20180531143703.GL3259@localhost> Date: Thu, 31 May 2018 16:37:03 +0200 To: Alan Stern Cc: Johan Hovold , Roger Quadros , balbi@kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org List-ID: T24gVGh1LCBNYXkgMzEsIDIwMTggYXQgMTA6MDc6MDVBTSAtMDQwMCwgQWxhbiBTdGVybiB3cm90 ZToKPiBPbiBUaHUsIDMxIE1heSAyMDE4LCBKb2hhbiBIb3ZvbGQgd3JvdGU6Cj4gCj4gPiA+IFRo aXMgYnJlYWtzIHJ1bnRpbWUgcG0gYXMgeW91IG5vdyBnZXQgYSBzZWNvbmQgcm91bmQgb2YgY2xv Y2sgZW5hYmxlcwo+ID4gPiB3aGljaCBhcmUgbmV2ZXIgYmFsYW5jZWQgb24gcnVudGltZSBzdXNw ZW5kICh0aGUgY2xvY2tzIGFyZSBmaXJzdAo+ID4gPiBlbmFibGVkIGluIGR3YzNfb2Zfc2ltcGxl X2Nsa19pbml0KCkgYWJvdmUgYW5kIHdpdGggeW91ciBjaGFuZ2UgYWdhaW4gaW4KPiA+ID4gZHdj M19vZl9zaW1wbGVfcnVudGltZV9yZXN1bWUoKSkuCj4gPiA+IAo+ID4gPiBPbiB0aGUgb3RoZXIg aGFuZCwgd2UgY3VycmVudGx5IHJldHVybiBmcm9tIHByb2JlKCkgd2l0aCBhIHBvc2l0aXZlIFJQ TQo+ID4gPiBjb3VudCBzbyBwZXJoYXBzIHRoZSBSUE0gY2FsbGJhY2tzIGNhbiBqdXN0IGJlIHJl bW92ZWQgYWx0b2dldGhlciAoaS5lLgo+ID4gPiB1bmxlc3Mgc29tZSBvdGhlciBlbnRpdHkgZHJv cHMgdGhhdCBjb3VudCBhdCBzb21lIHBvaW50IGJlZm9yZQo+ID4gPiByZW1vdmUoKSkuCj4gPiA+ IAo+ID4gPiA+ICAJcmV0ID0gb2ZfcGxhdGZvcm1fcG9wdWxhdGUobnAsIE5VTEwsIE5VTEwsIGRl dik7Cj4gPiA+ID4gIAlpZiAocmV0KSB7Cj4gPiA+ID4gIAkJZm9yIChpID0gMDsgaSA8IHNpbXBs ZS0+bnVtX2Nsb2NrczsgaSsrKSB7Cj4gPiA+ID4gQEAgLTEzMSwxMCArMTM0LDYgQEAgc3RhdGlj IGludCBkd2MzX29mX3NpbXBsZV9wcm9iZShzdHJ1Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2KQo+ ID4gPiA+ICAJCWdvdG8gZXJyX3Jlc2V0Y19hc3NlcnQ7Cj4gPiA+ID4gIAl9Cj4gPiA+ID4gIAo+ ID4gPiA+IC0JcG1fcnVudGltZV9zZXRfYWN0aXZlKGRldik7Cj4gPiA+ID4gLQlwbV9ydW50aW1l X2VuYWJsZShkZXYpOwo+ID4gPiA+IC0JcG1fcnVudGltZV9nZXRfc3luYyhkZXYpOwo+ID4gPiA+ IC0KPiA+ID4gPiAgCXJldHVybiAwOwo+ID4gPiA+ICAKPiA+ID4gPiAgZXJyX3Jlc2V0Y19hc3Nl cnQ6Cj4gPiA+IAo+ID4gPiBBbHNvIG5vdGUgdGhhdCB0aGVyZSdzIGN1cnJlbnRseSBhIHVzZS1h ZnRlci1mcmVlIGluIHJlbW92ZSgpLCB3aGVyZQo+ID4gPiBwbV9ydW50aW1lX3B1dF9zeW5jKCkg aXMgY2FsbGVkIGFmdGVyIHRoZSBjbG9ja3MgaGF2ZSBiZWVuIHB1dC4KPiA+ID4gU29tZXRoaW5n IGxpa2UgdGhlIGJlbG93ICh1bnRlc3RlZCkgcGF0Y2ggc2hvdWxkIGZpeCBpdC4KPiA+IAo+ID4g V2hhdCBhYm91dCB0aGUgdXNlLWFmdGVyLWZyZWUgaW4gcmVtb3ZlPyBTaGFsbCBJIHJlc3VibWl0 IHRoZSBmaXggYmVsb3cKPiA+IHNlcGFyYXRlbHk/Cj4gPiAKPiA+IFRoYW5rcywKPiA+IEpvaGFu Cj4gPiAKPiA+ID4gRnJvbSAzNWMzODRjMzEwMTBjMzQ0ZDQwM2MyNmZjMGExZGRlMGZkNjhlZjRh IE1vbiBTZXAgMTcgMDA6MDA6MDAgMjAwMQo+ID4gPiBGcm9tOiBKb2hhbiBIb3ZvbGQgPGpvaGFu QGtlcm5lbC5vcmc+Cj4gPiA+IERhdGU6IE1vbiwgMjggTWF5IDIwMTggMTc6MzE6NDUgKzAyMDAK PiA+ID4gU3ViamVjdDogW1BBVENIXSB1c2I6IGR3YzM6IG9mLXNpbXBsZTogZml4IHVzZS1hZnRl ci1mcmVlIG9uIHJlbW92ZQo+ID4gPiAKPiA+ID4gVGhlIGNsb2NrcyBoYXZlIGFscmVhZHkgYmVl biBleHBsaWNpdGx5IGRpc2FibGVkIGFuZCBwdXQgYXMgcGFydCBvZgo+ID4gPiByZW1vdmUoKSBz byB0aGUgcnVudGltZSBzdXNwZW5kIGNhbGxiYWNrIG11c3Qgbm90IGJlIHJ1biB3aGVuIGJhbGFu Y2luZwo+ID4gPiB0aGUgcnVudGltZSBQTSB1c2FnZSBjb3VudCBiZWZvcmUgcmV0dXJuaW5nLgo+ ID4gPiAKPiA+ID4gRml4ZXM6IDE2YWRjNjc0ZDBkNiAoInVzYjogZHdjMzogYWRkIGdlbmVyaWMg T0YgZ2x1ZSBsYXllciIpCj4gPiA+IFNpZ25lZC1vZmYtYnk6IEpvaGFuIEhvdm9sZCA8am9oYW5A a2VybmVsLm9yZz4KPiA+ID4gLS0tCj4gPiA+ICBkcml2ZXJzL3VzYi9kd2MzL2R3YzMtb2Ytc2lt cGxlLmMgfCAzICsrLQo+ID4gPiAgMSBmaWxlIGNoYW5nZWQsIDIgaW5zZXJ0aW9ucygrKSwgMSBk ZWxldGlvbigtKQo+ID4gPiAKPiA+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvdXNiL2R3YzMvZHdj My1vZi1zaW1wbGUuYyBiL2RyaXZlcnMvdXNiL2R3YzMvZHdjMy1vZi1zaW1wbGUuYwo+ID4gPiBp bmRleCBjYjJlZTk2ZmQzZTguLmI5Yzg2OWNkNjU4NSAxMDA2NDQKPiA+ID4gLS0tIGEvZHJpdmVy cy91c2IvZHdjMy9kd2MzLW9mLXNpbXBsZS5jCj4gPiA+ICsrKyBiL2RyaXZlcnMvdXNiL2R3YzMv ZHdjMy1vZi1zaW1wbGUuYwo+ID4gPiBAQCAtMTY1LDggKzE2NSw5IEBAIHN0YXRpYyBpbnQgZHdj M19vZl9zaW1wbGVfcmVtb3ZlKHN0cnVjdCBwbGF0Zm9ybV9kZXZpY2UgKnBkZXYpCj4gPiA+ICAK PiA+ID4gIAlyZXNldF9jb250cm9sX3B1dChzaW1wbGUtPnJlc2V0cyk7Cj4gPiA+ICAKPiA+ID4g LQlwbV9ydW50aW1lX3B1dF9zeW5jKGRldik7Cj4gPiA+ICsJcG1fcnVudGltZV9wdXRfbm9pZGxl KGRldik7Cj4gPiA+ICAJcG1fcnVudGltZV9kaXNhYmxlKGRldik7Cj4gPiA+ICsJcG1fcnVudGlt ZV9zZXRfc3VzcGVuZGVkKGRldik7Cj4gPiA+ICAKPiA+ID4gIAlyZXR1cm4gMDsKPiA+ID4gIH0K PiAKPiBUaGlzIGlzIGEgbGl0dGxlIHJhY3kgLS0gdGhlcmUgbWlnaHQgYmUgYSBydW50aW1lLXN1 c3BlbmQgY2FsbGJhY2sKPiBiZXR3ZWVuIHRoZSBwdXRfbm9pZGxlIGFuZCB0aGUgZGlzYWJsZS4g IChUaGUgcHV0X25vaWRsZSBpdHNlbGYgd29uJ3QKPiBjYXVzZSBhIGNhbGxiYWNrIHRvIGhhcHBl biwgYnV0IHNvbWV0aGluZyBlbHNlIGNvdWxkLikKPiAKPiBJdCB3b3VsZCBiZSBiZXR0ZXIgdG8g ZG8gdGhlIGRpc2FibGUgZmlyc3QgYW5kIHRoZW4gdGhlIHB1dF9ub2lkbGUuCgpHb29kIHBvaW50 LiBJJ2xsIHNlbmQgYSB2MiBmb3IgRmVsaXBlIHRvIGNvbnNpZGVyLgoKVGhhbmtzLApKb2hhbgot LS0KVG8gdW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2Ny aWJlIGxpbnV4LXVzYiIgaW4KdGhlIGJvZHkgb2YgYSBtZXNzYWdlIHRvIG1ham9yZG9tb0B2Z2Vy Lmtlcm5lbC5vcmcKTW9yZSBtYWpvcmRvbW8gaW5mbyBhdCAgaHR0cDovL3ZnZXIua2VybmVsLm9y Zy9tYWpvcmRvbW8taW5mby5odG1sCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755406AbeEaOhN (ORCPT ); Thu, 31 May 2018 10:37:13 -0400 Received: from mail-lf0-f65.google.com ([209.85.215.65]:44976 "EHLO mail-lf0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755302AbeEaOhI (ORCPT ); Thu, 31 May 2018 10:37:08 -0400 X-Google-Smtp-Source: ADUXVKLNtvqKR5D1UcJ8pOR36URKtuNBzoOI2erVkrTQSde7B9J1WQL0EN47mN25q/jbdTiTjj3Iyw== Date: Thu, 31 May 2018 16:37:03 +0200 From: Johan Hovold To: Alan Stern Cc: Johan Hovold , Roger Quadros , balbi@kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] usb: dwc3: of_simple: don't call pm_runtime_set_active() Message-ID: <20180531143703.GL3259@localhost> References: <20180531132538.GH3259@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.0 (2018-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 31, 2018 at 10:07:05AM -0400, Alan Stern wrote: > On Thu, 31 May 2018, Johan Hovold wrote: > > > > This breaks runtime pm as you now get a second round of clock enables > > > which are never balanced on runtime suspend (the clocks are first > > > enabled in dwc3_of_simple_clk_init() above and with your change again in > > > dwc3_of_simple_runtime_resume()). > > > > > > On the other hand, we currently return from probe() with a positive RPM > > > count so perhaps the RPM callbacks can just be removed altogether (i.e. > > > unless some other entity drops that count at some point before > > > remove()). > > > > > > > ret = of_platform_populate(np, NULL, NULL, dev); > > > > if (ret) { > > > > for (i = 0; i < simple->num_clocks; i++) { > > > > @@ -131,10 +134,6 @@ static int dwc3_of_simple_probe(struct platform_device *pdev) > > > > goto err_resetc_assert; > > > > } > > > > > > > > - pm_runtime_set_active(dev); > > > > - pm_runtime_enable(dev); > > > > - pm_runtime_get_sync(dev); > > > > - > > > > return 0; > > > > > > > > err_resetc_assert: > > > > > > Also note that there's currently a use-after-free in remove(), where > > > pm_runtime_put_sync() is called after the clocks have been put. > > > Something like the below (untested) patch should fix it. > > > > What about the use-after-free in remove? Shall I resubmit the fix below > > separately? > > > > Thanks, > > Johan > > > > > From 35c384c31010c344d403c26fc0a1dde0fd68ef4a Mon Sep 17 00:00:00 2001 > > > From: Johan Hovold > > > Date: Mon, 28 May 2018 17:31:45 +0200 > > > Subject: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove > > > > > > The clocks have already been explicitly disabled and put as part of > > > remove() so the runtime suspend callback must not be run when balancing > > > the runtime PM usage count before returning. > > > > > > Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer") > > > Signed-off-by: Johan Hovold > > > --- > > > drivers/usb/dwc3/dwc3-of-simple.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c > > > index cb2ee96fd3e8..b9c869cd6585 100644 > > > --- a/drivers/usb/dwc3/dwc3-of-simple.c > > > +++ b/drivers/usb/dwc3/dwc3-of-simple.c > > > @@ -165,8 +165,9 @@ static int dwc3_of_simple_remove(struct platform_device *pdev) > > > > > > reset_control_put(simple->resets); > > > > > > - pm_runtime_put_sync(dev); > > > + pm_runtime_put_noidle(dev); > > > pm_runtime_disable(dev); > > > + pm_runtime_set_suspended(dev); > > > > > > return 0; > > > } > > This is a little racy -- there might be a runtime-suspend callback > between the put_noidle and the disable. (The put_noidle itself won't > cause a callback to happen, but something else could.) > > It would be better to do the disable first and then the put_noidle. Good point. I'll send a v2 for Felipe to consider. Thanks, Johan