[PATCH 0/5] x86, MSI, AHCI: Support multiple MSIs

[PATCH 0/5] x86, MSI, AHCI: Support multiple MSIs

[Alexander Gordeev]

Currently multiple MSI mode is limited to a single vector per device (at
least on x86 and PPC). This series breathes life into pci_enable_msi_block()
and makes it possible to set interrupt affinity for multiple IRQs, similarly
to MSI-X. Yet, only for x86 and only when IOMMUs are present.

Although IRQ and PCI subsystems are modified, the current behaviour left
intact. The drivers could just start using multiple MSIs just by following
the existing documentation.

The AHCI device driver makes use of the new mode and a new function -
pci_enable_msi_block_auto() - patches 4,5.

The series is adapted to Ingo's -tip repository, x86/apic branch.
Patches 4,5 could be applied independently of patches 1-3.

Attached patches:
  1/5 x86, MSI: Support multiple MSIs in presense of IRQ remapping
  2/5 x86, MSI: Allocate as many multiple IRQs as requested
  3/5 x86, MSI: Minor readability fixes
  4/5 PCI, MSI: Enable multiple MSIs with pci_enable_msi_block_auto()
  5/5 AHCI: Support multiple MSIs

 Documentation/PCI/MSI-HOWTO.txt |   41 ++++++++-
 arch/x86/kernel/apic/io_apic.c  |  170 ++++++++++++++++++++++++++++++++++++---
 drivers/ata/ahci.c              |   11 +--
 drivers/ata/ahci.h              |    1 +
 drivers/ata/libahci.c           |   24 ++++++
 drivers/ata/libata-core.c       |   66 +++++++++++++++
 drivers/pci/msi.c               |   39 +++++++++-
 include/linux/irq.h             |    6 ++
 include/linux/libata.h          |    3 +
 include/linux/msi.h             |    1 +
 include/linux/pci.h             |    7 ++
 kernel/irq/chip.c               |   30 +++++--
 kernel/irq/irqdesc.c            |   31 +++++++
 13 files changed, 397 insertions(+), 33 deletions(-)

-- 
1.7.7.6


-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH 1/5] x86, MSI: Support multiple MSIs in presense of IRQ remapping

[Alexander Gordeev]

The MSI specification has several constraints in comparison with MSI-X,
most notable of them is the inability to configure MSIs independently.
As a result, it is impossible to dispatch interrupts from different
queues to different CPUs. This is largely devalues the support of
multiple MSIs in SMP systems.

Also, a necessity to allocate a contiguous block of vector numbers for
devices capable of multiple MSIs might cause a considerable pressure on
x86 interrupt vector allocator and could lead to fragmentation of the
interrupt vectors space.

This patch overcomes both drawbacks in presense of IRQ remapping and
lets devices take advantage of multiple queues and per-IRQ affinity
assignments.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 arch/x86/kernel/apic/io_apic.c |  166 +++++++++++++++++++++++++++++++++++++--
 include/linux/irq.h            |    6 ++
 kernel/irq/chip.c              |   30 +++++--
 kernel/irq/irqdesc.c           |   31 ++++++++
 4 files changed, 216 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index a951ef7..f083049 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -305,6 +305,11 @@ static int alloc_irq_from(unsigned int from, int node)
 	return irq_alloc_desc_from(from, node);
 }
 
+static int alloc_irqs_from(unsigned int from, unsigned int count, int node)
+{
+	return irq_alloc_descs_from(from, count, node);
+}
+
 static void free_irq_at(unsigned int at, struct irq_cfg *cfg)
 {
 	free_irq_cfg(at, cfg);
@@ -3030,6 +3035,55 @@ int create_irq(void)
 	return irq;
 }
 
+unsigned int create_irqs(unsigned int from, unsigned int count, int node)
+{
+	struct irq_cfg **cfg;
+	unsigned long flags;
+	int irq, i;
+
+	if (from < nr_irqs_gsi)
+		from = nr_irqs_gsi;
+
+	cfg = kzalloc_node(count * sizeof(cfg[0]), GFP_KERNEL, node);
+	if (!cfg)
+		return 0;
+
+	irq = alloc_irqs_from(from, count, node);
+	if (irq < 0)
+		goto out_cfgs;
+
+	for (i = 0; i < count; i++) {
+		cfg[i] = alloc_irq_cfg(irq + i, node);
+		if (!cfg[i])
+			goto out_irqs;
+	}
+
+	raw_spin_lock_irqsave(&vector_lock, flags);
+	for (i = 0; i < count; i++)
+		if (__assign_irq_vector(irq + i, cfg[i], apic->target_cpus()))
+			goto out_vecs;
+	raw_spin_unlock_irqrestore(&vector_lock, flags);
+
+	for (i = 0; i < count; i++) {
+		irq_set_chip_data(irq + i, cfg[i]);
+		irq_clear_status_flags(irq + i, IRQ_NOREQUEST);
+	}
+
+	kfree(cfg);
+	return irq;
+
+out_vecs:
+	for (; i; i--)
+		__clear_irq_vector(irq + i - 1, cfg[i - 1]);
+	raw_spin_unlock_irqrestore(&vector_lock, flags);
+out_irqs:
+	for (i = 0; i < count; i++)
+		free_irq_at(irq + i, cfg[i]);
+out_cfgs:
+	kfree(cfg);
+	return 0;
+}
+
 void destroy_irq(unsigned int irq)
 {
 	struct irq_cfg *cfg = irq_get_chip_data(irq);
@@ -3045,6 +3099,27 @@ void destroy_irq(unsigned int irq)
 	free_irq_at(irq, cfg);
 }
 
+static inline void destroy_irqs(unsigned int irq, unsigned int count)
+{
+	unsigned int i;
+	for (i = 0; i < count; i++)
+		destroy_irq(irq + i);
+}
+
+static inline int
+can_create_pow_of_two_irqs(unsigned int from, unsigned int count)
+{
+	if ((count > 1) && (count % 2))
+		return -EINVAL;
+
+	for (; count; count = count / 2) {
+		if (!irq_can_alloc_irqs(from, count))
+			return count;
+	}
+
+	return -ENOSPC;
+}
+
 /*
  * MSI message composition
  */
@@ -3136,18 +3211,25 @@ static struct irq_chip msi_chip = {
 	.irq_retrigger		= ioapic_retrigger_irq,
 };
 
-static int setup_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int irq)
+static int setup_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc,
+			 unsigned int irq_base, unsigned int irq_offset)
 {
 	struct irq_chip *chip = &msi_chip;
 	struct msi_msg msg;
+	unsigned int irq = irq_base + irq_offset;
 	int ret;
 
 	ret = msi_compose_msg(dev, irq, &msg, -1);
 	if (ret < 0)
 		return ret;
 
-	irq_set_msi_desc(irq, msidesc);
-	write_msi_msg(irq, &msg);
+	irq_set_msi_desc_off(irq_base, irq_offset, msidesc);
+
+	/* MSI-X message is written per-IRQ, the offset is always 0.
+	 * MSI message denotes a contiguous group of IRQs, written for 0th IRQ.
+	 */
+	if (!irq_offset)
+		write_msi_msg(irq, &msg);
 
 	if (irq_remapped(irq_get_chip_data(irq))) {
 		irq_set_status_flags(irq, IRQ_MOVE_PCNTXT);
@@ -3161,16 +3243,12 @@ static int setup_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int irq)
 	return 0;
 }
 
-int native_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
+int setup_msix_irqs(struct pci_dev *dev, int nvec)
 {
 	int node, ret, sub_handle, index = 0;
 	unsigned int irq, irq_want;
 	struct msi_desc *msidesc;
 
-	/* x86 doesn't support multiple MSI yet */
-	if (type == PCI_CAP_ID_MSI && nvec > 1)
-		return 1;
-
 	node = dev_to_node(&dev->dev);
 	irq_want = nr_irqs_gsi;
 	sub_handle = 0;
@@ -3199,7 +3277,7 @@ int native_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 				goto error;
 		}
 no_ir:
-		ret = setup_msi_irq(dev, msidesc, irq);
+		ret = setup_msi_irq(dev, msidesc, irq, 0);
 		if (ret < 0)
 			goto error;
 		sub_handle++;
@@ -3211,6 +3289,76 @@ error:
 	return ret;
 }
 
+int setup_msi_irqs(struct pci_dev *dev, int nvec)
+{
+	int node, ret, sub_handle, index = 0;
+	unsigned int irq;
+	struct msi_desc *msidesc;
+
+	if (nvec > 1 && !irq_remapping_enabled)
+		return 1;
+
+	nvec = __roundup_pow_of_two(nvec);
+	ret = can_create_pow_of_two_irqs(nr_irqs_gsi, nvec);
+	if (ret != nvec)
+		return ret;
+
+	WARN_ON(!list_is_singular(&dev->msi_list));
+	msidesc = list_entry(dev->msi_list.next, struct msi_desc, list);
+	WARN_ON(msidesc->irq);
+	WARN_ON(msidesc->msi_attrib.multiple);
+
+	node = dev_to_node(&dev->dev);
+	irq = create_irqs(nr_irqs_gsi, nvec, node);
+	if (irq == 0)
+		return -ENOSPC;
+
+	if (!irq_remapping_enabled) {
+		ret = setup_msi_irq(dev, msidesc, irq, 0);
+		if (ret < 0)
+			goto error;
+		return 0;
+	}
+
+	msidesc->msi_attrib.multiple = ilog2(nvec);
+	for (sub_handle = 0; sub_handle < nvec; sub_handle++) {
+		if (!sub_handle) {
+			index = msi_alloc_remapped_irq(dev, irq, nvec);
+			if (index < 0) {
+				ret = index;
+				goto error;
+			}
+		} else {
+			ret = msi_setup_remapped_irq(dev, irq + sub_handle,
+						     index, sub_handle);
+			if (ret < 0)
+				goto error;
+		}
+		ret = setup_msi_irq(dev, msidesc, irq, sub_handle);
+		if (ret < 0)
+			goto error;
+	}
+	return 0;
+
+error:
+	destroy_irqs(irq, nvec);
+
+	/* Restore altered MSI descriptor fields and prevent just destroyed
+	 * IRQs from tearing down again in default_teardown_msi_irqs()
+	 */
+	msidesc->irq = 0;
+	msidesc->msi_attrib.multiple = 0;
+
+	return ret;
+}
+
+int native_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
+{
+	if (type == PCI_CAP_ID_MSI)
+		return setup_msi_irqs(dev, nvec);
+	return setup_msix_irqs(dev, nvec);
+}
+
 void native_teardown_msi_irq(unsigned int irq)
 {
 	destroy_irq(irq);
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 47a937c..bccddc0 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -523,6 +523,8 @@ extern int irq_set_handler_data(unsigned int irq, void *data);
 extern int irq_set_chip_data(unsigned int irq, void *data);
 extern int irq_set_irq_type(unsigned int irq, unsigned int type);
 extern int irq_set_msi_desc(unsigned int irq, struct msi_desc *entry);
+extern int irq_set_msi_desc_off(unsigned int irq_base, unsigned int irq_offset,
+				struct msi_desc *entry);
 extern struct irq_data *irq_get_irq_data(unsigned int irq);
 
 static inline struct irq_chip *irq_get_chip(unsigned int irq)
@@ -585,8 +587,12 @@ int __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
 #define irq_alloc_desc_from(from, node)		\
 	irq_alloc_descs(-1, from, 1, node)
 
+#define irq_alloc_descs_from(from, cnt, node)	\
+	irq_alloc_descs(-1, from, cnt, node)
+
 void irq_free_descs(unsigned int irq, unsigned int cnt);
 int irq_reserve_irqs(unsigned int from, unsigned int cnt);
+int irq_can_alloc_irqs(unsigned int from, unsigned int cnt);
 
 static inline void irq_free_desc(unsigned int irq)
 {
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index eebd6d5..dccbec1 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -90,27 +90,41 @@ int irq_set_handler_data(unsigned int irq, void *data)
 EXPORT_SYMBOL(irq_set_handler_data);
 
 /**
- *	irq_set_msi_desc - set MSI descriptor data for an irq
- *	@irq:	Interrupt number
- *	@entry:	Pointer to MSI descriptor data
+ *	irq_set_msi_desc_off - set MSI descriptor data for an irq at offset
+ *	@irq_base:	Interrupt number base
+ *	@irq_offset:	Interrupt number offset
+ *	@entry:		Pointer to MSI descriptor data
  *
- *	Set the MSI descriptor entry for an irq
+ *	Set the MSI descriptor entry for an irq at offset
  */
-int irq_set_msi_desc(unsigned int irq, struct msi_desc *entry)
+int irq_set_msi_desc_off(unsigned int irq_base, unsigned int irq_offset,
+			 struct msi_desc *entry)
 {
 	unsigned long flags;
-	struct irq_desc *desc = irq_get_desc_lock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
+	struct irq_desc *desc = irq_get_desc_lock(irq_base + irq_offset, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
 
 	if (!desc)
 		return -EINVAL;
 	desc->irq_data.msi_desc = entry;
-	if (entry)
-		entry->irq = irq;
+	if (entry && !irq_offset)
+		entry->irq = irq_base;
 	irq_put_desc_unlock(desc, flags);
 	return 0;
 }
 
 /**
+ *	irq_set_msi_desc - set MSI descriptor data for an irq
+ *	@irq:	Interrupt number
+ *	@entry:	Pointer to MSI descriptor data
+ *
+ *	Set the MSI descriptor entry for an irq
+ */
+int irq_set_msi_desc(unsigned int irq, struct msi_desc *entry)
+{
+	return irq_set_msi_desc_off(irq, 0, entry);
+}
+
+/**
  *	irq_set_chip_data - set irq chip data for an irq
  *	@irq:	Interrupt number
  *	@data:	Pointer to chip specific data
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 192a302..8287b78 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -210,6 +210,13 @@ static int irq_expand_nr_irqs(unsigned int nr)
 	return 0;
 }
 
+static int irq_can_expand_nr_irqs(unsigned int nr)
+{
+	if (nr > IRQ_BITMAP_BITS)
+		return -ENOMEM;
+	return 0;
+}
+
 int __init early_irq_init(void)
 {
 	int i, initcnt, node = first_online_node;
@@ -414,6 +421,30 @@ int irq_reserve_irqs(unsigned int from, unsigned int cnt)
 }
 
 /**
+ * irq_can_alloc_irqs - checks if a range of irqs could be allocated
+ * @from:	check from irq number
+ * @cnt:	number of irqs to check
+ *
+ * Returns 0 on success or an appropriate error code
+ */
+int irq_can_alloc_irqs(unsigned int from, unsigned int cnt)
+{
+	unsigned int start;
+	int ret = 0;
+
+	if (!cnt)
+		return -EINVAL;
+
+	mutex_lock(&sparse_irq_lock);
+	start = bitmap_find_next_zero_area(allocated_irqs, IRQ_BITMAP_BITS,
+					   from, cnt, 0);
+	mutex_unlock(&sparse_irq_lock);
+	if (start + cnt > nr_irqs)
+		ret = irq_can_expand_nr_irqs(start + cnt);
+	return ret;
+}
+
+/**
  * irq_get_next_irq - get next allocated irq number
  * @offset:	where to start the search
  *
-- 
1.7.7.6


-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH 2/5] x86, MSI: Allocate as many multiple IRQs as requested

[Alexander Gordeev]

When multiple MSIs are enabled with pci_enable_msi_block() the number of
allocated IRQs 'nvec' is rounded up to the nearest value of power of two.
That could lead to a condition when number of requested and used IRQs is
less than number of actually allocated IRQs.

This fix introduces 'msi_desc::nvec' field to address the above issue -
when non-zero, it holds the number of allocated IRQs. Otherwise, the old
method is used.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 arch/x86/kernel/apic/io_apic.c |   16 +++++++---------
 drivers/pci/msi.c              |   10 ++++++++--
 include/linux/msi.h            |    1 +
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index f083049..5a5c92b 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -3107,16 +3107,12 @@ static inline void destroy_irqs(unsigned int irq, unsigned int count)
 }
 
 static inline int
-can_create_pow_of_two_irqs(unsigned int from, unsigned int count)
+can_create_irqs(unsigned int from, unsigned int count)
 {
-	if ((count > 1) && (count % 2))
-		return -EINVAL;
-
-	for (; count; count = count / 2) {
+	for (; count; count = count - 1) {
 		if (!irq_can_alloc_irqs(from, count))
 			return count;
 	}
-
 	return -ENOSPC;
 }
 
@@ -3298,8 +3294,7 @@ int setup_msi_irqs(struct pci_dev *dev, int nvec)
 	if (nvec > 1 && !irq_remapping_enabled)
 		return 1;
 
-	nvec = __roundup_pow_of_two(nvec);
-	ret = can_create_pow_of_two_irqs(nr_irqs_gsi, nvec);
+	ret = can_create_irqs(nr_irqs_gsi, nvec);
 	if (ret != nvec)
 		return ret;
 
@@ -3307,11 +3302,13 @@ int setup_msi_irqs(struct pci_dev *dev, int nvec)
 	msidesc = list_entry(dev->msi_list.next, struct msi_desc, list);
 	WARN_ON(msidesc->irq);
 	WARN_ON(msidesc->msi_attrib.multiple);
+	WARN_ON(msidesc->nvec);
 
 	node = dev_to_node(&dev->dev);
 	irq = create_irqs(nr_irqs_gsi, nvec, node);
 	if (irq == 0)
 		return -ENOSPC;
+	msidesc->nvec = nvec;
 
 	if (!irq_remapping_enabled) {
 		ret = setup_msi_irq(dev, msidesc, irq, 0);
@@ -3320,7 +3317,7 @@ int setup_msi_irqs(struct pci_dev *dev, int nvec)
 		return 0;
 	}
 
-	msidesc->msi_attrib.multiple = ilog2(nvec);
+	msidesc->msi_attrib.multiple = ilog2(__roundup_pow_of_two(nvec));
 	for (sub_handle = 0; sub_handle < nvec; sub_handle++) {
 		if (!sub_handle) {
 			index = msi_alloc_remapped_irq(dev, irq, nvec);
@@ -3348,6 +3345,7 @@ error:
 	 */
 	msidesc->irq = 0;
 	msidesc->msi_attrib.multiple = 0;
+	msidesc->nvec = 0;
 
 	return ret;
 }
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index a825d78..f0752d1 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -79,7 +79,10 @@ void default_teardown_msi_irqs(struct pci_dev *dev)
 		int i, nvec;
 		if (entry->irq == 0)
 			continue;
-		nvec = 1 << entry->msi_attrib.multiple;
+		if (entry->nvec)
+			nvec = entry->nvec;
+		else
+			nvec = 1 << entry->msi_attrib.multiple;
 		for (i = 0; i < nvec; i++)
 			arch_teardown_msi_irq(entry->irq + i);
 	}
@@ -336,7 +339,10 @@ static void free_msi_irqs(struct pci_dev *dev)
 		int i, nvec;
 		if (!entry->irq)
 			continue;
-		nvec = 1 << entry->msi_attrib.multiple;
+		if (entry->nvec)
+			nvec = entry->nvec;
+		else
+			nvec = 1 << entry->msi_attrib.multiple;
 		for (i = 0; i < nvec; i++)
 			BUG_ON(irq_has_action(entry->irq + i));
 	}
diff --git a/include/linux/msi.h b/include/linux/msi.h
index ce93a34..6f4dfba 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -35,6 +35,7 @@ struct msi_desc {
 
 	u32 masked;			/* mask bits */
 	unsigned int irq;
+	unsigned int nvec;
 	struct list_head list;
 
 	union {
-- 
1.7.7.6


-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH 3/5] x86, MSI: Minor readability fixes

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 arch/x86/kernel/apic/io_apic.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 5a5c92b..888f3b9 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -3142,7 +3142,7 @@ static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq,
 
 	if (irq_remapped(cfg)) {
 		compose_remapped_msi_msg(pdev, irq, dest, msg, hpet_id);
-		return err;
+		return 0;
 	}
 
 	if (x2apic_enabled())
@@ -3169,7 +3169,7 @@ static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq,
 			MSI_DATA_DELIVERY_LOWPRI) |
 		MSI_DATA_VECTOR(cfg->vector);
 
-	return err;
+	return 0;
 }
 
 static int
@@ -3251,7 +3251,7 @@ int setup_msix_irqs(struct pci_dev *dev, int nvec)
 	list_for_each_entry(msidesc, &dev->msi_list, list) {
 		irq = create_irq_nr(irq_want, node);
 		if (irq == 0)
-			return -1;
+			return -ENOSPC;
 		irq_want = irq + 1;
 		if (!irq_remapping_enabled)
 			goto no_ir;
-- 
1.7.7.6


-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH 4/5] PCI, MSI: Enable multiple MSIs with pci_enable_msi_block_auto()

[Alexander Gordeev]

The new function pci_enable_msi_block_auto() tries to allocate maximum
possible number of MSIs up to the number the device supports. It
generalizes a pattern when pci_enable_msi_block() contiguously called
until it succeeds or fails.

Opposite to pci_enable_msi_block() which takes the number of MSIs to
allocate as a input parameter, pci_enable_msi_block_auto() could be used
by device drivers to obtain the number of assigned MSIs.

The function could also be used by device drivers which do not care
about the exact number of assigned MSIs.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 Documentation/PCI/MSI-HOWTO.txt |   41 ++++++++++++++++++++++++++++++++++----
 drivers/pci/msi.c               |   29 +++++++++++++++++++++++++++
 include/linux/pci.h             |    7 ++++++
 3 files changed, 72 insertions(+), 5 deletions(-)

diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
index 53e6fca..d5ab6d7 100644
--- a/Documentation/PCI/MSI-HOWTO.txt
+++ b/Documentation/PCI/MSI-HOWTO.txt
@@ -127,15 +127,46 @@ on the number of vectors that can be allocated; pci_enable_msi_block()
 returns as soon as it finds any constraint that doesn't allow the
 call to succeed.
 
-4.2.3 pci_disable_msi
+4.2.3 pci_enable_msi_block_auto
+
+int pci_enable_msi_block_auto(struct pci_dev *dev, int *count)
+
+This variation on pci_enable_msi() call allows a device driver to request
+the maximum possible number of MSIs.  The MSI specification only allows
+interrupts to be allocated in powers of two, up to a maximum of 2^5 (32).
+
+If this function returns 0, it has succeeded in allocating as many
+interrupts as the device supports.
+
+If this function returns a positive number, it indicates that it has
+succeeded, but the number of allocated interrupts is less than the number
+of interrupts the device supports. The returned value in this case is the
+number of allocated interrupts.
+
+In both cases above, the function enables MSI on this device and updates
+dev->irq to be the lowest of the new interrupts assigned to it. The other
+interrupts assigned to the device are in the range dev->irq to dev->irq +
+number of allocated interrupts - 1.
+
+If the device driver needs to know the number of allocated interrupts it
+can pass the pointer count where that number is stored. If the device driver
+has other means to obtain that number or does not need it, NULL pointer can
+be passed instead.
+
+If this function returns a negative number, it indicates an error and the
+driver should not attempt to request any more MSI interrupts for this
+device.
+
+4.2.4 pci_disable_msi
 
 void pci_disable_msi(struct pci_dev *dev)
 
 This function should be used to undo the effect of pci_enable_msi() or
-pci_enable_msi_block().  Calling it restores dev->irq to the pin-based
-interrupt number and frees the previously allocated message signaled
-interrupt(s).  The interrupt may subsequently be assigned to another
-device, so drivers should not cache the value of dev->irq.
+pci_enable_msi_block() or pci_enable_msi_block_auto().  Calling it restores
+dev->irq to the pin-based interrupt number and frees the previously
+allocated message signaled interrupt(s).  The interrupt may subsequently be
+assigned to another device, so drivers should not cache the value of
+dev->irq.
 
 Before calling this function, a device driver must always call free_irq()
 on any interrupt for which it previously called request_irq().
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index f0752d1..ef982c1 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -845,6 +845,35 @@ int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec)
 }
 EXPORT_SYMBOL(pci_enable_msi_block);
 
+int pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *nvec)
+{
+	int ret, pos, envec, maxvec;
+	u16 msgctl;
+
+	pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
+	if (!pos)
+		return -EINVAL;
+
+	pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &msgctl);
+	maxvec = 1 << ((msgctl & PCI_MSI_FLAGS_QMASK) >> 1);
+	ret = maxvec;
+
+	do {
+		envec = ret;
+		ret = pci_enable_msi_block(dev, envec);
+	} while (ret > 0);
+
+	if (ret < 0)
+		return ret;
+	if (nvec)
+		*nvec = envec;
+	if (envec < maxvec)
+		return envec;
+	return 0;
+
+}
+EXPORT_SYMBOL(pci_enable_msi_block_auto);
+
 void pci_msi_shutdown(struct pci_dev *dev)
 {
 	struct msi_desc *desc;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d8c379d..14788d0 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1024,6 +1024,12 @@ static inline int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec)
 	return -1;
 }
 
+static inline int
+pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *nvec)
+{
+	return -1;
+}
+
 static inline void pci_msi_shutdown(struct pci_dev *dev)
 { }
 static inline void pci_disable_msi(struct pci_dev *dev)
@@ -1055,6 +1061,7 @@ static inline int pci_msi_enabled(void)
 }
 #else
 extern int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec);
+extern int pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *nvec);
 extern void pci_msi_shutdown(struct pci_dev *dev);
 extern void pci_disable_msi(struct pci_dev *dev);
 extern int pci_msix_table_size(struct pci_dev *dev);
-- 
1.7.7.6


-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH 5/5] AHCI: Support multiple MSIs

[Alexander Gordeev]

Take advantage of multiple MSIs implementation on x86 - on systems with
IRQ remapping AHCI ports not only get assigned separate MSI vectors -
but also separate IRQs. As result, interrupts generated by different
ports could be serviced on different CPUs rather than on a single one.

In cases when number of allocated MSIs is less than requested the Sharing
Last MSI mode does not get used, no matter implemented in hardware or not.
Instead, the driver assumes the advantage of multiple MSIs is negated and
falls back to the single MSI mode as if MRSM bit was set (some Intel chips
implement this strategy anyway - MRSM bit gets set even if the number of
allocated MSIs exceeds the number of implemented ports).

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/ata/ahci.c        |   11 +++----
 drivers/ata/ahci.h        |    1 +
 drivers/ata/libahci.c     |   24 ++++++++++++++++
 drivers/ata/libata-core.c |   66 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/libata.h    |    3 ++
 5 files changed, 99 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index ebaf67e..4bc2cc2 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1044,7 +1044,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	struct device *dev = &pdev->dev;
 	struct ahci_host_priv *hpriv;
 	struct ata_host *host;
-	int n_ports, i, rc;
+	int n_ports, n_msis, i, rc;
 	int ahci_pci_bar = AHCI_PCI_BAR_STANDARD;
 
 	VPRINTK("ENTER\n");
@@ -1129,11 +1129,10 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ahci_sb600_enable_64bit(pdev))
 		hpriv->flags &= ~AHCI_HFLAG_32BIT_ONLY;
 
-	if ((hpriv->flags & AHCI_HFLAG_NO_MSI) || pci_enable_msi(pdev))
-		pci_intx(pdev, 1);
-
 	hpriv->mmio = pcim_iomap_table(pdev)[ahci_pci_bar];
 
+	n_msis = ahci_init_interrupts(pdev, hpriv);
+
 	/* save initial config */
 	ahci_pci_save_initial_config(pdev, hpriv);
 
@@ -1229,8 +1228,8 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	ahci_pci_print_info(host);
 
 	pci_set_master(pdev);
-	return ata_host_activate(host, pdev->irq, ahci_interrupt, IRQF_SHARED,
-				 &ahci_sht);
+	return ata_ahci_host_activate(host, pdev->irq, n_msis, ahci_interrupt,
+				      IRQF_SHARED, &ahci_sht);
 }
 
 static int __init ahci_init(void)
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index c2594dd..0f2d557 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -328,6 +328,7 @@ void ahci_save_initial_config(struct device *dev,
 			      unsigned int mask_port_map);
 void ahci_init_controller(struct ata_host *host);
 int ahci_reset_controller(struct ata_host *host);
+int ahci_init_interrupts(struct pci_dev *pdev, struct ahci_host_priv *hpriv);
 
 int ahci_do_softreset(struct ata_link *link, unsigned int *class,
 		      int pmp, unsigned long deadline,
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index f9eaa82..d08ef77 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -39,6 +39,7 @@
 #include <linux/blkdev.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
+#include <linux/pci.h>
 #include <linux/dma-mapping.h>
 #include <linux/device.h>
 #include <scsi/scsi_host.h>
@@ -1128,6 +1129,29 @@ void ahci_init_controller(struct ata_host *host)
 }
 EXPORT_SYMBOL_GPL(ahci_init_controller);
 
+int ahci_init_interrupts(struct pci_dev *pdev, struct ahci_host_priv *hpriv)
+{
+	int rc;
+	unsigned int nvec;
+
+	if (!(hpriv->flags & AHCI_HFLAG_NO_MSI)) {
+		rc = pci_enable_msi_block_auto(pdev, &nvec);
+		if ((rc == 0) || (rc == 1)) {
+			return nvec;
+		} else if (rc > 0) {
+			/* assume that advantage of multipe MSIs is negated,
+			 * so fallback to single MSI mode to save resources */
+			pci_disable_msi(pdev);
+			if (!pci_enable_msi(pdev))
+				return 1;
+		}
+	}
+
+	pci_intx(pdev, 1);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ahci_init_interrupts);
+
 static void ahci_dev_config(struct ata_device *dev)
 {
 	struct ahci_host_priv *hpriv = dev->link->ap->host->private_data;
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index cece3a4..3f5e735 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6154,6 +6154,71 @@ int ata_host_activate(struct ata_host *host, int irq,
 }
 
 /**
+ *	ata_ahci_host_activate - start AHCI host, request IRQs and register it
+ *	@host: target ATA host
+ *	@irq: base IRQ number to request
+ *	@n_msis: number of MSIs allocated for this host
+ *	@irq_handler: irq_handler used when requesting IRQs
+ *	@irq_flags: irq_flags used when requesting IRQs
+ *	@sht: scsi_host_template to use when registering the host
+ *
+ *	Similar to ata_host_activate, but requests IRQs according to AHCI-1.1
+ *	when multiple MSIs were allocated. That is one MSI per port, starting
+ *	from @irq. Also, when number of MSIs is less than number of ports,
+ *	last MSI is shared among those ports which did not get their personal
+ *	MSI vector - see 10.6.2.3.1.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ */
+int ata_ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis,
+			   irq_handler_t irq_handler, unsigned long irq_flags,
+			   struct scsi_host_template *sht)
+{
+	int i, rc;
+	unsigned int n_irqs;
+
+	if (!irq || (n_msis < 2))
+		return ata_host_activate(host, irq, irq_handler, irq_flags, sht);
+
+	rc = ata_host_start(host);
+	if (rc)
+		return rc;
+
+	n_irqs = min(host->n_ports, n_msis);
+
+	for (i = 0; i < n_irqs; i++) {
+		rc = devm_request_irq(host->dev,
+				      irq + i, irq_handler, irq_flags,
+				      dev_driver_string(host->dev), host);
+		if (rc)
+			goto out_free_irqs;
+	}
+
+	for (i = 0; i < n_irqs; i++)
+		ata_port_desc(host->ports[i], "irq %d", irq + i);
+	for (; i < host->n_ports; i++)
+		ata_port_desc(host->ports[i], "irq %d", n_irqs - 1);
+
+	rc = ata_host_register(host, sht);
+	if (rc)
+		goto out_free_all_irqs;
+
+	return 0;
+
+out_free_all_irqs:
+	i = n_irqs;
+out_free_irqs:
+	for (; i; i--)
+		devm_free_irq(host->dev, irq + i - 1, host);
+
+	return rc;
+}
+
+/**
  *	ata_port_detach - Detach ATA port in prepration of device removal
  *	@ap: ATA port to be detached
  *
@@ -6748,6 +6813,7 @@ EXPORT_SYMBOL_GPL(ata_slave_link_init);
 EXPORT_SYMBOL_GPL(ata_host_start);
 EXPORT_SYMBOL_GPL(ata_host_register);
 EXPORT_SYMBOL_GPL(ata_host_activate);
+EXPORT_SYMBOL_GPL(ata_ahci_host_activate);
 EXPORT_SYMBOL_GPL(ata_host_detach);
 EXPORT_SYMBOL_GPL(ata_sg_init);
 EXPORT_SYMBOL_GPL(ata_qc_complete);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 6e887c7..c6b40f5 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -986,6 +986,9 @@ extern int ata_host_register(struct ata_host *host,
 extern int ata_host_activate(struct ata_host *host, int irq,
 			     irq_handler_t irq_handler, unsigned long irq_flags,
 			     struct scsi_host_template *sht);
+extern int ata_ahci_host_activate(struct ata_host *host, int irq,
+			unsigned int n_msis, irq_handler_t irq_handler,
+			unsigned long irq_flags, struct scsi_host_template *sht);
 extern void ata_host_detach(struct ata_host *host);
 extern void ata_host_init(struct ata_host *, struct device *,
 			  unsigned long, struct ata_port_operations *);
-- 
1.7.7.6


-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 4/5] PCI, MSI: Enable multiple MSIs with pci_enable_msi_block_auto()

[Bjorn Helgaas]

On Thu, Aug 16, 2012 at 8:49 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> The new function pci_enable_msi_block_auto() tries to allocate maximum
> possible number of MSIs up to the number the device supports. It
> generalizes a pattern when pci_enable_msi_block() contiguously called
> until it succeeds or fails.
>
> Opposite to pci_enable_msi_block() which takes the number of MSIs to
> allocate as a input parameter, pci_enable_msi_block_auto() could be used
> by device drivers to obtain the number of assigned MSIs.
>
> The function could also be used by device drivers which do not care
> about the exact number of assigned MSIs.
>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  Documentation/PCI/MSI-HOWTO.txt |   41 ++++++++++++++++++++++++++++++++++----
>  drivers/pci/msi.c               |   29 +++++++++++++++++++++++++++
>  include/linux/pci.h             |    7 ++++++
>  3 files changed, 72 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/PCI/MSI-HOWTO.txt b/Documentation/PCI/MSI-HOWTO.txt
> index 53e6fca..d5ab6d7 100644
> --- a/Documentation/PCI/MSI-HOWTO.txt
> +++ b/Documentation/PCI/MSI-HOWTO.txt
> @@ -127,15 +127,46 @@ on the number of vectors that can be allocated; pci_enable_msi_block()
>  returns as soon as it finds any constraint that doesn't allow the
>  call to succeed.
>
> -4.2.3 pci_disable_msi
> +4.2.3 pci_enable_msi_block_auto
> +
> +int pci_enable_msi_block_auto(struct pci_dev *dev, int *count)
> +
> +This variation on pci_enable_msi() call allows a device driver to request
> +the maximum possible number of MSIs.  The MSI specification only allows
> +interrupts to be allocated in powers of two, up to a maximum of 2^5 (32).
> +
> +If this function returns 0, it has succeeded in allocating as many
> +interrupts as the device supports.
> +
> +If this function returns a positive number, it indicates that it has
> +succeeded, but the number of allocated interrupts is less than the number
> +of interrupts the device supports. The returned value in this case is the
> +number of allocated interrupts.

Seems like it would be simpler to avoid the special case of returning
zero.  You could return a negative value for failure, otherwise return
the number of interrupts allocated.

Then you could also dispense with the "int *count" argument, because
the caller could just look at the return value.

> +In both cases above, the function enables MSI on this device and updates
> +dev->irq to be the lowest of the new interrupts assigned to it. The other
> +interrupts assigned to the device are in the range dev->irq to dev->irq +
> +number of allocated interrupts - 1.
> +
> +If the device driver needs to know the number of allocated interrupts it
> +can pass the pointer count where that number is stored. If the device driver
> +has other means to obtain that number or does not need it, NULL pointer can
> +be passed instead.
> +
> +If this function returns a negative number, it indicates an error and the
> +driver should not attempt to request any more MSI interrupts for this
> +device.
> +
> +4.2.4 pci_disable_msi
>
>  void pci_disable_msi(struct pci_dev *dev)
>
>  This function should be used to undo the effect of pci_enable_msi() or
> -pci_enable_msi_block().  Calling it restores dev->irq to the pin-based
> -interrupt number and frees the previously allocated message signaled
> -interrupt(s).  The interrupt may subsequently be assigned to another
> -device, so drivers should not cache the value of dev->irq.
> +pci_enable_msi_block() or pci_enable_msi_block_auto().  Calling it restores
> +dev->irq to the pin-based interrupt number and frees the previously
> +allocated message signaled interrupt(s).  The interrupt may subsequently be
> +assigned to another device, so drivers should not cache the value of
> +dev->irq.
>
>  Before calling this function, a device driver must always call free_irq()
>  on any interrupt for which it previously called request_irq().
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index f0752d1..ef982c1 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -845,6 +845,35 @@ int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec)
>  }
>  EXPORT_SYMBOL(pci_enable_msi_block);
>
> +int pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *nvec)
> +{
> +       int ret, pos, envec, maxvec;
> +       u16 msgctl;
> +
> +       pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
> +       if (!pos)
> +               return -EINVAL;
> +
> +       pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &msgctl);
> +       maxvec = 1 << ((msgctl & PCI_MSI_FLAGS_QMASK) >> 1);
> +       ret = maxvec;
> +
> +       do {
> +               envec = ret;
> +               ret = pci_enable_msi_block(dev, envec);
> +       } while (ret > 0);
> +
> +       if (ret < 0)
> +               return ret;
> +       if (nvec)
> +               *nvec = envec;
> +       if (envec < maxvec)
> +               return envec;
> +       return 0;
> +
> +}
> +EXPORT_SYMBOL(pci_enable_msi_block_auto);
> +
>  void pci_msi_shutdown(struct pci_dev *dev)
>  {
>         struct msi_desc *desc;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d8c379d..14788d0 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1024,6 +1024,12 @@ static inline int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec)
>         return -1;
>  }
>
> +static inline int
> +pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *nvec)
> +{
> +       return -1;
> +}
> +
>  static inline void pci_msi_shutdown(struct pci_dev *dev)
>  { }
>  static inline void pci_disable_msi(struct pci_dev *dev)
> @@ -1055,6 +1061,7 @@ static inline int pci_msi_enabled(void)
>  }
>  #else
>  extern int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec);
> +extern int pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *nvec);
>  extern void pci_msi_shutdown(struct pci_dev *dev);
>  extern void pci_disable_msi(struct pci_dev *dev);
>  extern int pci_msix_table_size(struct pci_dev *dev);
> --
> 1.7.7.6
>
>
> --
> Regards,
> Alexander Gordeev
> agordeev@redhat.com

Re: [PATCH 0/5] x86, MSI, AHCI: Support multiple MSIs

[Jeff Garzik]

On 08/16/2012 10:45 AM, Alexander Gordeev wrote:
> Currently multiple MSI mode is limited to a single vector per device (at
> least on x86 and PPC). This series breathes life into pci_enable_msi_block()
> and makes it possible to set interrupt affinity for multiple IRQs, similarly
> to MSI-X. Yet, only for x86 and only when IOMMUs are present.
>
> Although IRQ and PCI subsystems are modified, the current behaviour left
> intact. The drivers could just start using multiple MSIs just by following
> the existing documentation.
>
> The AHCI device driver makes use of the new mode and a new function -
> pci_enable_msi_block_auto() - patches 4,5.
>
> The series is adapted to Ingo's -tip repository, x86/apic branch.
> Patches 4,5 could be applied independently of patches 1-3.
>
> Attached patches:
>    1/5 x86, MSI: Support multiple MSIs in presense of IRQ remapping
>    2/5 x86, MSI: Allocate as many multiple IRQs as requested
>    3/5 x86, MSI: Minor readability fixes
>    4/5 PCI, MSI: Enable multiple MSIs with pci_enable_msi_block_auto()
>    5/5 AHCI: Support multiple MSIs

Numbers?  I would like to see measurements that show this is a benefit. 
  I'm especially wondering about lock contention for the case of 
multiple, fast devices (e.g. RAM over SATA, or PCIe flash).

Because I see the following obvious problems immediately:

1) AHCI takes a host-wide lock during interrupt processing, which quite 
reduces the value of "interrupts generated by different
ports could be serviced on different CPUs"

2) We do not put AHCI-specific code in libata-core.  Try libahci.c or 
ahci.c.

Regards,

	Jeff

Re: [PATCH 4/5] PCI, MSI: Enable multiple MSIs with pci_enable_msi_block_auto()

[Alexander Gordeev]

On Thu, Aug 16, 2012 at 10:00:39AM -0600, Bjorn Helgaas wrote:
> On Thu, Aug 16, 2012 at 8:49 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> > -4.2.3 pci_disable_msi
> > +4.2.3 pci_enable_msi_block_auto
> > +
> > +int pci_enable_msi_block_auto(struct pci_dev *dev, int *count)
> > +
> > +This variation on pci_enable_msi() call allows a device driver to request
> > +the maximum possible number of MSIs.  The MSI specification only allows
> > +interrupts to be allocated in powers of two, up to a maximum of 2^5 (32).
> > +
> > +If this function returns 0, it has succeeded in allocating as many
> > +interrupts as the device supports.
> > +
> > +If this function returns a positive number, it indicates that it has
> > +succeeded, but the number of allocated interrupts is less than the number
> > +of interrupts the device supports. The returned value in this case is the
> > +number of allocated interrupts.
> 
> Seems like it would be simpler to avoid the special case of returning
> zero.  You could return a negative value for failure, otherwise return
> the number of interrupts allocated.

But this special case is important, because some drivers would not get
satisfied with just any number of interrupts allocated (i.e. few Intel AHCI
chips (seems) have hardware logic that compares qmask vs qsize and simply
falls back to single interrupt if they are not equal).

So I see the fact that maximum possible number of interrupts were allocated
at least as important than the number itself.

> Then you could also dispense with the "int *count" argument, because
> the caller could just look at the return value.

What about returning the number of allocated interrtupts while storing the
number of supported interrupts to "int *count" (or maxcount)?

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 0/5] x86, MSI, AHCI: Support multiple MSIs

[Alexander Gordeev]

On Thu, Aug 16, 2012 at 12:19:43PM -0400, Jeff Garzik wrote:
> 1) AHCI takes a host-wide lock during interrupt processing, which
> quite reduces the value of "interrupts generated by different
> ports could be serviced on different CPUs"
> 
> 2) We do not put AHCI-specific code in libata-core.  Try libahci.c
> or ahci.c.

Will fix that and post along with numbers.

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 4/5] PCI, MSI: Enable multiple MSIs with pci_enable_msi_block_auto()

[Bjorn Helgaas]

On Fri, Aug 17, 2012 at 2:19 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> On Thu, Aug 16, 2012 at 10:00:39AM -0600, Bjorn Helgaas wrote:
>> On Thu, Aug 16, 2012 at 8:49 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
>> > -4.2.3 pci_disable_msi
>> > +4.2.3 pci_enable_msi_block_auto
>> > +
>> > +int pci_enable_msi_block_auto(struct pci_dev *dev, int *count)
>> > +
>> > +This variation on pci_enable_msi() call allows a device driver to request
>> > +the maximum possible number of MSIs.  The MSI specification only allows
>> > +interrupts to be allocated in powers of two, up to a maximum of 2^5 (32).
>> > +
>> > +If this function returns 0, it has succeeded in allocating as many
>> > +interrupts as the device supports.
>> > +
>> > +If this function returns a positive number, it indicates that it has
>> > +succeeded, but the number of allocated interrupts is less than the number
>> > +of interrupts the device supports. The returned value in this case is the
>> > +number of allocated interrupts.
>>
>> Seems like it would be simpler to avoid the special case of returning
>> zero.  You could return a negative value for failure, otherwise return
>> the number of interrupts allocated.
>
> But this special case is important, because some drivers would not get
> satisfied with just any number of interrupts allocated (i.e. few Intel AHCI
> chips (seems) have hardware logic that compares qmask vs qsize and simply
> falls back to single interrupt if they are not equal).
>
> So I see the fact that maximum possible number of interrupts were allocated
> at least as important than the number itself.
>
>> Then you could also dispense with the "int *count" argument, because
>> the caller could just look at the return value.
>
> What about returning the number of allocated interrtupts while storing the
> number of supported interrupts to "int *count" (or maxcount)?

I like that idea a lot better because then you don't need a special
case to interpret the return value.