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: <20180531132538.GH3259@localhost> Date: Thu, 31 May 2018 15:25:38 +0200 To: Roger Quadros , balbi@kernel.org Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org List-ID: SGkgRmVsaXBlLAoKT24gTW9uLCBNYXkgMjgsIDIwMTggYXQgMDU6NDE6NDhQTSArMDIwMCwgSm9o YW4gSG92b2xkIHdyb3RlOgo+IE9uIE1vbiwgTWF5IDI4LCAyMDE4IGF0IDA1OjM2OjE0UE0gKzAz MDAsIFJvZ2VyIFF1YWRyb3Mgd3JvdGU6Cj4gPiBEb24ndCBjYWxsIHBtX3J1bnRpbWVfc2V0X2Fj dGl2ZSgpIGFzIGl0IHdpbGwgcHJldmVudCB0aGUgZGV2aWNlCj4gPiBmcm9tIGJlaW5nIGFjdGl2 YXRlZCBpbiB0aGUgbmV4dCBwbV9ydW50aW1lX2dldF9zeW5jKCkgY2FsbC4KPiA+IAo+ID4gQWxz byBjYWxsIHBtX3J1bnRpbWVfZ2V0X3N5bmMoKSBiZWZvcmUgb2ZfcGxhdGZvcm1fcG9wdWxhdGUo KS4KPiAKPiBUaGlzIHBhcmFncmFwaCBkZXNjcmliZXMgd2hhdCB5b3UgZG8sIGJ1dCBub3Qgd2h5 IGRvIGl0Lgo+IAo+ID4gU2lnbmVkLW9mZi1ieTogUm9nZXIgUXVhZHJvcyA8cm9nZXJxQHRpLmNv bT4KPiA+IC0tLQo+ID4gIGRyaXZlcnMvdXNiL2R3YzMvZHdjMy1vZi1zaW1wbGUuYyB8IDcgKysr LS0tLQo+ID4gIDEgZmlsZSBjaGFuZ2VkLCAzIGluc2VydGlvbnMoKyksIDQgZGVsZXRpb25zKC0p Cj4gPiAKPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL3VzYi9kd2MzL2R3YzMtb2Ytc2ltcGxlLmMg Yi9kcml2ZXJzL3VzYi9kd2MzL2R3YzMtb2Ytc2ltcGxlLmMKPiA+IGluZGV4IGU5OGQyMjEuLjJj YmI1YzAgMTAwNjQ0Cj4gPiAtLS0gYS9kcml2ZXJzL3VzYi9kd2MzL2R3YzMtb2Ytc2ltcGxlLmMK PiA+ICsrKyBiL2RyaXZlcnMvdXNiL2R3YzMvZHdjMy1vZi1zaW1wbGUuYwo+ID4gQEAgLTEyMSw2 ICsxMjEsOSBAQCBzdGF0aWMgaW50IGR3YzNfb2Zfc2ltcGxlX3Byb2JlKHN0cnVjdCBwbGF0Zm9y bV9kZXZpY2UgKnBkZXYpCj4gPiAgCWlmIChyZXQpCj4gPiAgCQlnb3RvIGVycl9yZXNldGNfYXNz ZXJ0Owo+ID4gIAo+ID4gKwlwbV9ydW50aW1lX2VuYWJsZShkZXYpOwo+ID4gKwlwbV9ydW50aW1l X2dldF9zeW5jKGRldik7Cj4gCj4gVGhpcyBicmVha3MgcnVudGltZSBwbSBhcyB5b3Ugbm93IGdl dCBhIHNlY29uZCByb3VuZCBvZiBjbG9jayBlbmFibGVzCj4gd2hpY2ggYXJlIG5ldmVyIGJhbGFu Y2VkIG9uIHJ1bnRpbWUgc3VzcGVuZCAodGhlIGNsb2NrcyBhcmUgZmlyc3QKPiBlbmFibGVkIGlu IGR3YzNfb2Zfc2ltcGxlX2Nsa19pbml0KCkgYWJvdmUgYW5kIHdpdGggeW91ciBjaGFuZ2UgYWdh aW4gaW4KPiBkd2MzX29mX3NpbXBsZV9ydW50aW1lX3Jlc3VtZSgpKS4KPiAKPiBPbiB0aGUgb3Ro ZXIgaGFuZCwgd2UgY3VycmVudGx5IHJldHVybiBmcm9tIHByb2JlKCkgd2l0aCBhIHBvc2l0aXZl IFJQTQo+IGNvdW50IHNvIHBlcmhhcHMgdGhlIFJQTSBjYWxsYmFja3MgY2FuIGp1c3QgYmUgcmVt b3ZlZCBhbHRvZ2V0aGVyIChpLmUuCj4gdW5sZXNzIHNvbWUgb3RoZXIgZW50aXR5IGRyb3BzIHRo YXQgY291bnQgYXQgc29tZSBwb2ludCBiZWZvcmUKPiByZW1vdmUoKSkuCj4gCj4gPiAgCXJldCA9 IG9mX3BsYXRmb3JtX3BvcHVsYXRlKG5wLCBOVUxMLCBOVUxMLCBkZXYpOwo+ID4gIAlpZiAocmV0 KSB7Cj4gPiAgCQlmb3IgKGkgPSAwOyBpIDwgc2ltcGxlLT5udW1fY2xvY2tzOyBpKyspIHsKPiA+ IEBAIC0xMzEsMTAgKzEzNCw2IEBAIHN0YXRpYyBpbnQgZHdjM19vZl9zaW1wbGVfcHJvYmUoc3Ry dWN0IHBsYXRmb3JtX2RldmljZSAqcGRldikKPiA+ICAJCWdvdG8gZXJyX3Jlc2V0Y19hc3NlcnQ7 Cj4gPiAgCX0KPiA+ICAKPiA+IC0JcG1fcnVudGltZV9zZXRfYWN0aXZlKGRldik7Cj4gPiAtCXBt X3J1bnRpbWVfZW5hYmxlKGRldik7Cj4gPiAtCXBtX3J1bnRpbWVfZ2V0X3N5bmMoZGV2KTsKPiA+ IC0KPiA+ICAJcmV0dXJuIDA7Cj4gPiAgCj4gPiAgZXJyX3Jlc2V0Y19hc3NlcnQ6Cj4gCj4gQWxz byBub3RlIHRoYXQgdGhlcmUncyBjdXJyZW50bHkgYSB1c2UtYWZ0ZXItZnJlZSBpbiByZW1vdmUo KSwgd2hlcmUKPiBwbV9ydW50aW1lX3B1dF9zeW5jKCkgaXMgY2FsbGVkIGFmdGVyIHRoZSBjbG9j a3MgaGF2ZSBiZWVuIHB1dC4KPiBTb21ldGhpbmcgbGlrZSB0aGUgYmVsb3cgKHVudGVzdGVkKSBw YXRjaCBzaG91bGQgZml4IGl0LgoKV2hhdCBhYm91dCB0aGUgdXNlLWFmdGVyLWZyZWUgaW4gcmVt b3ZlPyBTaGFsbCBJIHJlc3VibWl0IHRoZSBmaXggYmVsb3cKc2VwYXJhdGVseT8KClRoYW5rcywK Sm9oYW4KCj4gRnJvbSAzNWMzODRjMzEwMTBjMzQ0ZDQwM2MyNmZjMGExZGRlMGZkNjhlZjRhIE1v biBTZXAgMTcgMDA6MDA6MDAgMjAwMQo+IEZyb206IEpvaGFuIEhvdm9sZCA8am9oYW5Aa2VybmVs Lm9yZz4KPiBEYXRlOiBNb24sIDI4IE1heSAyMDE4IDE3OjMxOjQ1ICswMjAwCj4gU3ViamVjdDog W1BBVENIXSB1c2I6IGR3YzM6IG9mLXNpbXBsZTogZml4IHVzZS1hZnRlci1mcmVlIG9uIHJlbW92 ZQo+IAo+IFRoZSBjbG9ja3MgaGF2ZSBhbHJlYWR5IGJlZW4gZXhwbGljaXRseSBkaXNhYmxlZCBh bmQgcHV0IGFzIHBhcnQgb2YKPiByZW1vdmUoKSBzbyB0aGUgcnVudGltZSBzdXNwZW5kIGNhbGxi YWNrIG11c3Qgbm90IGJlIHJ1biB3aGVuIGJhbGFuY2luZwo+IHRoZSBydW50aW1lIFBNIHVzYWdl IGNvdW50IGJlZm9yZSByZXR1cm5pbmcuCj4gCj4gRml4ZXM6IDE2YWRjNjc0ZDBkNiAoInVzYjog ZHdjMzogYWRkIGdlbmVyaWMgT0YgZ2x1ZSBsYXllciIpCj4gU2lnbmVkLW9mZi1ieTogSm9oYW4g SG92b2xkIDxqb2hhbkBrZXJuZWwub3JnPgo+IC0tLQo+ICBkcml2ZXJzL3VzYi9kd2MzL2R3YzMt b2Ytc2ltcGxlLmMgfCAzICsrLQo+ICAxIGZpbGUgY2hhbmdlZCwgMiBpbnNlcnRpb25zKCspLCAx IGRlbGV0aW9uKC0pCj4gCj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvdXNiL2R3YzMvZHdjMy1vZi1z aW1wbGUuYyBiL2RyaXZlcnMvdXNiL2R3YzMvZHdjMy1vZi1zaW1wbGUuYwo+IGluZGV4IGNiMmVl OTZmZDNlOC4uYjljODY5Y2Q2NTg1IDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMvdXNiL2R3YzMvZHdj My1vZi1zaW1wbGUuYwo+ICsrKyBiL2RyaXZlcnMvdXNiL2R3YzMvZHdjMy1vZi1zaW1wbGUuYwo+ IEBAIC0xNjUsOCArMTY1LDkgQEAgc3RhdGljIGludCBkd2MzX29mX3NpbXBsZV9yZW1vdmUoc3Ry dWN0IHBsYXRmb3JtX2RldmljZSAqcGRldikKPiAgCj4gIAlyZXNldF9jb250cm9sX3B1dChzaW1w bGUtPnJlc2V0cyk7Cj4gIAo+IC0JcG1fcnVudGltZV9wdXRfc3luYyhkZXYpOwo+ICsJcG1fcnVu dGltZV9wdXRfbm9pZGxlKGRldik7Cj4gIAlwbV9ydW50aW1lX2Rpc2FibGUoZGV2KTsKPiArCXBt X3J1bnRpbWVfc2V0X3N1c3BlbmRlZChkZXYpOwo+ICAKPiAgCXJldHVybiAwOwo+ICB9Ci0tLQpU byB1bnN1YnNjcmliZSBmcm9tIHRoaXMgbGlzdDogc2VuZCB0aGUgbGluZSAidW5zdWJzY3JpYmUg bGludXgtdXNiIiBpbgp0aGUgYm9keSBvZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZnZXIua2Vy bmVsLm9yZwpNb3JlIG1ham9yZG9tbyBpbmZvIGF0ICBodHRwOi8vdmdlci5rZXJuZWwub3JnL21h am9yZG9tby1pbmZvLmh0bWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755144AbeEaNZp (ORCPT ); Thu, 31 May 2018 09:25:45 -0400 Received: from mail-lf0-f66.google.com ([209.85.215.66]:38092 "EHLO mail-lf0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754820AbeEaNZn (ORCPT ); Thu, 31 May 2018 09:25:43 -0400 X-Google-Smtp-Source: ADUXVKLiPj6QDYM24E9n+nqrQybIJfmdqJXADjiCriV/gAm9lZ9HYyv5xtwR7BkmjKILX/EJ9Si+FQ== Date: Thu, 31 May 2018 15:25:38 +0200 From: Johan Hovold To: Roger Quadros , balbi@kernel.org Cc: 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: <20180531132538.GH3259@localhost> References: <1527518174-27860-1-git-send-email-rogerq@ti.com> <1527518174-27860-2-git-send-email-rogerq@ti.com> <20180528152030.GA3261@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180528152030.GA3261@localhost> 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 Hi Felipe, On Mon, May 28, 2018 at 05:41:48PM +0200, Johan Hovold wrote: > On Mon, May 28, 2018 at 05:36:14PM +0300, Roger Quadros wrote: > > Don't call pm_runtime_set_active() as it will prevent the device > > from being activated in the next pm_runtime_get_sync() call. > > > > Also call pm_runtime_get_sync() before of_platform_populate(). > > This paragraph describes what you do, but not why do it. > > > Signed-off-by: Roger Quadros > > --- > > drivers/usb/dwc3/dwc3-of-simple.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c > > index e98d221..2cbb5c0 100644 > > --- a/drivers/usb/dwc3/dwc3-of-simple.c > > +++ b/drivers/usb/dwc3/dwc3-of-simple.c > > @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev) > > if (ret) > > goto err_resetc_assert; > > > > + pm_runtime_enable(dev); > > + pm_runtime_get_sync(dev); > > 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; > }