Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 31 May 2018 15:25:38 +0200
From:   Johan Hovold <johan@kernel.org>
To:     Roger Quadros <rogerq@ti.com>, 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
Precedence: bulk

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;
>  }