diff mbox

PCI MSI: allow alignment restrictions on vector allocation

Message ID 20170925041153.26574-1-drake@endlessm.com (mailing list archive)
State Not Applicable
Delegated to: Kalle Valo
Headers show

Commit Message

Daniel Drake Sept. 25, 2017, 4:11 a.m. UTC
ath9k hardware claims to support up to 4 MSI vectors, and when run in
that configuration, it would be allowed to modify the lower bits of the
MSI Message Data when generating interrupts in order to signal which
of the 4 vectors the interrupt is being raised on.

Linux's PCI-MSI irqchip only supports a single MSI vector for each
device, and it tells the device this, but the device appears to assume
it is working with 4, as it will unset the lower 2 bits of Message Data
presumably to indicate that it is an IRQ for the first of 4 possible
vectors.

Linux will then receive an interrupt on the wrong vector, so the
ath9k interrupt handler will not be invoked.

To work around this, introduce a mechanism where the vector assignment
algorithm can be restricted to only a subset of available vector numbers
based on a bitmap.

As a user of this bitmap, introduce a pci_dev.align_msi_vector flag which
can be used to state that MSI vector numbers must be aligned to a specific
amount. If we 4-align the ath9k MSI vector then the lower bits will
already be 0 and hence the device will not modify the Message Data away
from its original value.

This is needed in order to support the wifi card in at least 8 new Acer
consumer laptop models which come with the Foxconn NFA335 WiFi module.
Legacy interrupts do not work on that module, so MSI support is required.

Signed-off-by: Daniel Drake <drake@endlessm.com>

https://phabricator.endlessm.com/T16988
---
 arch/x86/include/asm/hw_irq.h |  1 +
 arch/x86/kernel/apic/msi.c    | 15 +++++++++++++++
 arch/x86/kernel/apic/vector.c | 32 +++++++++++++++++++++++++-------
 include/linux/pci.h           |  2 ++
 4 files changed, 43 insertions(+), 7 deletions(-)

This solves the issue described here:
https://marc.info/?l=linux-pci&m=150238260826803&w=2

If this approach looks good I'll follow up with the ath9k patch
to enable MSI interrupts.

Comments

kernel test robot Sept. 26, 2017, 8:32 p.m. UTC | #1
Hi Daniel,

[auto build test ERROR on pci/next]
[also build test ERROR on v4.14-rc2]
[cannot apply to tip/x86/core next-20170926]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Daniel-Drake/PCI-MSI-allow-alignment-restrictions-on-vector-allocation/20170927-035611
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: i386-randconfig-x000-201739 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   arch/x86/kernel/apic/vector.c: In function 'assign_irq_vector_policy':
>> arch/x86/kernel/apic/vector.c:266:25: error: 'struct irq_alloc_info' has no member named 'allowed_vectors'
      allowed_vectors = info->allowed_vectors;
                            ^~

vim +266 arch/x86/kernel/apic/vector.c

   252	
   253	static int assign_irq_vector_policy(int irq, int node,
   254					    struct apic_chip_data *data,
   255					    struct irq_alloc_info *info,
   256					    struct irq_data *irqdata)
   257	{
   258		unsigned long *allowed_vectors = NULL;
   259	
   260		/* Some MSI interrupts have restrictions on which vector numbers
   261		 * can be used.
   262		 */
   263		if (info &&
   264			(info->type == X86_IRQ_ALLOC_TYPE_MSI ||
   265			 info->type == X86_IRQ_ALLOC_TYPE_MSIX))
 > 266			allowed_vectors = info->allowed_vectors;
   267	
   268		if (info && info->mask)
   269			return assign_irq_vector(irq, data, info->mask, irqdata,
   270						 allowed_vectors);
   271		if (node != NUMA_NO_NODE &&
   272		    assign_irq_vector(irq, data, cpumask_of_node(node), irqdata,
   273				      allowed_vectors) == 0)
   274			return 0;
   275		return assign_irq_vector(irq, data, apic->target_cpus(), irqdata,
   276					 allowed_vectors);
   277	}
   278	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Thomas Gleixner Sept. 27, 2017, 3:28 p.m. UTC | #2
On Mon, 25 Sep 2017, Daniel Drake wrote:

Please send x86 related patches to LKML as documented in MAINTAINERS. That
patch is mainly x86 and not PCI.

> ath9k hardware claims to support up to 4 MSI vectors, and when run in
> that configuration, it would be allowed to modify the lower bits of the
> MSI Message Data when generating interrupts in order to signal which
> of the 4 vectors the interrupt is being raised on.
> 
> Linux's PCI-MSI irqchip only supports a single MSI vector for each
> device, and it tells the device this, but the device appears to assume
> it is working with 4, as it will unset the lower 2 bits of Message Data
> presumably to indicate that it is an IRQ for the first of 4 possible
> vectors.
> 
> Linux will then receive an interrupt on the wrong vector, so the
> ath9k interrupt handler will not be invoked.
> 
> To work around this, introduce a mechanism where the vector assignment
> algorithm can be restricted to only a subset of available vector numbers
> based on a bitmap.
> 
> As a user of this bitmap, introduce a pci_dev.align_msi_vector flag which
> can be used to state that MSI vector numbers must be aligned to a specific
> amount. If we 4-align the ath9k MSI vector then the lower bits will
> already be 0 and hence the device will not modify the Message Data away
> from its original value.
> 
> This is needed in order to support the wifi card in at least 8 new Acer
> consumer laptop models which come with the Foxconn NFA335 WiFi module.
> Legacy interrupts do not work on that module, so MSI support is required.

Sigh, what a mess.

> diff --git a/arch/x86/include/asm/hw_irq.h b/arch/x86/include/asm/hw_irq.h
> index 6dfe366a8804..7f35178586a1 100644
> --- a/arch/x86/include/asm/hw_irq.h
> +++ b/arch/x86/include/asm/hw_irq.h
> @@ -77,6 +77,7 @@ struct irq_alloc_info {
>  		struct {
>  			struct pci_dev	*msi_dev;
>  			irq_hw_number_t	msi_hwirq;
> +			DECLARE_BITMAP(allowed_vectors, NR_VECTORS);

Uurgh. No. You want to express an alignment requirement. Why would you need
a bitmap for that? A simple

	unsigned int align_vector;

is sufficient.


> @@ -178,6 +179,9 @@ static int __assign_irq_vector(int irq, struct apic_chip_data *d,
>  		if (test_bit(vector, used_vectors))
>  			goto next;
>  
> +		if (allowed_vectors && !test_bit(vector, allowed_vectors))
> +			goto next;

This code is gone in -next and replaced by a new vector allocator.

  git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/apic

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index f68c58a93dd0..7755450be02d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -419,6 +419,8 @@ struct pci_dev {
>  #endif
>  #ifdef CONFIG_PCI_MSI
>  	const struct attribute_group **msi_irq_groups;
> +	int align_msi_vector;	/* device requires MSI vector numbers aligned
> +				 * by this amount */

This wants to be unsigned int and please get rid of this butt ugly
formatted comment.

But the real question is how this is supposed to work with

 - interrupt remapping

 - non X86 machines

I doubt that this can be made generic enough to make it work all over the
place. Tell Acer/Foxconn to fix their stupid firmware.

Thanks,

	tglx
diff mbox

Patch

diff --git a/arch/x86/include/asm/hw_irq.h b/arch/x86/include/asm/hw_irq.h
index 6dfe366a8804..7f35178586a1 100644
--- a/arch/x86/include/asm/hw_irq.h
+++ b/arch/x86/include/asm/hw_irq.h
@@ -77,6 +77,7 @@  struct irq_alloc_info {
 		struct {
 			struct pci_dev	*msi_dev;
 			irq_hw_number_t	msi_hwirq;
+			DECLARE_BITMAP(allowed_vectors, NR_VECTORS);
 		};
 #endif
 #ifdef	CONFIG_X86_IO_APIC
diff --git a/arch/x86/kernel/apic/msi.c b/arch/x86/kernel/apic/msi.c
index 9b18be764422..80067873cfd5 100644
--- a/arch/x86/kernel/apic/msi.c
+++ b/arch/x86/kernel/apic/msi.c
@@ -111,6 +111,21 @@  int pci_msi_prepare(struct irq_domain *domain, struct device *dev, int nvec,
 		arg->flags |= X86_IRQ_ALLOC_CONTIGUOUS_VECTORS;
 	}
 
+	if (pdev->align_msi_vector) {
+		/* We have specific alignment requirements on the vector
+		 * number used by the device. Set up a bitmap that restricts
+		 * the vector selection accordingly.
+		 */
+		int i = pdev->align_msi_vector;
+
+		set_bit(0, arg->allowed_vectors);
+		for (; i < NR_VECTORS; i += pdev->align_msi_vector)
+			set_bit(i, arg->allowed_vectors);
+	} else {
+		/* No specific alignment requirements so allow all vectors. */
+		bitmap_fill(arg->allowed_vectors, NR_VECTORS);
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pci_msi_prepare);
diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index 88c214e75a6b..64ddac198c25 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -104,7 +104,8 @@  static void free_apic_chip_data(struct apic_chip_data *data)
 
 static int __assign_irq_vector(int irq, struct apic_chip_data *d,
 			       const struct cpumask *mask,
-			       struct irq_data *irqdata)
+			       struct irq_data *irqdata,
+			       unsigned long *allowed_vectors)
 {
 	/*
 	 * NOTE! The local APIC isn't very good at handling
@@ -178,6 +179,9 @@  static int __assign_irq_vector(int irq, struct apic_chip_data *d,
 		if (test_bit(vector, used_vectors))
 			goto next;
 
+		if (allowed_vectors && !test_bit(vector, allowed_vectors))
+			goto next;
+
 		for_each_cpu(new_cpu, vector_searchmask) {
 			if (!IS_ERR_OR_NULL(per_cpu(vector_irq, new_cpu)[vector]))
 				goto next;
@@ -234,13 +238,14 @@  static int __assign_irq_vector(int irq, struct apic_chip_data *d,
 
 static int assign_irq_vector(int irq, struct apic_chip_data *data,
 			     const struct cpumask *mask,
-			     struct irq_data *irqdata)
+			     struct irq_data *irqdata,
+			     unsigned long *allowed_vectors)
 {
 	int err;
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&vector_lock, flags);
-	err = __assign_irq_vector(irq, data, mask, irqdata);
+	err = __assign_irq_vector(irq, data, mask, irqdata, allowed_vectors);
 	raw_spin_unlock_irqrestore(&vector_lock, flags);
 	return err;
 }
@@ -250,12 +255,25 @@  static int assign_irq_vector_policy(int irq, int node,
 				    struct irq_alloc_info *info,
 				    struct irq_data *irqdata)
 {
+	unsigned long *allowed_vectors = NULL;
+
+	/* Some MSI interrupts have restrictions on which vector numbers
+	 * can be used.
+	 */
+	if (info &&
+		(info->type == X86_IRQ_ALLOC_TYPE_MSI ||
+		 info->type == X86_IRQ_ALLOC_TYPE_MSIX))
+		allowed_vectors = info->allowed_vectors;
+
 	if (info && info->mask)
-		return assign_irq_vector(irq, data, info->mask, irqdata);
+		return assign_irq_vector(irq, data, info->mask, irqdata,
+					 allowed_vectors);
 	if (node != NUMA_NO_NODE &&
-	    assign_irq_vector(irq, data, cpumask_of_node(node), irqdata) == 0)
+	    assign_irq_vector(irq, data, cpumask_of_node(node), irqdata,
+			      allowed_vectors) == 0)
 		return 0;
-	return assign_irq_vector(irq, data, apic->target_cpus(), irqdata);
+	return assign_irq_vector(irq, data, apic->target_cpus(), irqdata,
+				 allowed_vectors);
 }
 
 static void clear_irq_vector(int irq, struct apic_chip_data *data)
@@ -549,7 +567,7 @@  static int apic_set_affinity(struct irq_data *irq_data,
 	if (!cpumask_intersects(dest, cpu_online_mask))
 		return -EINVAL;
 
-	err = assign_irq_vector(irq, data, dest, irq_data);
+	err = assign_irq_vector(irq, data, dest, irq_data, NULL);
 	return err ? err : IRQ_SET_MASK_OK;
 }
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index f68c58a93dd0..7755450be02d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -419,6 +419,8 @@  struct pci_dev {
 #endif
 #ifdef CONFIG_PCI_MSI
 	const struct attribute_group **msi_irq_groups;
+	int align_msi_vector;	/* device requires MSI vector numbers aligned
+				 * by this amount */
 #endif
 	struct pci_vpd *vpd;
 #ifdef CONFIG_PCI_ATS