Skip to contentSkip to conversation

PCI: vmd: Free up IRQs on suspend path

PATCH Superseded Scott Bauer 3 participants 6 messages Jul 20, 2017 PCI

lore
Share & tools
lore

Scott Bauer

author
#0 lore raw

PATCH 1/1 PCI: vmd: Free up IRQs on suspend path

This patch frees up the IRQs we request on the suspend path, and reallocates them on the resume path.

Fixes:

[  559.964386] CPU 111 disable failed: CPU has 9 vectors assigned and there are only 0 available.
[  559.966824] Error taking CPU111 down: -34
[  559.966825] Non-boot CPUs are not disabled
[  559.966826] Enabling non-boot CPUs ...
Signed-off-by Scott Bauer <[email protected]>
---
 drivers/pci/host/vmd.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
drivers/pci/host/vmd.c +14 -0
index 6088c3083194..88bf0c754fb2 100644
@@ -755,6 +755,11 @@ static void vmd_remove(struct pci_dev *dev)
 static int vmd_suspend(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
+	struct vmd_dev *vmd = pci_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < vmd->msix_count; i++)
+                devm_free_irq(dev, pci_irq_vector(pdev, i), &vmd->irqs[i]);
 
 	pci_save_state(pdev);
 	return 0;

@@ -763,7 +768,16 @@ static int vmd_suspend(struct device *dev)
 static int vmd_resume(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
+	struct vmd_dev *vmd = pci_get_drvdata(pdev);
+	int err, i;
 
+	for (i = 0; i < vmd->msix_count; i++) {
+		err = devm_request_irq(dev, pci_irq_vector(pdev, i),
+				       vmd_irq, IRQF_NO_THREAD,
+				       "vmd", &vmd->irqs[i]);
+		if (err)
+			return err;
+	}
 	pci_restore_state(pdev);
 	return 0;
 }
Signature · 1 line
2.11.0
#1 lore raw
Quoted 31 lines · Scott Bauer

This patch frees up the IRQs we request on the suspend path, and reallocates them on the resume path.

Fixes:

[  559.964386] CPU 111 disable failed: CPU has 9 vectors assigned and there are only 0 available.
[  559.966824] Error taking CPU111 down: -34
[  559.966825] Non-boot CPUs are not disabled
[  559.966826] Enabling non-boot CPUs ...
Signed-off-by Scott Bauer <[email protected]>
---
 drivers/pci/host/vmd.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
drivers/pci/host/vmd.c +5 -0
index 6088c3083194..88bf0c754fb2 100644
@@ -755,6 +755,11 @@ static void vmd_remove(struct pci_dev *dev)
 static int vmd_suspend(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
+	struct vmd_dev *vmd = pci_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < vmd->msix_count; i++)
+                devm_free_irq(dev, pci_irq_vector(pdev, i), &vmd->irqs[i]);
 
 	pci_save_state(pdev);
 	return 0;

drivers/pci/host/vmd.c static int vmd_suspend(struct device *dev) · line 776

Quoted 9 lines
drivers/pci/host/vmd.c +5 -0
@@ -763,7 +768,16 @@ static int vmd_suspend(struct device *dev)
 static int vmd_resume(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
+	struct vmd_dev *vmd = pci_get_drvdata(pdev);
+	int err, i;
 
+	for (i = 0; i < vmd->msix_count; i++) {
+		err = devm_request_irq(dev, pci_irq_vector(pdev, i),
+				       vmd_irq, IRQF_NO_THREAD,

The flag here looks redundant to me because the fn calls into request_threaded_irq with no thread_fn. But since it's a bit more verbose, can you change the other devm_request_irq to use IRQF_NO_THREAD?

Quoted 8 lines
drivers/pci/host/vmd.c +4 -0
+				       "vmd", &vmd->irqs[i]);
+		if (err)
+			return err;
+	}
 	pci_restore_state(pdev);
 	return 0;
 }
author
#2 lore raw
Quoted 38 lines · Jon Derrick
Quoted 37 lines · Scott Bauer

This patch frees up the IRQs we request on the suspend path, and reallocates them on the resume path.

Fixes:

[  559.964386] CPU 111 disable failed: CPU has 9 vectors assigned and there are only 0 available.
[  559.966824] Error taking CPU111 down: -34
[  559.966825] Non-boot CPUs are not disabled
[  559.966826] Enabling non-boot CPUs ...
Signed-off-by Scott Bauer <[email protected]>
---
 drivers/pci/host/vmd.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
drivers/pci/host/vmd.c +7 -0
index 6088c3083194..88bf0c754fb2 100644
@@ -755,6 +755,11 @@ static void vmd_remove(struct pci_dev *dev)
 static int vmd_suspend(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
+	struct vmd_dev *vmd = pci_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < vmd->msix_count; i++)
+                devm_free_irq(dev, pci_irq_vector(pdev, i), &vmd->irqs[i]);
 
 	pci_save_state(pdev);
 	return 0;

@@ -763,7 +768,16 @@ static int vmd_suspend(struct device *dev)
 static int vmd_resume(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
+	struct vmd_dev *vmd = pci_get_drvdata(pdev);
+	int err, i;
Quoted 6 lines

drivers/pci/host/vmd.c static int vmd_suspend(struct device *dev) · line 776

Quoted 3 lines
drivers/pci/host/vmd.c +3 -0
+	for (i = 0; i < vmd->msix_count; i++) {
+		err = devm_request_irq(dev, pci_irq_vector(pdev, i),
+				       vmd_irq, IRQF_NO_THREAD,

The flag here looks redundant to me because the fn calls into request_threaded_irq with no thread_fn. But since it's a bit more verbose, can you change the other devm_request_irq to use IRQF_NO_THREAD?

I actually yanked the request irq from the other call. It was changed to NO_THREAD in April:

(3eefa790c968) PCI: host: Mark PCIe/PCI (MSI) cascade ISR as IRQF_NO_THREAD

#3 lore raw

On 07/20/2017 03:34 PM, Scott Bauer wrote:
[snip]

Quoted 7 lines
Quoted 6 lines

drivers/pci/host/vmd.c static int vmd_suspend(struct device *dev) · line 776

Quoted 3 lines
drivers/pci/host/vmd.c +3 -0
+	for (i = 0; i < vmd->msix_count; i++) {
+		err = devm_request_irq(dev, pci_irq_vector(pdev, i),
+				       vmd_irq, IRQF_NO_THREAD,

The flag here looks redundant to me because the fn calls into request_threaded_irq with no thread_fn. But since it's a bit more verbose, can you change the other devm_request_irq to use IRQF_NO_THREAD?

Quoted 5 lines

I actually yanked the request irq from the other call. It was changed to NO_THREAD in April:

(3eefa790c968) PCI: host: Mark PCIe/PCI (MSI) cascade ISR as IRQF_NO_THREAD

Ah so it was. Sorry was looking at v4.12

I think we'll need to do more than free the irq handlers because if an interrupt occurs after that, I think it'll get kicked to handle_bad_irq. I think we just need to add pci_disable_device after save and pcim_enable_device after restore.

author
#4 lore raw
Quoted 15 lines · Jon Derrick

On 07/20/2017 03:34 PM, Scott Bauer wrote:
[snip]

Quoted 7 lines
Quoted 6 lines

drivers/pci/host/vmd.c static int vmd_suspend(struct device *dev) · line 776

Quoted 3 lines
drivers/pci/host/vmd.c +3 -0
+	for (i = 0; i < vmd->msix_count; i++) {
+		err = devm_request_irq(dev, pci_irq_vector(pdev, i),
+				       vmd_irq, IRQF_NO_THREAD,

The flag here looks redundant to me because the fn calls into request_threaded_irq with no thread_fn. But since it's a bit more verbose, can you change the other devm_request_irq to use IRQF_NO_THREAD?

Quoted 5 lines

I actually yanked the request irq from the other call. It was changed to NO_THREAD in April:

(3eefa790c968) PCI: host: Mark PCIe/PCI (MSI) cascade ISR as IRQF_NO_THREAD

Quoted 7 lines

Ah so it was. Sorry was looking at v4.12

I think we'll need to do more than free the irq handlers because if an interrupt occurs after that, I think it'll get kicked to handle_bad_irq. I think we just need to add pci_disable_device after save and pcim_enable_device after restore.

I orignally had this in the patch I was testing on SLES and took it out when it seemingly didn't make a difference. I didnt test yanking a drive or anything behind the domain while it was suspended, so I wouldn't have hit your scenario.

I'll add it back in. Also, I think I need to CC stable on this one too since this is missing all the way since the original inception of this driver.

#5 lore raw
Quoted 7 lines · Scott Bauer
Quoted 5 lines · Jon Derrick

I think we'll need to do more than free the irq handlers because if an interrupt occurs after that, I think it'll get kicked to handle_bad_irq. I think we just need to add pci_disable_device after save and pcim_enable_device after restore.

Quoted 6 lines

I orignally had this in the patch I was testing on SLES and took it out when it seemingly didn't make a difference. I didnt test yanking a drive or anything behind the domain while it was suspended, so I wouldn't have hit your scenario.

I'll add it back in. Also, I think I need to CC stable on this one too since this is missing all the way since the original inception of this driver.

I think your patch should be okay as-is. After freeing VMD's IRQs, the kernel will automatically clear the MSI-x control bit to disable it from firing.