| Message ID | 20170925041153.26574-1-drake@endlessm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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
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 --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
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.