From: Jonathan Currier dullfire@yahoo.com
Commit 7d5ec3d36123 ("PCI/MSI: Mask all unused MSI-X entries") introduces a readl() from ENTRY_VECTOR_CTRL before the writel() to ENTRY_DATA. This is correct, however some hardware, like the Sun Neptune chips, the niu module, will cause an error and/or fatal trap if any MSIX table entry is read before the corresponding ENTRY_DATA field is written to. This patch adds an optional early writel() in msix_prepare_msi_desc().
Cc: stable@vger.kernel.org Signed-off-by: Jonathan Currier dullfire@yahoo.com --- drivers/pci/msi/msi.c | 2 ++ include/linux/pci.h | 2 ++ 2 files changed, 4 insertions(+)
diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c index 3a45879d85db..50d87fb5e37f 100644 --- a/drivers/pci/msi/msi.c +++ b/drivers/pci/msi/msi.c @@ -611,6 +611,8 @@ void msix_prepare_msi_desc(struct pci_dev *dev, struct msi_desc *desc) if (desc->pci.msi_attrib.can_mask) { void __iomem *addr = pci_msix_desc_addr(desc);
+ if (dev->dev_flags & PCI_DEV_FLAGS_MSIX_TOUCH_ENTRY_DATA_FIRST) + writel(0, addr + PCI_MSIX_ENTRY_DATA); desc->pci.msix_ctrl = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL); } } diff --git a/include/linux/pci.h b/include/linux/pci.h index 37d97bef060f..b8b95b58d522 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -245,6 +245,8 @@ enum pci_dev_flags { PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11), /* Device does honor MSI masking despite saying otherwise */ PCI_DEV_FLAGS_HAS_MSI_MASKING = (__force pci_dev_flags_t) (1 << 12), + /* Device requires write to PCI_MSIX_ENTRY_DATA before any MSIX reads */ + PCI_DEV_FLAGS_MSIX_TOUCH_ENTRY_DATA_FIRST = (__force pci_dev_flags_t) (1 << 13), };
enum pci_irq_reroute_variant {
On 11/18/24 00:48, dullfire@yahoo.com wrote:
From: Jonathan Currier dullfire@yahoo.com
Commit 7d5ec3d36123 ("PCI/MSI: Mask all unused MSI-X entries") introduces a readl() from ENTRY_VECTOR_CTRL before the writel() to ENTRY_DATA. This is correct, however some hardware, like the Sun Neptune chips, the niu module, will cause an error and/or fatal trap if any MSIX table entry is read before the corresponding ENTRY_DATA field is written to. This patch adds an optional early writel() in msix_prepare_msi_desc().
Why the issue can't be addressed into the relevant device driver? It looks like an H/W bug, a driver specific fix looks IMHO more fitting.
A cross subsystem series, like this one, gives some extra complication to maintainers.
Thanks,
Paolo
On 11/21/24 02:55, Paolo Abeni wrote:
On 11/18/24 00:48, dullfire@yahoo.com wrote:
From: Jonathan Currier dullfire@yahoo.com
Commit 7d5ec3d36123 ("PCI/MSI: Mask all unused MSI-X entries") introduces a readl() from ENTRY_VECTOR_CTRL before the writel() to ENTRY_DATA. This is correct, however some hardware, like the Sun Neptune chips, the niu module, will cause an error and/or fatal trap if any MSIX table entry is read before the corresponding ENTRY_DATA field is written to. This patch adds an optional early writel() in msix_prepare_msi_desc().
Why the issue can't be addressed into the relevant device driver? It looks like an H/W bug, a driver specific fix looks IMHO more fitting.
I considered this approach, and thus asked about it in the mailing lists here: https://lore.kernel.org/sparclinux/7de14cca-e2fa-49f7-b83e-5f8322cc9e56@yaho... It sounds like you are suggesting approach 1 (add the MSIX register writes to niu). I did do a quick and dirty test of that. However it ended up requiring msix_map_region(), and pci_msix_desc_addr(), both of are internal to the msi code, and not exported or in pubic headers. The msix_table_size() macro was also needed. I could either export those functions, or reproduce their code in the niu driver. However, as Thomas' suggestion seemed very simple and elegant to me, I decided to got with it.
If it is actually preferable to either copy those msix functions into niu, they are not very large, or export them (GPL I would assume?) let me know and I can make that change.
A cross subsystem series, like this one, gives some extra complication to maintainers.
Thanks,
Paolo
Sincerely, Jonathan Currier
On 11/21/24 10:22, Dullfire wrote:
On 11/21/24 02:55, Paolo Abeni wrote:
On 11/18/24 00:48, dullfire@yahoo.com wrote:
From: Jonathan Currier dullfire@yahoo.com
Commit 7d5ec3d36123 ("PCI/MSI: Mask all unused MSI-X entries") introduces a readl() from ENTRY_VECTOR_CTRL before the writel() to ENTRY_DATA. This is correct, however some hardware, like the Sun Neptune chips, the niu module, will cause an error and/or fatal trap if any MSIX table entry is read before the corresponding ENTRY_DATA field is written to. This patch adds an optional early writel() in msix_prepare_msi_desc().
Why the issue can't be addressed into the relevant device driver? It looks like an H/W bug, a driver specific fix looks IMHO more fitting.
I considered this approach, and thus asked about it in the mailing lists here: https://lore.kernel.org/sparclinux/7de14cca-e2fa-49f7-b83e-5f8322cc9e56@yaho...
I forgot about such thread, thank you for the reminder. Since the more hackish code is IRQ specific, if Thomas is fine with that, I'll not oppose.
A cross subsystem series, like this one, gives some extra complication to maintainers.
The niu driver is not exactly under very active development, I guess the whole series could go via the IRQ subsystem, if Thomas agrees.
Cheers,
Paolo
On 11/21/24 04:28, Paolo Abeni wrote:
On 11/21/24 10:22, Dullfire wrote:
On 11/21/24 02:55, Paolo Abeni wrote:
On 11/18/24 00:48, dullfire@yahoo.com wrote:
From: Jonathan Currier dullfire@yahoo.com
Commit 7d5ec3d36123 ("PCI/MSI: Mask all unused MSI-X entries") introduces a readl() from ENTRY_VECTOR_CTRL before the writel() to ENTRY_DATA. This is correct, however some hardware, like the Sun Neptune chips, the niu module, will cause an error and/or fatal trap if any MSIX table entry is read before the corresponding ENTRY_DATA field is written to. This patch adds an optional early writel() in msix_prepare_msi_desc().
Why the issue can't be addressed into the relevant device driver? It looks like an H/W bug, a driver specific fix looks IMHO more fitting.
I considered this approach, and thus asked about it in the mailing lists here: https://lore.kernel.org/sparclinux/7de14cca-e2fa-49f7-b83e-5f8322cc9e56@yaho...
I forgot about such thread, thank you for the reminder. Since the more hackish code is IRQ specific, if Thomas is fine with that, I'll not oppose.
A cross subsystem series, like this one, gives some extra complication to maintainers.
The niu driver is not exactly under very active development, I guess the whole series could go via the IRQ subsystem, if Thomas agrees.
Cheers,
Paolo
Thomas, does this work for you, or is there something else you would to see in this series?
Sincerely, Jonathan Currier
linux-stable-mirror@lists.linaro.org
-
Dullfire -
dullfire＠yahoo.com
-
Paolo Abeni