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: <20180528152030.GA3261@localhost> Date: Mon, 28 May 2018 17:41:48 +0200 To: Roger Quadros Cc: balbi@kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org List-ID: T24gTW9uLCBNYXkgMjgsIDIwMTggYXQgMDU6MzY6MTRQTSArMDMwMCwgUm9nZXIgUXVhZHJvcyB3 cm90ZToKPiBEb24ndCBjYWxsIHBtX3J1bnRpbWVfc2V0X2FjdGl2ZSgpIGFzIGl0IHdpbGwgcHJl dmVudCB0aGUgZGV2aWNlCj4gZnJvbSBiZWluZyBhY3RpdmF0ZWQgaW4gdGhlIG5leHQgcG1fcnVu dGltZV9nZXRfc3luYygpIGNhbGwuCj4gCj4gQWxzbyBjYWxsIHBtX3J1bnRpbWVfZ2V0X3N5bmMo KSBiZWZvcmUgb2ZfcGxhdGZvcm1fcG9wdWxhdGUoKS4KClRoaXMgcGFyYWdyYXBoIGRlc2NyaWJl cyB3aGF0IHlvdSBkbywgYnV0IG5vdCB3aHkgZG8gaXQuCgo+IFNpZ25lZC1vZmYtYnk6IFJvZ2Vy IFF1YWRyb3MgPHJvZ2VycUB0aS5jb20+Cj4gLS0tCj4gIGRyaXZlcnMvdXNiL2R3YzMvZHdjMy1v Zi1zaW1wbGUuYyB8IDcgKysrLS0tLQo+ICAxIGZpbGUgY2hhbmdlZCwgMyBpbnNlcnRpb25zKCsp LCA0IGRlbGV0aW9ucygtKQo+IAo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL3VzYi9kd2MzL2R3YzMt b2Ytc2ltcGxlLmMgYi9kcml2ZXJzL3VzYi9kd2MzL2R3YzMtb2Ytc2ltcGxlLmMKPiBpbmRleCBl OThkMjIxLi4yY2JiNWMwIDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMvdXNiL2R3YzMvZHdjMy1vZi1z aW1wbGUuYwo+ICsrKyBiL2RyaXZlcnMvdXNiL2R3YzMvZHdjMy1vZi1zaW1wbGUuYwo+IEBAIC0x MjEsNiArMTIxLDkgQEAgc3RhdGljIGludCBkd2MzX29mX3NpbXBsZV9wcm9iZShzdHJ1Y3QgcGxh dGZvcm1fZGV2aWNlICpwZGV2KQo+ICAJaWYgKHJldCkKPiAgCQlnb3RvIGVycl9yZXNldGNfYXNz ZXJ0Owo+ICAKPiArCXBtX3J1bnRpbWVfZW5hYmxlKGRldik7Cj4gKwlwbV9ydW50aW1lX2dldF9z eW5jKGRldik7CgpUaGlzIGJyZWFrcyBydW50aW1lIHBtIGFzIHlvdSBub3cgZ2V0IGEgc2Vjb25k IHJvdW5kIG9mIGNsb2NrIGVuYWJsZXMKd2hpY2ggYXJlIG5ldmVyIGJhbGFuY2VkIG9uIHJ1bnRp bWUgc3VzcGVuZCAodGhlIGNsb2NrcyBhcmUgZmlyc3QKZW5hYmxlZCBpbiBkd2MzX29mX3NpbXBs ZV9jbGtfaW5pdCgpIGFib3ZlIGFuZCB3aXRoIHlvdXIgY2hhbmdlIGFnYWluIGluCmR3YzNfb2Zf c2ltcGxlX3J1bnRpbWVfcmVzdW1lKCkpLgoKT24gdGhlIG90aGVyIGhhbmQsIHdlIGN1cnJlbnRs eSByZXR1cm4gZnJvbSBwcm9iZSgpIHdpdGggYSBwb3NpdGl2ZSBSUE0KY291bnQgc28gcGVyaGFw cyB0aGUgUlBNIGNhbGxiYWNrcyBjYW4ganVzdCBiZSByZW1vdmVkIGFsdG9nZXRoZXIgKGkuZS4K dW5sZXNzIHNvbWUgb3RoZXIgZW50aXR5IGRyb3BzIHRoYXQgY291bnQgYXQgc29tZSBwb2ludCBi ZWZvcmUKcmVtb3ZlKCkpLgoKPiAgCXJldCA9IG9mX3BsYXRmb3JtX3BvcHVsYXRlKG5wLCBOVUxM LCBOVUxMLCBkZXYpOwo+ICAJaWYgKHJldCkgewo+ICAJCWZvciAoaSA9IDA7IGkgPCBzaW1wbGUt Pm51bV9jbG9ja3M7IGkrKykgewo+IEBAIC0xMzEsMTAgKzEzNCw2IEBAIHN0YXRpYyBpbnQgZHdj M19vZl9zaW1wbGVfcHJvYmUoc3RydWN0IHBsYXRmb3JtX2RldmljZSAqcGRldikKPiAgCQlnb3Rv IGVycl9yZXNldGNfYXNzZXJ0Owo+ICAJfQo+ICAKPiAtCXBtX3J1bnRpbWVfc2V0X2FjdGl2ZShk ZXYpOwo+IC0JcG1fcnVudGltZV9lbmFibGUoZGV2KTsKPiAtCXBtX3J1bnRpbWVfZ2V0X3N5bmMo ZGV2KTsKPiAtCj4gIAlyZXR1cm4gMDsKPiAgCj4gIGVycl9yZXNldGNfYXNzZXJ0OgoKQWxzbyBu b3RlIHRoYXQgdGhlcmUncyBjdXJyZW50bHkgYSB1c2UtYWZ0ZXItZnJlZSBpbiByZW1vdmUoKSwg d2hlcmUKcG1fcnVudGltZV9wdXRfc3luYygpIGlzIGNhbGxlZCBhZnRlciB0aGUgY2xvY2tzIGhh dmUgYmVlbiBwdXQuClNvbWV0aGluZyBsaWtlIHRoZSBiZWxvdyAodW50ZXN0ZWQpIHBhdGNoIHNo b3VsZCBmaXggaXQuCgpKb2hhbgoKCkZyb20gMzVjMzg0YzMxMDEwYzM0NGQ0MDNjMjZmYzBhMWRk ZTBmZDY4ZWY0YSBNb24gU2VwIDE3IDAwOjAwOjAwIDIwMDEKRnJvbTogSm9oYW4gSG92b2xkIDxq b2hhbkBrZXJuZWwub3JnPgpEYXRlOiBNb24sIDI4IE1heSAyMDE4IDE3OjMxOjQ1ICswMjAwClN1 YmplY3Q6IFtQQVRDSF0gdXNiOiBkd2MzOiBvZi1zaW1wbGU6IGZpeCB1c2UtYWZ0ZXItZnJlZSBv biByZW1vdmUKClRoZSBjbG9ja3MgaGF2ZSBhbHJlYWR5IGJlZW4gZXhwbGljaXRseSBkaXNhYmxl ZCBhbmQgcHV0IGFzIHBhcnQgb2YKcmVtb3ZlKCkgc28gdGhlIHJ1bnRpbWUgc3VzcGVuZCBjYWxs YmFjayBtdXN0IG5vdCBiZSBydW4gd2hlbiBiYWxhbmNpbmcKdGhlIHJ1bnRpbWUgUE0gdXNhZ2Ug Y291bnQgYmVmb3JlIHJldHVybmluZy4KCkZpeGVzOiAxNmFkYzY3NGQwZDYgKCJ1c2I6IGR3YzM6 IGFkZCBnZW5lcmljIE9GIGdsdWUgbGF5ZXIiKQpTaWduZWQtb2ZmLWJ5OiBKb2hhbiBIb3ZvbGQg PGpvaGFuQGtlcm5lbC5vcmc+Ci0tLQogZHJpdmVycy91c2IvZHdjMy9kd2MzLW9mLXNpbXBsZS5j IHwgMyArKy0KIDEgZmlsZSBjaGFuZ2VkLCAyIGluc2VydGlvbnMoKyksIDEgZGVsZXRpb24oLSkK CmRpZmYgLS1naXQgYS9kcml2ZXJzL3VzYi9kd2MzL2R3YzMtb2Ytc2ltcGxlLmMgYi9kcml2ZXJz L3VzYi9kd2MzL2R3YzMtb2Ytc2ltcGxlLmMKaW5kZXggY2IyZWU5NmZkM2U4Li5iOWM4NjljZDY1 ODUgMTAwNjQ0Ci0tLSBhL2RyaXZlcnMvdXNiL2R3YzMvZHdjMy1vZi1zaW1wbGUuYworKysgYi9k cml2ZXJzL3VzYi9kd2MzL2R3YzMtb2Ytc2ltcGxlLmMKQEAgLTE2NSw4ICsxNjUsOSBAQCBzdGF0 aWMgaW50IGR3YzNfb2Zfc2ltcGxlX3JlbW92ZShzdHJ1Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2 KQogCiAJcmVzZXRfY29udHJvbF9wdXQoc2ltcGxlLT5yZXNldHMpOwogCi0JcG1fcnVudGltZV9w dXRfc3luYyhkZXYpOworCXBtX3J1bnRpbWVfcHV0X25vaWRsZShkZXYpOwogCXBtX3J1bnRpbWVf ZGlzYWJsZShkZXYpOworCXBtX3J1bnRpbWVfc2V0X3N1c3BlbmRlZChkZXYpOwogCiAJcmV0dXJu IDA7CiB9Cg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1034695AbeE1PmB (ORCPT ); Mon, 28 May 2018 11:42:01 -0400 Received: from mail-wr0-f195.google.com ([209.85.128.195]:44592 "EHLO mail-wr0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965409AbeE1Plv (ORCPT ); Mon, 28 May 2018 11:41:51 -0400 X-Google-Smtp-Source: ADUXVKIRrqsITqC+Ba6c5djv7RRoOaCaH+DqUYNpElz1xgJLt5thwTo9UatL9fDJXqem4yXVCGqj1g== Date: Mon, 28 May 2018 17:41:48 +0200 From: Johan Hovold To: Roger Quadros 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: <20180528152030.GA3261@localhost> References: <1527518174-27860-1-git-send-email-rogerq@ti.com> <1527518174-27860-2-git-send-email-rogerq@ti.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1527518174-27860-2-git-send-email-rogerq@ti.com> 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 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. 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; } -- 2.17.0