vga switcheroo: Prevent nouveau irq handler from kill the system

PATCH In review Alex Williamson 3 participants 9 messages May 2, 2011 – May 4, 2011

Subsystems (4)

DRM drivers DRM misc nouveau VGA_SWITCHEROO

lore

Share & tools

lore

Alex Williamson

author

2011-05-02 00:49 UTC #0 lore raw

cover vga switcheroo: Prevent nouveau irq handler from kill the system

On an Intel/Nvidia hybrid graphics system, using the integrated graphics and powering off the discrete, one of the primary sources of system hangs seems to be the nouveau driver processing interrupts while the device is powered off. This short series removes the unbalanced pci_enable_device() from core switcheroo code, allowing us to check pci_is_enabled() from the nouveau irq handler to determine whether to touch hardware. Thanks,

Alex

---

Alex Williamson (2):
drm/nouveau: Check that the device is enabled before processing interrupt
vga_switcheroo: Remove unbalanced pci_enable_device

 drivers/gpu/drm/nouveau/nouveau_drv.c |    2 ++
 drivers/gpu/drm/nouveau/nouveau_irq.c |    3 +++
 drivers/gpu/vga/vga_switcheroo.c      |    6 ------
 3 files changed, 5 insertions(+), 6 deletions(-)

Alex Williamson

author

2011-05-02 00:49 UTC #1 lore raw

PATCH 1/2 vga_switcheroo: Remove unbalanced pci_enable_device

This is unbalanced and probably more fitting for the client to take care of. Remove it.

Signed-off-by Alex Williamson <[email protected]>

---

 drivers/gpu/vga/vga_switcheroo.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

drivers/gpu/vga/vga_switcheroo.c +0 -6

index e01cacb..f0f244d 100644
@@ -215,7 +215,6 @@ static int vga_switchoff(struct vga_switcheroo_client *client)
 /* stage one happens before delay */
 static int vga_switchto_stage1(struct vga_switcheroo_client *new_client)
 {
-	int ret;
 	int i;
 	struct vga_switcheroo_client *active = NULL;

@@ -231,11 +230,6 @@ static int vga_switchto_stage1(struct vga_switcheroo_client *new_client)
 	if (!active)
 		return 0;
 
-	/* power up the first device */
-	ret = pci_enable_device(new_client->pdev);
-	if (ret)
-		return ret;
-
 	if (new_client->pwr_state == VGA_SWITCHEROO_OFF)
 		vga_switchon(new_client);

Dave Airlie

maintainer

2011-05-04 03:49 UTC #2 lore raw

Re: [PATCH 1/2] vga_switcheroo: Remove unbalanced pci_enable_device

Quoted 2 lines

This is unbalanced and probably more fitting for the client to take care of. Remove it.

I've queued this in -next.

Dave.

Alex Williamson

author

2011-05-02 00:49 UTC #3 lore raw

PATCH 2/2 drm/nouveau: Check that the device is enabled before processing interrupt

In reply to Alex Williamson

We're likely to be sharing an interrupt line with other devices, which means our handler might get called after we've turned off the device via vga switcheroo. This can lead to all sorts of badness, like nv04_fifo_isr() spewing "PFIFO still angry after 100 spins, halt" to the console before the system enters a hard hang.

We can avoid this by simply checking if the device is still enabled before processing an interrupt. To avoid races, flush any inflight interrupts using synchronize_irq(). Note that since pci_intx() is called after pci_save_state(), pci_restore_state() will automatically re-enable INTx.

Signed-off-by Alex Williamson <[email protected]>

---

 drivers/gpu/drm/nouveau/nouveau_drv.c |    2 ++
 drivers/gpu/drm/nouveau/nouveau_irq.c |    3 +++
 2 files changed, 5 insertions(+), 0 deletions(-)

drivers/gpu/drm/nouveau/nouveau_drv.c +2 -0

index 155ebdc..405d4f1 100644
@@ -230,7 +230,9 @@ nouveau_pci_suspend(struct pci_dev *pdev, pm_message_t pm_state)
 	NV_INFO(dev, "And we're gone!\n");
 	pci_save_state(pdev);
 	if (pm_state.event == PM_EVENT_SUSPEND) {
+		pci_intx(pdev, 0);
 		pci_disable_device(pdev);
+		synchronize_irq(drm_dev_to_irq(dev));
 		pci_set_power_state(pdev, PCI_D3hot);
 	}

drivers/gpu/drm/nouveau/nouveau_irq.c +3 -0

index 2ba7265..8fd17e6 100644
@@ -78,6 +78,9 @@ nouveau_irq_handler(DRM_IRQ_ARGS)
 	u32 stat;
 	int i;
 
+	if (unlikely(!pci_is_enabled(dev->pdev)))
+		return IRQ_NONE;
+
 	stat = nv_rd32(dev, NV03_PMC_INTR_0);
 	if (!stat)
 		return IRQ_NONE;

Dave Airlie

maintainer

2011-05-04 03:50 UTC #4 lore raw

Re: [PATCH 2/2] drm/nouveau: Check that the device is enabled before processing interrupt

Quoted 7 lines · Alex Williamson

We're likely to be sharing an interrupt line with other devices, which means our handler might get called after we've turned off the device via vga switcheroo. This can lead to all sorts of badness, like nv04_fifo_isr() spewing "PFIFO still angry after 100 spins, halt" to the console before the system enters a hard hang.

Quoted 5 lines

We can avoid this by simply checking if the device is still enabled before processing an interrupt. To avoid races, flush any inflight interrupts using synchronize_irq(). Note that since pci_intx() is called after pci_save_state(), pci_restore_state() will automatically re-enable INTx.

I still think we should just need the synchronize_irq followed by a check in the irq handler for all fs,

or is there a race there I'm missing?

Dave.

Alex Williamson

author

2011-05-04 04:18 UTC #5 lore raw

Re: [PATCH 2/2] drm/nouveau: Check that the device is enabled before processing interrupt

Quoted 15 lines · Dave Airlie

Quoted 7 lines · Alex Williamson
We're likely to be sharing an interrupt line with other devices, which means our handler might get called after we've turned off the device via vga switcheroo. This can lead to all sorts of badness, like nv04_fifo_isr() spewing "PFIFO still angry after 100 spins, halt" to the console before the system enters a hard hang.
Quoted 5 lines
We can avoid this by simply checking if the device is still enabled before processing an interrupt. To avoid races, flush any inflight interrupts using synchronize_irq(). Note that since pci_intx() is called after pci_save_state(), pci_restore_state() will automatically re-enable INTx.

Quoted 4 lines

I still think we should just need the synchronize_irq followed by a check in the irq handler for all fs,
or is there a race there I'm missing?

The synchronize_irq by itself doesn't guarantee anything. The irq handler could be immediately started on another CPU once that returns and be well past the first device read before we make it far enough through pci_set_power_state that the device becomes unresponsive. Can we guarantee that first device read in the interrupt handler will always be 0 or -1 in the suspend path? Even as the last milliamperes of charge drain out of the device?

By adding the device enabled check in the interrupt handler, disabling the device, then calling synchronize_irq, we guarantee that the entire interrupt handler path is not being executed and won't be executed again until we re-enable the device. It does seem a bit odd, but how many other devices in the system are entirely powered off, with a driver attached and interrupt handler registered while the system is still humming along? Thanks,

Alex

Dave Airlie

2011-05-04 04:22 UTC #6 lore raw

Re: [PATCH 2/2] drm/nouveau: Check that the device is enabled before processing interrupt

Quoted 21 lines · Alex Williamson

Quoted 15 lines · Dave Airlie
Quoted 7 lines · Alex Williamson
We're likely to be sharing an interrupt line with other devices, which means our handler might get called after we've turned off the device via vga switcheroo. This can lead to all sorts of badness, like nv04_fifo_isr() spewing "PFIFO still angry after 100 spins, halt" to the console before the system enters a hard hang.
Quoted 5 lines
We can avoid this by simply checking if the device is still enabled before processing an interrupt. To avoid races, flush any inflight interrupts using synchronize_irq(). Note that since pci_intx() is called after pci_save_state(), pci_restore_state() will automatically re-enable INTx.
Quoted 4 lines
I still think we should just need the synchronize_irq followed by a check in the irq handler for all fs,
or is there a race there I'm missing?

Quoted 7 lines

The synchronize_irq by itself doesn't guarantee anything. The irq handler could be immediately started on another CPU once that returns and be well past the first device read before we make it far enough through pci_set_power_state that the device becomes unresponsive. Can we guarantee that first device read in the interrupt handler will always be 0 or -1 in the suspend path? Even as the last milliamperes of charge drain out of the device?

It should always be a valid irq or 0xffffffff. It got nothing to do with milliamperes of charge, and all to do with the PCI BAR decodes being turned off.

Quoted 7 lines

By adding the device enabled check in the interrupt handler, disabling the device, then calling synchronize_irq, we guarantee that the entire interrupt handler path is not being executed and won't be executed again until we re-enable the device. It does seem a bit odd, but how many other devices in the system are entirely powered off, with a driver attached and interrupt handler registered while the system is still humming along? Thanks,

The theory is lots. OLPC does this sort of things for its breakfast I'd have thought.

which is why I still think we are missing something, when we D3 the device it should be the same as suspend/resume cycle pretty much,

I'll have to think about it a bit more and maybe see what PM guys can tell us.

mjg59 any clues?

Dave.

Alex Williamson

author

2011-05-04 04:47 UTC #7 lore raw

Re: [PATCH 2/2] drm/nouveau: Check that the device is enabled before processing interrupt

Quoted 30 lines · Dave Airlie

Quoted 21 lines · Alex Williamson
Quoted 15 lines · Dave Airlie
Quoted 7 lines · Alex Williamson
We're likely to be sharing an interrupt line with other devices, which means our handler might get called after we've turned off the device via vga switcheroo. This can lead to all sorts of badness, like nv04_fifo_isr() spewing "PFIFO still angry after 100 spins, halt" to the console before the system enters a hard hang.
Quoted 5 lines
We can avoid this by simply checking if the device is still enabled before processing an interrupt. To avoid races, flush any inflight interrupts using synchronize_irq(). Note that since pci_intx() is called after pci_save_state(), pci_restore_state() will automatically re-enable INTx.
Quoted 4 lines
I still think we should just need the synchronize_irq followed by a check in the irq handler for all fs,
or is there a race there I'm missing?
Quoted 7 lines
The synchronize_irq by itself doesn't guarantee anything. The irq handler could be immediately started on another CPU once that returns and be well past the first device read before we make it far enough through pci_set_power_state that the device becomes unresponsive. Can we guarantee that first device read in the interrupt handler will always be 0 or -1 in the suspend path? Even as the last milliamperes of charge drain out of the device?

Quoted 3 lines

It should always be a valid irq or 0xffffffff. It got nothing to do with milliamperes of charge, and all to do with the PCI BAR decodes being turned off.

Unfortunately this depends on the platform behavior for a master abort. But maybe this bring up another issue; we really want to make sure we're turning off decode after we've flushed any inflight interrupts. We can't guarantee that w/o some kind of synchronization and the device being enabled is probably too late... are you sure you don't want to uninstall the interrupt handler? ;)

Quoted 8 lines

Quoted 7 lines
By adding the device enabled check in the interrupt handler, disabling the device, then calling synchronize_irq, we guarantee that the entire interrupt handler path is not being executed and won't be executed again until we re-enable the device. It does seem a bit odd, but how many other devices in the system are entirely powered off, with a driver attached and interrupt handler registered while the system is still humming along? Thanks,

Quoted 5 lines

The theory is lots. OLPC does this sort of things for its breakfast I'd have thought.
which is why I still think we are missing something, when we D3 the device it should be the same as suspend/resume cycle pretty much,

Except the whole system goes down for a suspend/resume and we don't typically have to worry about stray interrupt during the down time. If our vga switcheroo handler is doing the right thing, we're not only going to D3, we're entirely removing power from the device. Some platforms (probably not ones we care about for switcheroo, but nonetheless) won't put up with a master abort on the bus that would be caused by reading from an effectively non-existent device. Thanks,

Alex

Dave Airlie

2011-05-04 04:55 UTC #8 lore raw

Re: [PATCH 2/2] drm/nouveau: Check that the device is enabled before processing interrupt

Quoted 35 lines · Alex Williamson

Quoted 30 lines · Dave Airlie
Quoted 21 lines · Alex Williamson
Quoted 15 lines · Dave Airlie
Quoted 7 lines · Alex Williamson
We're likely to be sharing an interrupt line with other devices, which means our handler might get called after we've turned off the device via vga switcheroo. This can lead to all sorts of badness, like nv04_fifo_isr() spewing "PFIFO still angry after 100 spins, halt" to the console before the system enters a hard hang.
Quoted 5 lines
We can avoid this by simply checking if the device is still enabled before processing an interrupt. To avoid races, flush any inflight interrupts using synchronize_irq(). Note that since pci_intx() is called after pci_save_state(), pci_restore_state() will automatically re-enable INTx.
Quoted 4 lines
I still think we should just need the synchronize_irq followed by a check in the irq handler for all fs,
or is there a race there I'm missing?
Quoted 7 lines
The synchronize_irq by itself doesn't guarantee anything. The irq handler could be immediately started on another CPU once that returns and be well past the first device read before we make it far enough through pci_set_power_state that the device becomes unresponsive. Can we guarantee that first device read in the interrupt handler will always be 0 or -1 in the suspend path? Even as the last milliamperes of charge drain out of the device?
Quoted 3 lines
It should always be a valid irq or 0xffffffff. It got nothing to do with milliamperes of charge, and all to do with the PCI BAR decodes being turned off.

Quoted 6 lines

Unfortunately this depends on the platform behavior for a master abort. But maybe this bring up another issue; we really want to make sure we're turning off decode after we've flushed any inflight interrupts. We can't guarantee that w/o some kind of synchronization and the device being enabled is probably too late... are you sure you don't want to uninstall the interrupt handler? ;)

The x86 behaviour is all we care about btw, don't even think about worry about someone creating a powerpc with hybrid graphics.

As I said OLPC does this lots. I'm hoping there is some example of how this should work, maybe it should work like this, but in that case I'd like a previous example of it.

Quoted 6 lines

Quoted 5 lines
The theory is lots. OLPC does this sort of things for its breakfast I'd have thought.
which is why I still think we are missing something, when we D3 the device it should be the same as suspend/resume cycle pretty much,

Quoted 7 lines

Except the whole system goes down for a suspend/resume and we don't typically have to worry about stray interrupt during the down time. If our vga switcheroo handler is doing the right thing, we're not only going to D3, we're entirely removing power from the device. Some platforms (probably not ones we care about for switcheroo, but nonetheless) won't put up with a master abort on the bus that would be caused by reading from an effectively non-existent device. Thanks,

Again OLPC and lots of other embedded systems D3 devices without D3ing the whole system, and AFAIK without deinstalling the irq handlers.

I'll try and find some time to look into it deeper later this week.

Dave.