[1/2] PCI/MSI: Add MSIX option to write to ENTRY_DATA before any reads

Commit Message

Dullfire Nov. 17, 2024, 11:48 p.m. UTC

From: Jonathan Currier <[e-mail]>

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: [e-mail]
Signed-off-by: Jonathan Currier <[e-mail]>
---
 drivers/pci/msi/msi.c | 2 ++
 include/linux/pci.h   | 2 ++
 2 files changed, 4 insertions(+)

Comments

Paolo Abeni Nov. 21, 2024, 8:55 a.m. UTC | #1

On 11/18/24 00:48, [e-mail] wrote:
> From: Jonathan Currier <[e-mail]>
> 
> 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

Dullfire Nov. 21, 2024, 9:22 a.m. UTC | #2

On 11/21/24 02:55, Paolo Abeni wrote:
>
> On 11/18/24 00:48, [e-mail] wrote:
>> From: Jonathan Currier <[e-mail]>
>>
>> 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/[e-mail]/T/
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

Paolo Abeni Nov. 21, 2024, 10:28 a.m. UTC | #3

On 11/21/24 10:22, Dullfire wrote:
> On 11/21/24 02:55, Paolo Abeni wrote:
>> On 11/18/24 00:48, [e-mail] wrote:
>>> From: Jonathan Currier <[e-mail]>
>>>
>>> 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/[e-mail]/T/

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

Patch

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 {