| Message ID | 20180528152030.GA3261@localhost (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <rogerq@ti.com> > > --- > > 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 <johan@kernel.org> > 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 <johan@kernel.org> > --- > 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; > } -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <johan@kernel.org> > > 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 <johan@kernel.org> > > --- > > 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. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <johan@kernel.org> > > > 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 <johan@kernel.org> > > > --- > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }