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
