Re: [PATCH 1/9] drm/nouveau: Don't leak runtime pm ref on driver unload

[Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>]

Hi Peter,

On Fri, May 27, 2016 at 03:07:33AM +0200, Peter Wu wrote:
> On Tue, May 24, 2016 at 06:03:27PM +0200, Lukas Wunner wrote:
> > nouveau_drm_load() calls pm_runtime_put() if nouveau_runtime_pm != 0,
> > but nouveau_drm_unload() calls pm_runtime_get_sync() unconditionally.
> > We therefore leak a runtime pm ref whenever nouveau is loaded with
> > runpm=0 and then unloaded. The GPU will subsequently never runtime
> > suspend even if nouveau is loaded again with runpm=1.
> > 
> > Fix by taking the runtime pm ref under the same condition that it was
> > released on driver load.
> > 
> > Fixes: 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)")
> > Cc: Dave Airlie <airlied@redhat.com>
> > Reported-by: Karol Herbst <nouveau@karolherbst.de>
> > Tested-by: Karol Herbst <nouveau@karolherbst.de>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> 
> Looks good, I tested this scenario:
> 
>     ru(){ cat /sys/bus/pci/devices/0000\:01:00.0/power/runtime_usage;}
>     ru # reports 1
>     modprobe nouveau runpm=0
>     ru # reports 2
>     rmmod nouveau
>     ru # reports 1
> 
> Without runpm=0 the count drops to 0 in the second step and stays 0 in
> the third step. After applying patch 2/9, this correctly reports 1 as
> expected (this is the same as manually setting power/control to on).

How exactly did you reach the situation where the root port didn't wake
up when you tried to load nouveau again? (IRC conversation this week.)

What's happening is, the PCI core will keep unbound devices (i.e.,
without driver) in D0 but the runtime status is allowed to change
to "suspended". So it'll appear to the kernel as if it was suspended
but in reality it stays in D0.

Once runtime pm for PCIe ports gets merged, the root port above the
GPU will indeed go to D3 in such a situation because the check
pm_children_suspended() (called from rpm_check_suspend_allowed())
returns true.

I'm not sure if this is desirable or not. If we keep unbound devices
in D0, should we allow ports above them to go to D3?

In any case, when nouveau is loaded again, local_pci_probe() will
call pm_runtime_get_sync(), which will implicitly set the runtime
status to "active" and which should also wake parents. So how did
you ever reach a point where you loaded nouveau and the root port
stayed asleep? Clearly we have a bug there, question is where.
This shouldn't work only if pm_runtime_forbid() was called on
driver unload.

Thanks for the extensive testing!

Lukas

> 
> Peter
> 
> > ---
> >  drivers/gpu/drm/nouveau/nouveau_drm.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > index 11f8dd9..faf7438 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > @@ -498,7 +498,10 @@ nouveau_drm_unload(struct drm_device *dev)
> >  {
> >  	struct nouveau_drm *drm = nouveau_drm(dev);
> >  
> > -	pm_runtime_get_sync(dev->dev);
> > +	if (nouveau_runtime_pm != 0) {
> > +		pm_runtime_get_sync(dev->dev);
> > +	}
> > +
> >  	nouveau_fbcon_fini(dev);
> >  	nouveau_accel_fini(drm);
> >  	nouveau_hwmon_fini(dev);
> > -- 
> > 2.8.1
> > 
> > _______________________________________________
> > Nouveau mailing list
> > Nouveau@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/nouveau
> 
> -- 
> Kind regards,
> Peter Wu
> https://lekensteyn.nl
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau