[lkml]   [2017]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] PCI MSI: allow alignment restrictions on vector allocation
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:// 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
> 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.



 \ /
  Last update: 2017-09-27 17:29    [W:0.198 / U:0.316 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site