Nouveau - Jul 2018 - [PATCH 1/5] drm/nouveau: Prevent RPM callback recursion in suspend/resume paths

On Tue, Jul 17, 2018 at 12:53:11PM -0400, Lyude Paul
wrote:
> On Tue, 2018-07-17 at 09:16 +0200, Lukas Wunner wrote:
> > On Mon, Jul 16, 2018 at 07:59:25PM -0400, Lyude Paul wrote:
> > > In order to fix all of the spots that need to have runtime PM
get/puts()
> > > added, we need to ensure that it's possible for us to call
> > > pm_runtime_get/put() in any context, regardless of how deep,
since
> > > almost all of the spots that are currently missing refs can
potentially
> > > get called in the runtime suspend/resume path. Otherwise,
we'll try to
> > > resume the GPU as we're trying to resume the GPU (and
vice-versa) and
> > > cause the kernel to deadlock.
> > > 
> > > With this, it should be safe to call the pm runtime functions in
any
> > > context in nouveau with one condition: any point in the driver
that
> > > calls pm_runtime_get*() cannot hold any locks owned by nouveau
that
> > > would be acquired anywhere inside nouveau_pmops_runtime_resume().
> > > This includes modesetting locks, i2c bus locks, etc.
> > 
> > [snip]
> > > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > @@ -835,6 +835,8 @@ nouveau_pmops_runtime_suspend(struct device
*dev)
> > >  		return -EBUSY;
> > >  	}
> > >  
> > > +	dev->power.disable_depth++;
> > > +
> > 
> > Anyway, if I understand the commit message correctly, you're
hitting a
> > pm_runtime_get_sync() in a code path that itself is called during a
> > pm_runtime_get_sync().  Could you include stack traces in the commit
> > message?  My gut feeling is that this patch masks a deeper issue,
> > e.g. if the runtime_resume code path does in fact directly poll
outputs,
> > that would seem wrong.  Runtime resume should merely make the card
> > accessible, i.e. reinstate power if necessary, put into PCI_D0,
> > restore registers, etc.  Output polling should be scheduled
> > asynchronously.
> 
> So: the reason that patch was added was mainly for the patches later in the
> series that add guards around the i2c bus and aux bus, since both of those
> require that the device be awake for it to work. Currently, the spot where
it
> would recurse is:
Okay, the PCI device is suspending and the nvkm_i2c_aux_acquire()
wants it in resumed state, so is waiting forever for the device to
runtime suspend in order to resume it again immediately afterwards.

The deadlock in the stack trace you've posted could be resolved using
the technique I used in d61a5c106351 by adding the following to
include/linux/pm_runtime.h:

static inline bool pm_runtime_status_suspending(struct device *dev)
{
	return dev->power.runtime_status == RPM_SUSPENDING;
}

static inline bool is_pm_work(struct device *dev)
{
	struct work_struct *work = current_work();

	return work && work->func == dev->power.work;
}

Then adding this to nvkm_i2c_aux_acquire():

	struct device *dev = pad->i2c->subdev.device->dev;

	if (!(is_pm_work(dev) && pm_runtime_status_suspending(dev))) {
		ret = pm_runtime_get_sync(dev);
		if (ret < 0 && ret != -EACCES)
			return ret;
	}

But here's the catch:  This only works for an *async* runtime suspend.
It doesn't work for pm_runtime_put_sync(), pm_runtime_suspend() etc,
because then the runtime suspend is executed in the context of the caller,
not in the context of dev->power.work.

So it's not a full solution, but hopefully something that gets you
going.  I'm not really familiar with the code paths leading to
nvkm_i2c_aux_acquire() to come up with a full solution off the top
of my head I'm afraid.

Note, it's not sufficient to just check pm_runtime_status_suspending(dev)
because if the runtime_suspend is carried out concurrently by something
else, this will return true but it's not guaranteed that the device is
actually kept awake until the i2c communication has been fully performed.

HTH,

Lukas

On Tue, 2018-07-17 at 20:20 +0200, Lukas Wunner wrote:
> On Tue, Jul 17, 2018 at 12:53:11PM -0400, Lyude Paul wrote:
> > On Tue, 2018-07-17 at 09:16 +0200, Lukas Wunner wrote:
> > > On Mon, Jul 16, 2018 at 07:59:25PM -0400, Lyude Paul wrote:
> > > > In order to fix all of the spots that need to have runtime
PM get/puts()
> > > > added, we need to ensure that it's possible for us to
call
> > > > pm_runtime_get/put() in any context, regardless of how deep,
since
> > > > almost all of the spots that are currently missing refs can
potentially
> > > > get called in the runtime suspend/resume path. Otherwise,
we'll try to
> > > > resume the GPU as we're trying to resume the GPU (and
vice-versa) and
> > > > cause the kernel to deadlock.
> > > > 
> > > > With this, it should be safe to call the pm runtime
functions in any
> > > > context in nouveau with one condition: any point in the
driver that
> > > > calls pm_runtime_get*() cannot hold any locks owned by
nouveau that
> > > > would be acquired anywhere inside
nouveau_pmops_runtime_resume().
> > > > This includes modesetting locks, i2c bus locks, etc.
> > > 
> > > [snip]
> > > > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > > @@ -835,6 +835,8 @@ nouveau_pmops_runtime_suspend(struct
device *dev)
> > > >  		return -EBUSY;
> > > >  	}
> > > >  
> > > > +	dev->power.disable_depth++;
> > > > +
> > > 
> > > Anyway, if I understand the commit message correctly, you're
hitting a
> > > pm_runtime_get_sync() in a code path that itself is called during
a
> > > pm_runtime_get_sync().  Could you include stack traces in the
commit
> > > message?  My gut feeling is that this patch masks a deeper issue,
> > > e.g. if the runtime_resume code path does in fact directly poll
outputs,
> > > that would seem wrong.  Runtime resume should merely make the
card
> > > accessible, i.e. reinstate power if necessary, put into PCI_D0,
> > > restore registers, etc.  Output polling should be scheduled
> > > asynchronously.
> > 
> > So: the reason that patch was added was mainly for the patches later
in the
> > series that add guards around the i2c bus and aux bus, since both of
those
> > require that the device be awake for it to work. Currently, the spot
where
> > it
> > would recurse is:
> 
> Okay, the PCI device is suspending and the nvkm_i2c_aux_acquire()
> wants it in resumed state, so is waiting forever for the device to
> runtime suspend in order to resume it again immediately afterwards.
> 
> The deadlock in the stack trace you've posted could be resolved using
> the technique I used in d61a5c106351 by adding the following to
> include/linux/pm_runtime.h:
> 
> static inline bool pm_runtime_status_suspending(struct device *dev)
> {
> 	return dev->power.runtime_status == RPM_SUSPENDING;
> }
> 
> static inline bool is_pm_work(struct device *dev)
> {
> 	struct work_struct *work = current_work();
> 
> 	return work && work->func == dev->power.work;
> }
> 
> Then adding this to nvkm_i2c_aux_acquire():
> 
> 	struct device *dev = pad->i2c->subdev.device->dev;
> 
> 	if (!(is_pm_work(dev) && pm_runtime_status_suspending(dev))) {
> 		ret = pm_runtime_get_sync(dev);
> 		if (ret < 0 && ret != -EACCES)
> 			return ret;
> 	}
> 
> But here's the catch:  This only works for an *async* runtime suspend.
> It doesn't work for pm_runtime_put_sync(), pm_runtime_suspend() etc,
> because then the runtime suspend is executed in the context of the caller,
> not in the context of dev->power.work.
> 
> So it's not a full solution, but hopefully something that gets you
> going.  I'm not really familiar with the code paths leading to
> nvkm_i2c_aux_acquire() to come up with a full solution off the top
> of my head I'm afraid.
OK-I was considering doing something similar to that commit beforehand but I
wasn't sure if I was going to just be hacking around an actual issue. That
doesn't seem to be the case. This is very helpful and hopefully I should be
able
to figure something out from this, thanks!
> 
> Note, it's not sufficient to just check
pm_runtime_status_suspending(dev)
> because if the runtime_suspend is carried out concurrently by something
> else, this will return true but it's not guaranteed that the device is
> actually kept awake until the i2c communication has been fully performed.
> 
> HTH,
> 
> Lukas

On Tue, Jul 17, 2018 at 02:24:31PM -0400, Lyude Paul
wrote:
> On Tue, 2018-07-17 at 20:20 +0200, Lukas Wunner wrote:
> > Okay, the PCI device is suspending and the nvkm_i2c_aux_acquire()
> > wants it in resumed state, so is waiting forever for the device to
> > runtime suspend in order to resume it again immediately afterwards.
> > 
> > The deadlock in the stack trace you've posted could be resolved
using
> > the technique I used in d61a5c106351 by adding the following to
> > include/linux/pm_runtime.h:
> > 
> > static inline bool pm_runtime_status_suspending(struct device *dev)
> > {
> > 	return dev->power.runtime_status == RPM_SUSPENDING;
> > }
> > 
> > static inline bool is_pm_work(struct device *dev)
> > {
> > 	struct work_struct *work = current_work();
> > 
> > 	return work && work->func == dev->power.work;
> > }
> > 
> > Then adding this to nvkm_i2c_aux_acquire():
> > 
> > 	struct device *dev = pad->i2c->subdev.device->dev;
> > 
> > 	if (!(is_pm_work(dev) && pm_runtime_status_suspending(dev)))
{
> > 		ret = pm_runtime_get_sync(dev);
> > 		if (ret < 0 && ret != -EACCES)
> > 			return ret;
> > 	}
> > 
> > But here's the catch:  This only works for an *async* runtime
suspend.
> > It doesn't work for pm_runtime_put_sync(), pm_runtime_suspend()
etc,
> > because then the runtime suspend is executed in the context of the
caller,
> > not in the context of dev->power.work.
> > 
> > So it's not a full solution, but hopefully something that gets you
> > going.  I'm not really familiar with the code paths leading to
> > nvkm_i2c_aux_acquire() to come up with a full solution off the top
> > of my head I'm afraid.
> 
> OK-I was considering doing something similar to that commit beforehand but
I
> wasn't sure if I was going to just be hacking around an actual issue.
That
> doesn't seem to be the case. This is very helpful and hopefully I
should be able
> to figure something out from this, thanks!
In some cases, the function acquiring the runtime PM ref is only called
from a couple of places and then it would be feasible and appropriate
to add a bool parameter to the function telling it to acquire the ref
or not.  So the function is told using a parameter which context it's
running in:  In the runtime_suspend code path or some other code path.

The technique to use current_work() is an alternative approach to figure
out the context if passing in an additional parameter is not feasible
for some reason.  That was the case with d61a5c106351.  That approach
only works for work items though.

Lukas

On Tue, Jul 17, 2018 at 8:20 PM, Lukas Wunner <lukas at wunner.de>
wrote:
> On Tue, Jul 17, 2018 at 12:53:11PM -0400, Lyude Paul wrote:
>> On Tue, 2018-07-17 at 09:16 +0200, Lukas Wunner wrote:
>> > On Mon, Jul 16, 2018 at 07:59:25PM -0400, Lyude Paul wrote:
>> > > In order to fix all of the spots that need to have runtime PM
get/puts()
>> > > added, we need to ensure that it's possible for us to
call
>> > > pm_runtime_get/put() in any context, regardless of how deep,
since
>> > > almost all of the spots that are currently missing refs can
potentially
>> > > get called in the runtime suspend/resume path. Otherwise,
we'll try to
>> > > resume the GPU as we're trying to resume the GPU (and
vice-versa) and
>> > > cause the kernel to deadlock.
>> > >
>> > > With this, it should be safe to call the pm runtime functions
in any
>> > > context in nouveau with one condition: any point in the
driver that
>> > > calls pm_runtime_get*() cannot hold any locks owned by
nouveau that
>> > > would be acquired anywhere inside
nouveau_pmops_runtime_resume().
>> > > This includes modesetting locks, i2c bus locks, etc.
>> >
>> > [snip]
>> > > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
>> > > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
>> > > @@ -835,6 +835,8 @@ nouveau_pmops_runtime_suspend(struct
device *dev)
>> > >           return -EBUSY;
>> > >   }
>> > >
>> > > + dev->power.disable_depth++;
>> > > +
>> >
>> > Anyway, if I understand the commit message correctly, you're
hitting a
>> > pm_runtime_get_sync() in a code path that itself is called during
a
>> > pm_runtime_get_sync().  Could you include stack traces in the
commit
>> > message?  My gut feeling is that this patch masks a deeper issue,
>> > e.g. if the runtime_resume code path does in fact directly poll
outputs,
>> > that would seem wrong.  Runtime resume should merely make the card
>> > accessible, i.e. reinstate power if necessary, put into PCI_D0,
>> > restore registers, etc.  Output polling should be scheduled
>> > asynchronously.
>>
>> So: the reason that patch was added was mainly for the patches later in
the
>> series that add guards around the i2c bus and aux bus, since both of
those
>> require that the device be awake for it to work. Currently, the spot
where it
>> would recurse is:
>
> Okay, the PCI device is suspending and the nvkm_i2c_aux_acquire()
> wants it in resumed state, so is waiting forever for the device to
> runtime suspend in order to resume it again immediately afterwards.
>
> The deadlock in the stack trace you've posted could be resolved using
> the technique I used in d61a5c106351 by adding the following to
> include/linux/pm_runtime.h:
>
> static inline bool pm_runtime_status_suspending(struct device *dev)
> {
>         return dev->power.runtime_status == RPM_SUSPENDING;
> }
>
> static inline bool is_pm_work(struct device *dev)
> {
>         struct work_struct *work = current_work();
>
>         return work && work->func == dev->power.work;
> }
>
> Then adding this to nvkm_i2c_aux_acquire():
>
>         struct device *dev = pad->i2c->subdev.device->dev;
>
>         if (!(is_pm_work(dev) &&
pm_runtime_status_suspending(dev))) {
>                 ret = pm_runtime_get_sync(dev);
>                 if (ret < 0 && ret != -EACCES)
>                         return ret;
>         }
>
> But here's the catch:  This only works for an *async* runtime suspend.
> It doesn't work for pm_runtime_put_sync(), pm_runtime_suspend() etc,
> because then the runtime suspend is executed in the context of the caller,
> not in the context of dev->power.work.
>
> So it's not a full solution, but hopefully something that gets you
> going.  I'm not really familiar with the code paths leading to
> nvkm_i2c_aux_acquire() to come up with a full solution off the top
> of my head I'm afraid.
>
> Note, it's not sufficient to just check
pm_runtime_status_suspending(dev)
> because if the runtime_suspend is carried out concurrently by something
> else, this will return true but it's not guaranteed that the device is
> actually kept awake until the i2c communication has been fully performed.
For the record, I don't quite like this approach as it seems to be
working around a broken dependency graph.

If you need to resume device A from within the runtime resume callback
of device B, then clearly B depends on A and there should be a link
between them.

That said, I do realize that it may be the path of least resistance,
but then I wonder if we can do better than this.

Thanks,
Rafael

On Wed, Jul 18, 2018 at 09:38:41AM +0200, Rafael J. Wysocki
wrote:
> On Tue, Jul 17, 2018 at 8:20 PM, Lukas Wunner <lukas at wunner.de>
wrote:
> > Okay, the PCI device is suspending and the nvkm_i2c_aux_acquire()
> > wants it in resumed state, so is waiting forever for the device to
> > runtime suspend in order to resume it again immediately afterwards.
> >
> > The deadlock in the stack trace you've posted could be resolved
using
> > the technique I used in d61a5c106351 by adding the following to
> > include/linux/pm_runtime.h:
> >
> > static inline bool pm_runtime_status_suspending(struct device *dev)
> > {
> >         return dev->power.runtime_status == RPM_SUSPENDING;
> > }
> >
> > static inline bool is_pm_work(struct device *dev)
> > {
> >         struct work_struct *work = current_work();
> >
> >         return work && work->func == dev->power.work;
> > }
> >
> > Then adding this to nvkm_i2c_aux_acquire():
> >
> >         struct device *dev = pad->i2c->subdev.device->dev;
> >
> >         if (!(is_pm_work(dev) &&
pm_runtime_status_suspending(dev))) {
> >                 ret = pm_runtime_get_sync(dev);
> >                 if (ret < 0 && ret != -EACCES)
> >                         return ret;
> >         }
[snip]
> 
> For the record, I don't quite like this approach as it seems to be
> working around a broken dependency graph.
> 
> If you need to resume device A from within the runtime resume callback
> of device B, then clearly B depends on A and there should be a link
> between them.
> 
> That said, I do realize that it may be the path of least resistance,
> but then I wonder if we can do better than this.
The GPU contains an i2c subdevice for each connector with DDC lines.
I believe those are modelled as children of the GPU's PCI device as
they're accessed via mmio of the PCI device.

The problem here is that when the GPU's PCI device runtime suspends,
its i2c child device needs to be runtime active to suspend the MST
topology.  Catch-22.

I don't know whether or not it's necessary to suspend the MST topology.
I'm not an expert on DisplayPort MultiStream transport.

BTW Lyude, in patch 4 and 5 of this series, you're runtime resuming
pad->i2c->subdev.device->dev.  Is this the PCI device or is it the i2c
device?  I'm always confused by nouveau's structs.  In
nvkm_i2c_bus_ctor()
I can see that the device you're runtime resuming is the parent of the
i2c_adapter:

	struct nvkm_device *device = pad->i2c->subdev.device;
	[...]
	bus->i2c.dev.parent = device->dev;

If the i2c_adapter is a child of the PCI device, it's sufficient
to runtime resume the i2c_adapter, i.e. bus->i2c.dev, and this will
implicitly runtime resume its parent.

Thanks,

Lukas