[RFC 0/4] Introduce a mechanism to allocate PCI IRQ on demand

[RFC 0/4] Introduce a mechanism to allocate PCI IRQ on demand

[Jiang Liu]

This patch set introduces a mechanism to allocate PCI IRQ on demand and
free it when not used anymore by hooking pci_device_probe() and
pci_device_remove().

It will be used to track IOAPIC pin usage on x86 so we could support
IOAPIC hot-removal.

The patch set passes Fengguang's 0day test suite and is available at:
https://github.com/jiangliu/linux.git pci_irq_v1

Thanks!
Gerry

Jiang Liu (4):
  PCI: Add hooks to allocate/free IRQ resources when binding/unbinding
    driver
  PCI, MSI: Optionally free legacy PCI IRQ when enabling MSI/MSI-X
  PCI, x86: Allocate PCI IRQ on demand and free it when not used
    anymore
  PCI: Introduce helpers to manage pci_dev->irq and
    pci_dev->irq_managed

 arch/x86/include/asm/pci_x86.h |    2 --
 arch/x86/pci/common.c          |   20 +++++++++-----------
 arch/x86/pci/intel_mid_pci.c   |    9 ++++++---
 arch/x86/pci/irq.c             |   23 ++++-------------------
 drivers/acpi/pci_irq.c         |   17 ++++-------------
 drivers/pci/msi.c              |    6 +++++-
 drivers/pci/pci-driver.c       |   33 +++++++++++++++++++++++----------
 include/linux/pci.h            |   19 +++++++++++++++++++
 8 files changed, 70 insertions(+), 59 deletions(-)

-- 
1.7.10.4

[RFC 1/4] PCI: Add hooks to allocate/free IRQ resources when binding/unbinding driver

[Jiang Liu]

Add two hook points pcibios_{alloc|free}_irq() into PCI core, which will
be called when binding/unbinding PCI device drivers. Then PCI arch code
may hook into these two points to allocate IRQ resources on demand and
free them when not used anymore.

Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 drivers/pci/pci-driver.c |   33 +++++++++++++++++++++++----------
 include/linux/pci.h      |    2 ++
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 3cb2210de553..8af4a671686f 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -388,18 +388,30 @@ static int __pci_device_probe(struct pci_driver *drv, struct pci_dev *pci_dev)
 	return error;
 }
 
-static int pci_device_probe(struct device *dev)
+int __weak pcibios_alloc_irq(struct pci_dev *dev)
 {
-	int error = 0;
-	struct pci_driver *drv;
-	struct pci_dev *pci_dev;
+	return dev->irq;
+}
 
-	drv = to_pci_driver(dev->driver);
-	pci_dev = to_pci_dev(dev);
-	pci_dev_get(pci_dev);
-	error = __pci_device_probe(drv, pci_dev);
-	if (error)
-		pci_dev_put(pci_dev);
+void __weak pcibios_free_irq(struct pci_dev *dev)
+{
+}
+
+static int pci_device_probe(struct device *dev)
+{
+	int error;
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct pci_driver *drv = to_pci_driver(dev->driver);
+
+	error = pcibios_alloc_irq(pci_dev);
+	if (error >= 0) {
+		pci_dev_get(pci_dev);
+		error = __pci_device_probe(drv, pci_dev);
+		if (error) {
+			pcibios_free_irq(pci_dev);
+			pci_dev_put(pci_dev);
+		}
+	}
 
 	return error;
 }
@@ -415,6 +427,7 @@ static int pci_device_remove(struct device *dev)
 			drv->remove(pci_dev);
 			pm_runtime_put_noidle(dev);
 		}
+		pcibios_free_irq(pci_dev);
 		pci_dev->driver = NULL;
 	}
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 353db8dc4c6e..f50d16a04abc 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1656,6 +1656,8 @@ int pcibios_set_pcie_reset_state(struct pci_dev *dev,
 int pcibios_add_device(struct pci_dev *dev);
 void pcibios_release_device(struct pci_dev *dev);
 void pcibios_penalize_isa_irq(int irq, int active);
+int pcibios_alloc_irq(struct pci_dev *dev);
+void pcibios_free_irq(struct pci_dev *dev);
 
 #ifdef CONFIG_HIBERNATE_CALLBACKS
 extern struct dev_pm_ops pcibios_pm_ops;
-- 
1.7.10.4

[RFC 2/4] PCI, MSI: Optionally free legacy PCI IRQ when enabling MSI/MSI-X

[Jiang Liu]

Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't
make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again.
So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate
when disabling MSI/MSI-X.

Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 drivers/pci/msi.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index c3e7dfcf9ff5..47cf72c669f0 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -686,6 +686,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
 	msi_set_enable(dev, 1);
 	dev->msi_enabled = 1;
 
+	pcibios_free_irq(dev);
 	dev->irq = entry->irq;
 	return 0;
 }
@@ -813,9 +814,10 @@ static int msix_capability_init(struct pci_dev *dev,
 	/* Set MSI-X enabled bits and unmask the function */
 	pci_intx_for_msi(dev, 0);
 	dev->msix_enabled = 1;
-
 	msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
 
+	pcibios_free_irq(dev);
+
 	return 0;
 
 out_avail:
@@ -930,6 +932,7 @@ void pci_msi_shutdown(struct pci_dev *dev)
 
 	/* Restore dev->irq to its default pin-assertion irq */
 	dev->irq = desc->msi_attrib.default_irq;
+	pcibios_alloc_irq(dev);
 }
 
 void pci_disable_msi(struct pci_dev *dev)
@@ -1030,6 +1033,7 @@ void pci_msix_shutdown(struct pci_dev *dev)
 	msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
 	pci_intx_for_msi(dev, 1);
 	dev->msix_enabled = 0;
+	pcibios_alloc_irq(dev);
 }
 
 void pci_disable_msix(struct pci_dev *dev)
-- 
1.7.10.4

[RFC 3/4] PCI, x86: Allocate PCI IRQ on demand and free it when not used anymore

[Jiang Liu]

To support IOAPIC hotplug, we need to correctly manage IOAPIC pin usage,
which is to allocate IRQs on demand and free them when not used anymore.
So use pcibios_alloc_irq() and pcibios_free_irq() to dynamically allocate
and free PCI IRQs.

Also remove obseleted code mp_should_keep_irq().

Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 arch/x86/include/asm/pci_x86.h |    2 --
 arch/x86/pci/common.c          |   20 +++++++++-----------
 arch/x86/pci/intel_mid_pci.c   |    7 +++++--
 arch/x86/pci/irq.c             |   15 +--------------
 drivers/acpi/pci_irq.c         |    9 +--------
 5 files changed, 16 insertions(+), 37 deletions(-)

diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h
index 164e3f8d3c3d..fa1195dae425 100644
--- a/arch/x86/include/asm/pci_x86.h
+++ b/arch/x86/include/asm/pci_x86.h
@@ -93,8 +93,6 @@ extern raw_spinlock_t pci_config_lock;
 extern int (*pcibios_enable_irq)(struct pci_dev *dev);
 extern void (*pcibios_disable_irq)(struct pci_dev *dev);
 
-extern bool mp_should_keep_irq(struct device *dev);
-
 struct pci_raw_ops {
 	int (*read)(unsigned int domain, unsigned int bus, unsigned int devfn,
 						int reg, int len, u32 *val);
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 8fd6f44aee83..dc78a4a9a466 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -673,24 +673,22 @@ int pcibios_add_device(struct pci_dev *dev)
 	return 0;
 }
 
-int pcibios_enable_device(struct pci_dev *dev, int mask)
+int pcibios_alloc_irq(struct pci_dev *dev)
 {
-	int err;
-
-	if ((err = pci_enable_resources(dev, mask)) < 0)
-		return err;
-
-	if (!pci_dev_msi_enabled(dev))
-		return pcibios_enable_irq(dev);
-	return 0;
+	return pcibios_enable_irq(dev);
 }
 
-void pcibios_disable_device (struct pci_dev *dev)
+void pcibios_free_irq(struct pci_dev *dev)
 {
-	if (!pci_dev_msi_enabled(dev) && pcibios_disable_irq)
+	if (pcibios_disable_irq)
 		pcibios_disable_irq(dev);
 }
 
+int pcibios_enable_device(struct pci_dev *dev, int mask)
+{
+	return pci_enable_resources(dev, mask);
+}
+
 int pci_ext_cfg_avail(void)
 {
 	if (raw_pci_ext_ops)
diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c
index 27062303c881..fb7a1f96d80c 100644
--- a/arch/x86/pci/intel_mid_pci.c
+++ b/arch/x86/pci/intel_mid_pci.c
@@ -234,10 +234,13 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev)
 
 static void intel_mid_pci_irq_disable(struct pci_dev *dev)
 {
-	if (!mp_should_keep_irq(&dev->dev) && dev->irq_managed &&
-	    dev->irq > 0) {
+	if (dev->irq_managed && dev->irq > 0) {
 		mp_unmap_irq(dev->irq);
 		dev->irq_managed = 0;
+		/*
+		 * Don't reset dev->irq here, otherwise
+		 * intel_mid_pci_irq_enable() will fail on next call.
+		 */
 	}
 }
 
diff --git a/arch/x86/pci/irq.c b/arch/x86/pci/irq.c
index 5dc6ca5e1741..e71b3dbd87b8 100644
--- a/arch/x86/pci/irq.c
+++ b/arch/x86/pci/irq.c
@@ -1256,22 +1256,9 @@ static int pirq_enable_irq(struct pci_dev *dev)
 	return 0;
 }
 
-bool mp_should_keep_irq(struct device *dev)
-{
-	if (dev->power.is_prepared)
-		return true;
-#ifdef CONFIG_PM
-	if (dev->power.runtime_status == RPM_SUSPENDING)
-		return true;
-#endif
-
-	return false;
-}
-
 static void pirq_disable_irq(struct pci_dev *dev)
 {
-	if (io_apic_assign_pci_irqs && !mp_should_keep_irq(&dev->dev) &&
-	    dev->irq_managed && dev->irq) {
+	if (io_apic_assign_pci_irqs && dev->irq_managed && dev->irq) {
 		mp_unmap_irq(dev->irq);
 		dev->irq = 0;
 		dev->irq_managed = 0;
diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index b1def411c0b8..e7f718d6918a 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -485,14 +485,6 @@ void acpi_pci_irq_disable(struct pci_dev *dev)
 	if (!pin || !dev->irq_managed || dev->irq <= 0)
 		return;
 
-	/* Keep IOAPIC pin configuration when suspending */
-	if (dev->dev.power.is_prepared)
-		return;
-#ifdef	CONFIG_PM
-	if (dev->dev.power.runtime_status == RPM_SUSPENDING)
-		return;
-#endif
-
 	entry = acpi_pci_irq_lookup(dev, pin);
 	if (!entry)
 		return;
@@ -513,5 +505,6 @@ void acpi_pci_irq_disable(struct pci_dev *dev)
 	if (gsi >= 0) {
 		acpi_unregister_gsi(gsi);
 		dev->irq_managed = 0;
+		dev->irq = 0;
 	}
 }
-- 
1.7.10.4

[RFC 4/4] PCI: Introduce helpers to manage pci_dev->irq and pci_dev->irq_managed

[Jiang Liu]

Introduce three helpers to manage pci_dev->irq and pci_dev->irq_managed,
which helps to improve maintenance.

Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 arch/x86/pci/intel_mid_pci.c |    4 ++--
 arch/x86/pci/irq.c           |   10 ++++------
 drivers/acpi/pci_irq.c       |   10 ++++------
 include/linux/pci.h          |   17 +++++++++++++++++
 4 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c
index fb7a1f96d80c..22aaefb4f1ca 100644
--- a/arch/x86/pci/intel_mid_pci.c
+++ b/arch/x86/pci/intel_mid_pci.c
@@ -211,7 +211,7 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev)
 	struct irq_alloc_info info;
 	int polarity;
 
-	if (dev->irq_managed && dev->irq > 0)
+	if (pci_has_managed_irq(dev))
 		return 0;
 
 	if (intel_mid_identify_cpu() == INTEL_MID_CPU_CHIP_TANGIER)
@@ -234,7 +234,7 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev)
 
 static void intel_mid_pci_irq_disable(struct pci_dev *dev)
 {
-	if (dev->irq_managed && dev->irq > 0) {
+	if (pci_has_managed_irq(dev)) {
 		mp_unmap_irq(dev->irq);
 		dev->irq_managed = 0;
 		/*
diff --git a/arch/x86/pci/irq.c b/arch/x86/pci/irq.c
index e71b3dbd87b8..a51079a96ded 100644
--- a/arch/x86/pci/irq.c
+++ b/arch/x86/pci/irq.c
@@ -1201,7 +1201,7 @@ static int pirq_enable_irq(struct pci_dev *dev)
 			struct pci_dev *temp_dev;
 			int irq;
 
-			if (dev->irq_managed && dev->irq > 0)
+			if (pci_has_managed_irq(dev))
 				return 0;
 
 			irq = IO_APIC_get_PCI_irq_vector(dev->bus->number,
@@ -1229,8 +1229,7 @@ static int pirq_enable_irq(struct pci_dev *dev)
 			}
 			dev = temp_dev;
 			if (irq >= 0) {
-				dev->irq_managed = 1;
-				dev->irq = irq;
+				pci_set_managed_irq(dev, irq);
 				dev_info(&dev->dev, "PCI->APIC IRQ transform: "
 					 "INT %c -> IRQ %d\n", 'A' + pin - 1, irq);
 				return 0;
@@ -1258,9 +1257,8 @@ static int pirq_enable_irq(struct pci_dev *dev)
 
 static void pirq_disable_irq(struct pci_dev *dev)
 {
-	if (io_apic_assign_pci_irqs && dev->irq_managed && dev->irq) {
+	if (io_apic_assign_pci_irqs && pci_has_managed_irq(dev)) {
 		mp_unmap_irq(dev->irq);
-		dev->irq = 0;
-		dev->irq_managed = 0;
+		pci_reset_managed_irq(dev);
 	}
 }
diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index e7f718d6918a..9d6343d02f7e 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -413,7 +413,7 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
 		return 0;
 	}
 
-	if (dev->irq_managed && dev->irq > 0)
+	if (pci_has_managed_irq(dev))
 		return 0;
 
 	entry = acpi_pci_irq_lookup(dev, pin);
@@ -458,8 +458,7 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
 		kfree(entry);
 		return rc;
 	}
-	dev->irq = rc;
-	dev->irq_managed = 1;
+	pci_set_managed_irq(dev, rc);
 
 	if (link)
 		snprintf(link_desc, sizeof(link_desc), " -> Link[%s]", link);
@@ -482,7 +481,7 @@ void acpi_pci_irq_disable(struct pci_dev *dev)
 	u8 pin;
 
 	pin = dev->pin;
-	if (!pin || !dev->irq_managed || dev->irq <= 0)
+	if (!pin || !pci_has_managed_irq(dev))
 		return;
 
 	entry = acpi_pci_irq_lookup(dev, pin);
@@ -504,7 +503,6 @@ void acpi_pci_irq_disable(struct pci_dev *dev)
 	dev_dbg(&dev->dev, "PCI INT %c disabled\n", pin_name(pin));
 	if (gsi >= 0) {
 		acpi_unregister_gsi(gsi);
-		dev->irq_managed = 0;
-		dev->irq = 0;
+		pci_reset_managed_irq(dev);
 	}
 }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index f50d16a04abc..4bc640eef76f 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -958,6 +958,23 @@ static inline int pci_is_managed(struct pci_dev *pdev)
 	return pdev->is_managed;
 }
 
+static inline void pci_set_managed_irq(struct pci_dev *pdev, unsigned int irq)
+{
+	pdev->irq = irq;
+	pdev->irq_managed = 1;
+}
+
+static inline void pci_reset_managed_irq(struct pci_dev *pdev)
+{
+	pdev->irq = 0;
+	pdev->irq_managed = 0;
+}
+
+static inline bool pci_has_managed_irq(struct pci_dev *pdev)
+{
+	return pdev->irq_managed && pdev->irq > 0;
+}
+
 void pci_disable_device(struct pci_dev *dev);
 
 extern unsigned int pcibios_max_latency;
-- 
1.7.10.4

Re: [RFC 2/4] PCI, MSI: Optionally free legacy PCI IRQ when enabling MSI/MSI-X

[Thomas Gleixner]

On Thu, 7 May 2015, Jiang Liu wrote:

> Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't
> make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again.
> So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate
> when disabling MSI/MSI-X.

This is a bit odd. With your proposed change we'll have:

     alloc_legacy_irq()

     msi[x]_enable()
	free_legacy_irq()

     msi[x]_disable()
	alloc_legacy_irq()

And after that we shut down the device which will free the legacy irq
again.

Shouldn't we allocate the legacy irq only if we really need it?

Thanks,

	tglx

Re: [RFC 0/4] Introduce a mechanism to allocate PCI IRQ on demand

[Thomas Gleixner]

On Thu, 7 May 2015, Jiang Liu wrote:

> This patch set introduces a mechanism to allocate PCI IRQ on demand and
> free it when not used anymore by hooking pci_device_probe() and
> pci_device_remove().
> 
> It will be used to track IOAPIC pin usage on x86 so we could support
> IOAPIC hot-removal.

Bjorn, any opinion on this?

Thanks,

	tglx

Re: [RFC 1/4] PCI: Add hooks to allocate/free IRQ resources when binding/unbinding driver

[Bjorn Helgaas]

On Thu, May 07, 2015 at 11:12:51AM +0800, Jiang Liu wrote:
> Add two hook points pcibios_{alloc|free}_irq() into PCI core, which will
> be called when binding/unbinding PCI device drivers. Then PCI arch code
> may hook into these two points to allocate IRQ resources on demand and
> free them when not used anymore.
> 
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
>  drivers/pci/pci-driver.c |   33 +++++++++++++++++++++++----------
>  include/linux/pci.h      |    2 ++
>  2 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 3cb2210de553..8af4a671686f 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> ...
> +static int pci_device_probe(struct device *dev)
> +{
> +	int error;
> +	struct pci_dev *pci_dev = to_pci_dev(dev);
> +	struct pci_driver *drv = to_pci_driver(dev->driver);
> +
> +	error = pcibios_alloc_irq(pci_dev);
> +	if (error >= 0) {
> +		pci_dev_get(pci_dev);
> +		error = __pci_device_probe(drv, pci_dev);
> +		if (error) {
> +			pcibios_free_irq(pci_dev);
> +			pci_dev_put(pci_dev);
> +		}
> +	}

Please structure it like this so the mainline code doesn't get buried in
the body of the "if":

    irq = pcibios_alloc_irq(pci_dev);
    if (irq < 0)
	return irq;

    pci_dev_get(pci_dev);
    ...

>  
>  	return error;
>  }

Re: [RFC 2/4] PCI, MSI: Optionally free legacy PCI IRQ when enabling MSI/MSI-X

[Bjorn Helgaas]

[+cc Michael]

On Thu, May 07, 2015 at 11:12:52AM +0800, Jiang Liu wrote:
> Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't
> make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again.
> So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate
> when disabling MSI/MSI-X.
> 
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
>  drivers/pci/msi.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index c3e7dfcf9ff5..47cf72c669f0 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -686,6 +686,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
>  	msi_set_enable(dev, 1);
>  	dev->msi_enabled = 1;
>  
> +	pcibios_free_irq(dev);

>From the recent discussion about disabling MSI at shutdown, I seem to
recall that turning off bus mastering also effectively disables MSI and
causes at least some devices to revert to using INTx.

So with this patch, do we now have a problem in the following scenario?

    pci_enable_msi(dev);	/* now frees INTx IRQ */
    pci_clear_master(dev);
    <device interrupts>		/* uses INTx since bus mastering disabled */

>  	dev->irq = entry->irq;
>  	return 0;
>  }
> @@ -813,9 +814,10 @@ static int msix_capability_init(struct pci_dev *dev,
>  	/* Set MSI-X enabled bits and unmask the function */
>  	pci_intx_for_msi(dev, 0);
>  	dev->msix_enabled = 1;
> -
>  	msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
>  
> +	pcibios_free_irq(dev);
> +
>  	return 0;
>  
>  out_avail:
> @@ -930,6 +932,7 @@ void pci_msi_shutdown(struct pci_dev *dev)
>  
>  	/* Restore dev->irq to its default pin-assertion irq */
>  	dev->irq = desc->msi_attrib.default_irq;
> +	pcibios_alloc_irq(dev);
>  }
>  
>  void pci_disable_msi(struct pci_dev *dev)
> @@ -1030,6 +1033,7 @@ void pci_msix_shutdown(struct pci_dev *dev)
>  	msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
>  	pci_intx_for_msi(dev, 1);
>  	dev->msix_enabled = 0;
> +	pcibios_alloc_irq(dev);
>  }
>  
>  void pci_disable_msix(struct pci_dev *dev)
> -- 
> 1.7.10.4
> 

Re: [RFC 2/4] PCI, MSI: Optionally free legacy PCI IRQ when enabling MSI/MSI-X

[Michael S. Tsirkin]

On Tue, May 19, 2015 at 10:08:38AM -0500, Bjorn Helgaas wrote:
> [+cc Michael]
> 
> On Thu, May 07, 2015 at 11:12:52AM +0800, Jiang Liu wrote:
> > Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't
> > make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again.
> > So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate
> > when disabling MSI/MSI-X.
> > 
> > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> > ---
> >  drivers/pci/msi.c |    6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index c3e7dfcf9ff5..47cf72c669f0 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -686,6 +686,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
> >  	msi_set_enable(dev, 1);
> >  	dev->msi_enabled = 1;
> >  
> > +	pcibios_free_irq(dev);
> 
> >From the recent discussion about disabling MSI at shutdown, I seem to
> recall that turning off bus mastering also effectively disables MSI and
> causes at least some devices to revert to using INTx.
> 
> So with this patch, do we now have a problem in the following scenario?

Can't say, I didn't follow the discussion. But I'll address the
comment below:

>     pci_enable_msi(dev);	/* now frees INTx IRQ */
>     pci_clear_master(dev);
>     <device interrupts>		/* uses INTx since bus mastering disabled */

Not really, this does not use INTx. Disabling bus master shuts down MSI
but does not enable INTx, you effectively get no interrupts at all: MSI
disabled INTx and bus master disabled MSI.


> >  	dev->irq = entry->irq;
> >  	return 0;
> >  }
> > @@ -813,9 +814,10 @@ static int msix_capability_init(struct pci_dev *dev,
> >  	/* Set MSI-X enabled bits and unmask the function */
> >  	pci_intx_for_msi(dev, 0);
> >  	dev->msix_enabled = 1;
> > -
> >  	msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
> >  
> > +	pcibios_free_irq(dev);
> > +
> >  	return 0;
> >  
> >  out_avail:
> > @@ -930,6 +932,7 @@ void pci_msi_shutdown(struct pci_dev *dev)
> >  
> >  	/* Restore dev->irq to its default pin-assertion irq */
> >  	dev->irq = desc->msi_attrib.default_irq;
> > +	pcibios_alloc_irq(dev);
> >  }
> >  
> >  void pci_disable_msi(struct pci_dev *dev)
> > @@ -1030,6 +1033,7 @@ void pci_msix_shutdown(struct pci_dev *dev)
> >  	msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
> >  	pci_intx_for_msi(dev, 1);
> >  	dev->msix_enabled = 0;
> > +	pcibios_alloc_irq(dev);
> >  }
> >  
> >  void pci_disable_msix(struct pci_dev *dev)
> > -- 
> > 1.7.10.4
> > 

Re: [RFC 2/4] PCI, MSI: Optionally free legacy PCI IRQ when enabling MSI/MSI-X

[Michael S. Tsirkin]

On Tue, May 19, 2015 at 05:16:54PM +0200, Michael S. Tsirkin wrote:
> On Tue, May 19, 2015 at 10:08:38AM -0500, Bjorn Helgaas wrote:
> > [+cc Michael]
> > 
> > On Thu, May 07, 2015 at 11:12:52AM +0800, Jiang Liu wrote:
> > > Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't
> > > make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again.
> > > So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate
> > > when disabling MSI/MSI-X.
> > > 
> > > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> > > ---
> > >  drivers/pci/msi.c |    6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > > index c3e7dfcf9ff5..47cf72c669f0 100644
> > > --- a/drivers/pci/msi.c
> > > +++ b/drivers/pci/msi.c
> > > @@ -686,6 +686,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
> > >  	msi_set_enable(dev, 1);
> > >  	dev->msi_enabled = 1;
> > >  
> > > +	pcibios_free_irq(dev);
> > 
> > >From the recent discussion about disabling MSI at shutdown, I seem to
> > recall that turning off bus mastering also effectively disables MSI and
> > causes at least some devices to revert to using INTx.

So to stress this point, turning off bus mastering also effectively
disables MSI without making devices revert to using INTx.
This is why clearing bus master is the right thing to do
if you don't want any interrupts at all.


> > So with this patch, do we now have a problem in the following scenario?
> 
> Can't say, I didn't follow the discussion. But I'll address the
> comment below:
> 
> >     pci_enable_msi(dev);	/* now frees INTx IRQ */
> >     pci_clear_master(dev);
> >     <device interrupts>		/* uses INTx since bus mastering disabled */
> 
> Not really, this does not use INTx. Disabling bus master shuts down MSI
> but does not enable INTx, you effectively get no interrupts at all: MSI
> disabled INTx and bus master disabled MSI.
> 
> 
> > >  	dev->irq = entry->irq;
> > >  	return 0;
> > >  }
> > > @@ -813,9 +814,10 @@ static int msix_capability_init(struct pci_dev *dev,
> > >  	/* Set MSI-X enabled bits and unmask the function */
> > >  	pci_intx_for_msi(dev, 0);
> > >  	dev->msix_enabled = 1;
> > > -
> > >  	msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
> > >  
> > > +	pcibios_free_irq(dev);
> > > +
> > >  	return 0;
> > >  
> > >  out_avail:
> > > @@ -930,6 +932,7 @@ void pci_msi_shutdown(struct pci_dev *dev)
> > >  
> > >  	/* Restore dev->irq to its default pin-assertion irq */
> > >  	dev->irq = desc->msi_attrib.default_irq;
> > > +	pcibios_alloc_irq(dev);
> > >  }
> > >  
> > >  void pci_disable_msi(struct pci_dev *dev)
> > > @@ -1030,6 +1033,7 @@ void pci_msix_shutdown(struct pci_dev *dev)
> > >  	msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
> > >  	pci_intx_for_msi(dev, 1);
> > >  	dev->msix_enabled = 0;
> > > +	pcibios_alloc_irq(dev);
> > >  }
> > >  
> > >  void pci_disable_msix(struct pci_dev *dev)
> > > -- 
> > > 1.7.10.4
> > > 

Re: [RFC 2/4] PCI, MSI: Optionally free legacy PCI IRQ when enabling MSI/MSI-X

[Bjorn Helgaas]

On Thu, May 07, 2015 at 11:12:52AM +0800, Jiang Liu wrote:
> Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't
> make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again.
> So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate
> when disabling MSI/MSI-X.

The rest of this series makes sense to me.  If you want to remove an
IOAPIC, you want to make sure all of the IRQs using that IOAPIC have been
freed.

But I'm trying to figure out this patch.  Do you want to free the IRQ when
enabling MSI because it enables you to remove the IOAPIC without removing
the device?  That wouldn't really make sense to me because then the device
has no possibility of using INTx.

Bjorn

> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
>  drivers/pci/msi.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index c3e7dfcf9ff5..47cf72c669f0 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -686,6 +686,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
>  	msi_set_enable(dev, 1);
>  	dev->msi_enabled = 1;
>  
> +	pcibios_free_irq(dev);
>  	dev->irq = entry->irq;
>  	return 0;
>  }
> @@ -813,9 +814,10 @@ static int msix_capability_init(struct pci_dev *dev,
>  	/* Set MSI-X enabled bits and unmask the function */
>  	pci_intx_for_msi(dev, 0);
>  	dev->msix_enabled = 1;
> -
>  	msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
>  
> +	pcibios_free_irq(dev);
> +
>  	return 0;
>  
>  out_avail:
> @@ -930,6 +932,7 @@ void pci_msi_shutdown(struct pci_dev *dev)
>  
>  	/* Restore dev->irq to its default pin-assertion irq */
>  	dev->irq = desc->msi_attrib.default_irq;
> +	pcibios_alloc_irq(dev);
>  }
>  
>  void pci_disable_msi(struct pci_dev *dev)
> @@ -1030,6 +1033,7 @@ void pci_msix_shutdown(struct pci_dev *dev)
>  	msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
>  	pci_intx_for_msi(dev, 1);
>  	dev->msix_enabled = 0;
> +	pcibios_alloc_irq(dev);
>  }
>  
>  void pci_disable_msix(struct pci_dev *dev)
> -- 
> 1.7.10.4
> 

Re: [RFC 2/4] PCI, MSI: Optionally free legacy PCI IRQ when enabling MSI/MSI-X

[Jiang Liu]

On 2015/5/16 5:02, Thomas Gleixner wrote:
> On Thu, 7 May 2015, Jiang Liu wrote:
> 
>> Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't
>> make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again.
>> So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate
>> when disabling MSI/MSI-X.
> 
> This is a bit odd. With your proposed change we'll have:
> 
>      alloc_legacy_irq()
> 
>      msi[x]_enable()
> 	free_legacy_irq()
> 
>      msi[x]_disable()
> 	alloc_legacy_irq()
Hi Thomas,
	It's for safety. I'm not sure whether the device driver will
make use of legacy IRQ after calling msi[x]_disable(). I have concerns
about following pattern in PCI device drivers:
---------------------------------------------------
if (enable_msi() == SUCCESS) {
	if (allocate_resource_for_msi() == SUCCESS)
		return;
	disable_msi();
}
use_legacy_irq()

Thanks!
Gerry

> 
> And after that we shut down the device which will free the legacy irq
> again.
> 
> Shouldn't we allocate the legacy irq only if we really need it?
> 
> Thanks,
> 
> 	tglx
> 

Re: [RFC 2/4] PCI, MSI: Optionally free legacy PCI IRQ when enabling MSI/MSI-X

[Jiang Liu]

On 2015/5/19 23:26, Michael S. Tsirkin wrote:
> On Tue, May 19, 2015 at 05:16:54PM +0200, Michael S. Tsirkin wrote:
>> On Tue, May 19, 2015 at 10:08:38AM -0500, Bjorn Helgaas wrote:
>>> [+cc Michael]
>>>
>>> On Thu, May 07, 2015 at 11:12:52AM +0800, Jiang Liu wrote:
>>>> Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't
>>>> make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again.
>>>> So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate
>>>> when disabling MSI/MSI-X.
>>>>
>>>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>>>> ---
>>>>  drivers/pci/msi.c |    6 +++++-
>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>>>> index c3e7dfcf9ff5..47cf72c669f0 100644
>>>> --- a/drivers/pci/msi.c
>>>> +++ b/drivers/pci/msi.c
>>>> @@ -686,6 +686,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
>>>>  	msi_set_enable(dev, 1);
>>>>  	dev->msi_enabled = 1;
>>>>  
>>>> +	pcibios_free_irq(dev);
>>>
>>> >From the recent discussion about disabling MSI at shutdown, I seem to
>>> recall that turning off bus mastering also effectively disables MSI and
>>> causes at least some devices to revert to using INTx.
> 
> So to stress this point, turning off bus mastering also effectively
> disables MSI without making devices revert to using INTx.
> This is why clearing bus master is the right thing to do
> if you don't want any interrupts at all.

Thanks for explanation, Michael:)

Re: [RFC 2/4] PCI, MSI: Optionally free legacy PCI IRQ when enabling MSI/MSI-X

[Jiang Liu]

On 2015/5/20 5:39, Bjorn Helgaas wrote:
> On Thu, May 07, 2015 at 11:12:52AM +0800, Jiang Liu wrote:
>> Once PCI MSI/MSI-X is enabled by the device driver, PCI device won't
>> make use of legacy PCI IRQ until PCI MSI/MSI-X is disabled again.
>> So optionally free legacy PCI IRQ when enabling MSI/MSI-X and reallocate
>> when disabling MSI/MSI-X.
> 
> The rest of this series makes sense to me.  If you want to remove an
> IOAPIC, you want to make sure all of the IRQs using that IOAPIC have been
> freed.
> 
> But I'm trying to figure out this patch.  Do you want to free the IRQ when
> enabling MSI because it enables you to remove the IOAPIC without removing
> the device?  That wouldn't really make sense to me because then the device
> has no possibility of using INTx.
Hi Bjorn
	Sorry for the confusion. This patch is not for IOAPIC
hot-removal, but it's an effort to use IRQ resource precisely.
The idea is to allocate IRQ resource on demand and release it
when not used. We could skip this patch if it's not worth the
cost.
Thanks!
Gerry

Re: [RFC 0/4] Introduce a mechanism to allocate PCI IRQ on demand

[Jiang Liu]

On 2015/5/19 21:35, Thomas Gleixner wrote:
> On Thu, 7 May 2015, Jiang Liu wrote:
> 
>> This patch set introduces a mechanism to allocate PCI IRQ on demand and
>> free it when not used anymore by hooking pci_device_probe() and
>> pci_device_remove().
>>
>> It will be used to track IOAPIC pin usage on x86 so we could support
>> IOAPIC hot-removal.
> 
> Bjorn, any opinion on this?
Hi Bjorn,
	With this patch applied, how about removing this workaround
from arch/x86/pci/irq.c?
-------------------------------------------------------------------
        if (io_apic_assign_pci_irqs && pci_routeirq) {
                struct pci_dev *dev = NULL;
                /*
                 * PCI IRQ routing is set up by pci_enable_device(), but we
                 * also do it here in case there are still broken
drivers that
                 * don't use pci_enable_device().
                 */
                printk(KERN_INFO "PCI: Routing PCI interrupts for all
devices because \"pci=routeirq\" specified\n");
                for_each_pci_dev(dev)
                        pirq_enable_irq(dev);
        }
-------------------------------------------------------
Thanks!
Gerry

> 
> Thanks,
> 
> 	tglx
> 

Re: [RFC 2/4] PCI, MSI: Optionally free legacy PCI IRQ when enabling MSI/MSI-X

[Thomas Gleixner]

On Wed, 20 May 2015, Jiang Liu wrote:
> On 2015/5/16 5:02, Thomas Gleixner wrote:
> > This is a bit odd. With your proposed change we'll have:
> > 
> >      alloc_legacy_irq()
> > 
> >      msi[x]_enable()
> > 	free_legacy_irq()
> > 
> >      msi[x]_disable()
> > 	alloc_legacy_irq()
> Hi Thomas,
> 	It's for safety. I'm not sure whether the device driver will
> make use of legacy IRQ after calling msi[x]_disable(). I have concerns
> about following pattern in PCI device drivers:
> ---------------------------------------------------
> if (enable_msi() == SUCCESS) {
> 	if (allocate_resource_for_msi() == SUCCESS)
> 		return;
> 	disable_msi();
> }
> use_legacy_irq()

Fair enough.

     tglx