x86/platform/intel-mid: Make IRQ allocation a bit more flexible

In the future we would use dynamic allocation for IRQ which brings non-1:1 mapping for IOAPIC domain. Thus, we need to respect return value of mp_map_gsi_to_irq() and assign it back to the device structure.

Besides that we need to read GSI from interrupt pin register to avoid cases when some drivers will try to initialize PCI device twice in a row which will call pcibios_enable_irq() twice as well.

	serial 0000:00:04.1: Mapped GSI28 to IRQ5
	serial 0000:00:04.2: Mapped GSI29 to IRQ5
	serial 0000:00:04.3: Mapped GSI54 to IRQ5
	8250_mid 0000:00:04.1: Mapped GSI28 to IRQ5
	8250_mid 0000:00:04.2: Mapped GSI29 to IRQ6
	8250_mid 0000:00:04.3: Mapped GSI54 to IRQ7

Signed-off-by Andy Shevchenko <[email protected]>

---
 arch/x86/pci/intel_mid_pci.c                                 | 12 ++++++++++--
 arch/x86/platform/intel-mid/device_libs/platform_mrfld_wdt.c |  6 +++---
 2 files changed, 13 insertions(+), 5 deletions(-)

arch/x86/pci/intel_mid_pci.c +10 -2

index 5a18aedcb341..b901ece278dd 100644
@@ -215,16 +215,23 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev)
 	struct irq_alloc_info info;
 	int polarity;
 	int ret;
+	u8 gsi;
 
 	if (dev->irq_managed && dev->irq > 0)
 		return 0;
 
+	ret = pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &gsi);
+	if (ret < 0) {
+		dev_warn(&dev->dev, "Failed to read interrupt line: %d\n", ret);
+		return ret;
+	}
+
 	switch (intel_mid_identify_cpu()) {
 	case INTEL_MID_CPU_CHIP_TANGIER:
 		polarity = IOAPIC_POL_HIGH;
 
 		/* Special treatment for IRQ0 */
-		if (dev->irq == 0) {
+		if (gsi == 0) {
 			/*
 			 * Skip HS UART common registers device since it has
 			 * IRQ0 assigned and not used by the kernel.

@@ -253,10 +260,11 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev)
 	 * MRST only have IOAPIC, the PCI irq lines are 1:1 mapped to
 	 * IOAPIC RTE entries, so we just enable RTE for the device.
 	 */
-	ret = mp_map_gsi_to_irq(dev->irq, IOAPIC_MAP_ALLOC, &info);
+	ret = mp_map_gsi_to_irq(gsi, IOAPIC_MAP_ALLOC, &info);
 	if (ret < 0)
 		return ret;
 
+	dev->irq = ret;
 	dev->irq_managed = 1;
 
 	return 0;

arch/x86/platform/intel-mid/device_libs/platform_mrfld_wdt.c +3 -3

index 9e304e2ea4f5..4f5fa65a1011 100644
@@ -30,13 +30,13 @@ static int tangier_probe(struct platform_device *pdev)
 {
 	struct irq_alloc_info info;
 	struct intel_mid_wdt_pdata *pdata = pdev->dev.platform_data;
-	int gsi, irq;
+	int gsi = TANGIER_EXT_TIMER0_MSI;
+	int irq;
 
 	if (!pdata)
 		return -EINVAL;
 
 	/* IOAPIC builds identity mapping between GSI and IRQ on MID */
-	gsi = pdata->irq;
 	ioapic_set_alloc_attr(&info, cpu_to_node(0), 1, 0);
 	irq = mp_map_gsi_to_irq(gsi, IOAPIC_MAP_ALLOC, &info);
 	if (irq < 0) {

@@ -44,11 +44,11 @@ static int tangier_probe(struct platform_device *pdev)
 		return irq;
 	}
 
+	pdata->irq = irq;
 	return 0;
 }
 
 static struct intel_mid_wdt_pdata tangier_pdata = {
-	.irq = TANGIER_EXT_TIMER0_MSI,
 	.probe = tangier_probe,
 };
 

Signature · 1 line

2.13.2

Quoted 5 lines · Andy Shevchenko

In the future we would use dynamic allocation for IRQ which brings non-1:1 mapping for IOAPIC domain. Thus, we need to respect return value
of mp_map_gsi_to_irq() and assign it back to the device structure.

Quoted 5 lines

Besides that we need to read GSI from interrupt pin register to avoid
cases when some drivers will try to initialize PCI device twice in a row
which will call pcibios_enable_irq() twice as well.

This seems sort of suspect.  It doesn't seem robust to rely on the value of PCI_INTERRUPT_LINE being zero before pcibios_enable_irq().

Can't we make pcibios_enable_irq() idempotent?  I guess I don't understand the real problem here.

Is this really two separate patches that could be separated?

Quoted 107 lines

	serial 0000:00:04.1: Mapped GSI28 to IRQ5
	serial 0000:00:04.2: Mapped GSI29 to IRQ5
	serial 0000:00:04.3: Mapped GSI54 to IRQ5
	8250_mid 0000:00:04.1: Mapped GSI28 to IRQ5
	8250_mid 0000:00:04.2: Mapped GSI29 to IRQ6
	8250_mid 0000:00:04.3: Mapped GSI54 to IRQ7

Signed-off-by Andy Shevchenko <[email protected]>

---
 arch/x86/pci/intel_mid_pci.c                                 | 12
++++++++++--
 arch/x86/platform/intel-mid/device_libs/platform_mrfld_wdt.c |  6
+++---

 2 files changed, 13 insertions(+), 5 deletions(-)

arch/x86/pci/intel_mid_pci.c +1 -0

index 5a18aedcb341..b901ece278dd 100644
@@ -215,16 +215,23 @@ static int intel_mid_pci_irq_enable(struct
 pci_dev *dev)
  	struct irq_alloc_info info;
  	int polarity;
  	int ret;
+	u8 gsi;

  if (dev->irq_managed && dev->irq > 0)
  return 0;

arch/x86/pci/intel_mid_pci.c +3 -0

+	ret = pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &gsi);
+	if (ret < 0) {
+		dev_warn(&dev->dev, "Failed to read interrupt line:

%d\n", ret);

arch/x86/pci/intel_mid_pci.c +3 -0

+		return ret;
+	}
+

  switch (intel_mid_identify_cpu()) {
  case INTEL_MID_CPU_CHIP_TANGIER:
  polarity = IOAPIC_POL_HIGH;

  /* Special treatment for IRQ0 */

arch/x86/pci/intel_mid_pci.c +1 -1

-		if (dev->irq == 0) {
+		if (gsi == 0) {

  /*
   * Skip HS UART common registers device since it has
   * IRQ0 assigned and not used by the kernel.

arch/x86/pci/intel_mid_pci.c +0 -0

@@ -253,10 +260,11 @@ static int intel_mid_pci_irq_enable(struct
 pci_dev *dev)
  	 * MRST only have IOAPIC, the PCI irq lines are 1:1 mapped

to
   * IOAPIC RTE entries, so we just enable RTE for the device.
   */

arch/x86/pci/intel_mid_pci.c +1 -1

-	ret = mp_map_gsi_to_irq(dev->irq, IOAPIC_MAP_ALLOC, &info);
+	ret = mp_map_gsi_to_irq(gsi, IOAPIC_MAP_ALLOC, &info);

  if (ret < 0)
  return ret;

arch/x86/pci/intel_mid_pci.c +1 -0

+	dev->irq = ret;

  dev->irq_managed = 1;

  return 0;

(quoted diff) +0 -0

mid/device_libs/platform_mrfld_wdt.c b/arch/x86/platform/intel- mid/device_libs/platform_mrfld_wdt.c
index 9e304e2ea4f5..4f5fa65a1011 100644

arch/x86/platform/intel-mid/device_libs/platform_mrfld_wdt.c +0 -0

@@ -30,13 +30,13 @@ static int tangier_probe(struct platform_device
 *pdev)
  {
  	struct irq_alloc_info info;

  struct intel_mid_wdt_pdata *pdata = pdev-

Quoted 1 line

dev.platform_data;

arch/x86/platform/intel-mid/device_libs/platform_mrfld_wdt.c +2 -1

-	int gsi, irq;
+	int gsi = TANGIER_EXT_TIMER0_MSI;
+	int irq;

  if (!pdata)
  return -EINVAL;

  /* IOAPIC builds identity mapping between GSI and IRQ on MID */

arch/x86/platform/intel-mid/device_libs/platform_mrfld_wdt.c +0 -1

-	gsi = pdata->irq;

  ioapic_set_alloc_attr(&info, cpu_to_node(0), 1, 0); irq = mp_map_gsi_to_irq(gsi, IOAPIC_MAP_ALLOC, &info); if (irq < 0) {

arch/x86/platform/intel-mid/device_libs/platform_mrfld_wdt.c +0 -0

@@ -44,11 +44,11 @@ static int tangier_probe(struct platform_device
 *pdev)
  		return irq;
  	}

arch/x86/platform/intel-mid/device_libs/platform_mrfld_wdt.c +1 -0

+	pdata->irq = irq;

  return 0;
 }

 static struct intel_mid_wdt_pdata tangier_pdata = {

arch/x86/platform/intel-mid/device_libs/platform_mrfld_wdt.c +0 -1

-	.irq = TANGIER_EXT_TIMER0_MSI,

  .probe = tangier_probe,
 };

--
2.13.2

Quoted 12 lines · Bjorn Helgaas

Quoted 10 lines · Andy Shevchenko

In the future we would use dynamic allocation for IRQ which brings non-1:1 mapping for IOAPIC domain. Thus, we need to respect return value
of mp_map_gsi_to_irq() and assign it back to the device structure.

Besides that we need to read GSI from interrupt pin register to avoid
cases when some drivers will try to initialize PCI device twice in a row
which will call pcibios_enable_irq() twice as well.

Quoted 2 lines

This seems sort of suspect.  It doesn't seem robust to rely on the value of PCI_INTERRUPT_LINE being zero before pcibios_enable_irq().

So, you are telling that it's possible to get pcibios_enable_irq() called with PCI_INTERRUPT_LINE == 0?

Quoted 2 lines

Can't we make pcibios_enable_irq() idempotent?  I guess I don't understand the real problem here.

Currently there is no problem here.

Firmware assigns IRQ line and wrote this to the configuration space. Intel MID uses 1:1 mapping (IRQ domain is STRICT), so, it just allocates a vector inside kernel with the very same number.

But...

If we switch to dynamic allocation (it's a default for ACPI case), _on this platform_ we will get wrong allocation in the IOAPIC since mapping is not 1:1 anymore.

That's what I'm trying to avoid (dev->irq after this patch points correctly to dynamically allocated number).

For old SFI enabled platforms it will not make any change.

Note, there is no legacy PIC on this platform (as same case as for ACPI HW reduced platforms), just IOAPIC.

Quoted 1 line

Is this really two separate patches that could be separated?

I didn't get this, what separation you see might be applied here?

Quoted 21 lines · Andy Shevchenko

Quoted 19 lines · Bjorn Helgaas

Quoted 8 lines · Andy Shevchenko

In the future we would use dynamic allocation for IRQ which brings
non-1:1 mapping for IOAPIC domain. Thus, we need to respect return
value
of mp_map_gsi_to_irq() and assign it back to the device structure.

Quoted 6 lines

Besides that we need to read GSI from interrupt pin register to avoid
cases when some drivers will try to initialize PCI device twice in a
row
which will call pcibios_enable_irq() twice as well.

This seems sort of suspect.  It doesn't seem robust to rely on the value of PCI_INTERRUPT_LINE being zero before pcibios_enable_irq().

Quoted 2 lines

So, you are telling that it's possible to get pcibios_enable_irq() called with PCI_INTERRUPT_LINE == 0?

I don't know about that.  I'm just saying that it sounds like you're relying on some condition that is hard to verify.  PCI_INTERRUPT_LINE is read/write and it's hard to know what, if anything, BIOS did with it.

Quoted 29 lines

Quoted 2 lines

Can't we make pcibios_enable_irq() idempotent?  I guess I don't understand the real problem here.

Currently there is no problem here.

Firmware assigns IRQ line and wrote this to the configuration space. Intel MID uses 1:1 mapping (IRQ domain is STRICT), so, it just allocates
a vector inside kernel with the very same number.

But...

If we switch to dynamic allocation (it's a default for ACPI case), _on
this platform_ we will get wrong allocation in the IOAPIC since mapping
is not 1:1 anymore.

That's what I'm trying to avoid (dev->irq after this patch points correctly to dynamically allocated number).

For old SFI enabled platforms it will not make any change.

Note, there is no legacy PIC on this platform (as same case as for ACPI
HW reduced platforms), just IOAPIC.

Quoted 1 line

Is this really two separate patches that could be separated?

Quoted 1 line

I didn't get this, what separation you see might be applied here?

The "Besides that" in your changelog suggested that "here is another change lumped into this patch."  I was wondering if the "read GSI from PCI_INTERRUPT_LINE" part could be separated out.  That makes me wonder whether this is really safe, since GSIs are 32 bits, but PCI_INTERRUPT_LINE is only 8 bits.  I don't know enough to know why this is safe in this case.

Quoted 22 lines · Bjorn Helgaas

Quoted 21 lines · Andy Shevchenko

Quoted 19 lines · Bjorn Helgaas

Quoted 8 lines · Andy Shevchenko

In the future we would use dynamic allocation for IRQ which brings
non-1:1 mapping for IOAPIC domain. Thus, we need to respect return
value
of mp_map_gsi_to_irq() and assign it back to the device structure.

Quoted 6 lines

Besides that we need to read GSI from interrupt pin register to avoid
cases when some drivers will try to initialize PCI device twice in a
row
which will call pcibios_enable_irq() twice as well.

This seems sort of suspect.  It doesn't seem robust to rely on the value of PCI_INTERRUPT_LINE being zero before pcibios_enable_irq().

Quoted 7 lines

Quoted 2 lines

So, you are telling that it's possible to get pcibios_enable_irq() called with PCI_INTERRUPT_LINE == 0?

I don't know about that.  I'm just saying that it sounds like you're relying on some condition that is hard to verify.  PCI_INTERRUPT_LINE is read/write and it's hard to know what, if anything, BIOS did with it.

_On this platform_ BIOS provides a static mapping for IRQ lines (there is no separate PIR, just IOAPIC). So, that register is initialized by firmware. We are not suppose to touch it _on this platform_.

Quoted 29 lines

Quoted 29 lines

Quoted 2 lines

Can't we make pcibios_enable_irq() idempotent?  I guess I don't understand the real problem here.

Currently there is no problem here.

Firmware assigns IRQ line and wrote this to the configuration space. Intel MID uses 1:1 mapping (IRQ domain is STRICT), so, it just allocates
a vector inside kernel with the very same number.

But...

If we switch to dynamic allocation (it's a default for ACPI case), _on
this platform_ we will get wrong allocation in the IOAPIC since mapping
is not 1:1 anymore.

That's what I'm trying to avoid (dev->irq after this patch points correctly to dynamically allocated number).

For old SFI enabled platforms it will not make any change.

Note, there is no legacy PIC on this platform (as same case as for ACPI
HW reduced platforms), just IOAPIC.

Quoted 1 line

Is this really two separate patches that could be separated?

Quoted 8 lines

Quoted 1 line

I didn't get this, what separation you see might be applied here?

The "Besides that" in your changelog suggested that "here is another change lumped into this patch."  I was wondering if the "read GSI from PCI_INTERRUPT_LINE" part could be separated out.  That makes me wonder whether this is really safe, since GSIs are 32 bits, but PCI_INTERRUPT_LINE is only 8 bits.  I don't know enough to know why this is safe in this case.

"Besides that" part means "we have a second circumstance we have to follow to avoid breaking working platforms".

The old code, if you look into patch, uses dev->irq as a predefined constant, which is not in case of dynamic allocation, so, if we respect return value from mp_map_gsi_to_irq() we may not use dev->irq as _source_ of IRQ mapping anymore.

I didn't check if we can do things in reversed order than I have done, i.e. apply part 2 as separate change and then respect the return value. It looks like it may be done in this way.

However, the patch is applied and I don't see any strong reason to revert it and bring again as a set of two patches.