[PATCH 0/2] Fix kernel panic on niu bind

[PATCH 0/2] Fix kernel panic on niu bind

[dullfire]

From: Jonathan Currier <dullfire@yahoo.com>

Currently, the niu module causes a fatal trap (tested on SPARC system)
when setting up it's MSIX interrupt vectors. A simple write to each
MSIX's vector table entries' ENTRY_DATA register is sufficient to work
around the issue for the current power cycle (note: this means booting a
working kernel, and then rebooting without power cycling will allow a
kernel without this fix to boot and work).

This series implements a struct pci_dev flag indicating ENTRY_DATA must be
written to before reads, and then sets the flag in the niu driver.

This series is based off of and tested on v6.11.5. 

Testing on next-20241115 was also done successfully with the following
caveats: On my test case (SPARC T2),
commit 03cfe0e05650 ("PCI/pwrctl: Ensure that the pwrctl drivers are probed before the PCI client drivers")
prevented all PCIe drivers from binding, including niu.
For my test case I disabled 03cfe0e05650's device_link_add().

Original mailing list discussion:
Link: https://lore.kernel.org/sparclinux/7de14cca-e2fa-49f7-b83e-5f8322cc9e56@yahoo.com/T/

Jonathan Currier (2):
  PCI/MSI: Add MSIX option to write to ENTRY_DATA before any reads
  net/niu: niu requires MSIX ENTRY_DATA fields touch before entry reads

 drivers/net/ethernet/sun/niu.c | 2 ++
 drivers/pci/msi/msi.c          | 2 ++
 include/linux/pci.h            | 2 ++
 3 files changed, 6 insertions(+)


base-commit: 05b1367d372aca98a4e09c1a0e7ff0b9d721b2bc
-- 
2.45.2

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

[dullfire]

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 {
-- 
2.45.2

[PATCH 2/2] net/niu: niu requires MSIX ENTRY_DATA fields touch before entry reads

[dullfire]

From: Jonathan Currier <dullfire@yahoo.com>

Fix niu_try_msix() to not cause a fatal trap on sparc systems.

Set PCI_DEV_FLAGS_MSIX_TOUCH_ENTRY_DATA_FIRST on the struct pci_dev to
work around a bug in the hardware or firmware.

For each vector entry in the msix table, niu chips will cause a fatal
trap if any registers in that entry are read before that entries'
ENTRY_DATA register is written to. Testing indicates writes to other
registers are not sufficient to prevent the fatal trap, however the value
does not appear to matter. This only needs to happen once after power up,
so simply rebooting into a kernel lacking this fix will NOT cause the
trap.

NON-RESUMABLE ERROR: Reporting on cpu 64
NON-RESUMABLE ERROR: TPC [0x00000000005f6900] <msix_prepare_msi_desc+0x90/0xa0>
NON-RESUMABLE ERROR: RAW [4010000000000016:00000e37f93e32ff:0000000202000080:ffffffffffffffff
NON-RESUMABLE ERROR:      0000000800000000:0000000000000000:0000000000000000:0000000000000000]
NON-RESUMABLE ERROR: handle [0x4010000000000016] stick [0x00000e37f93e32ff]
NON-RESUMABLE ERROR: type [precise nonresumable]
NON-RESUMABLE ERROR: attrs [0x02000080] < ASI sp-faulted priv >
NON-RESUMABLE ERROR: raddr [0xffffffffffffffff]
NON-RESUMABLE ERROR: insn effective address [0x000000c50020000c]
NON-RESUMABLE ERROR: size [0x8]
NON-RESUMABLE ERROR: asi [0x00]
CPU: 64 UID: 0 PID: 745 Comm: kworker/64:1 Not tainted 6.11.5 #63
Workqueue: events work_for_cpu_fn
TSTATE: 0000000011001602 TPC: 00000000005f6900 TNPC: 00000000005f6904 Y: 00000000    Not tainted
TPC: <msix_prepare_msi_desc+0x90/0xa0>
g0: 00000000000002e9 g1: 000000000000000c g2: 000000c50020000c g3: 0000000000000100
g4: ffff8000470307c0 g5: ffff800fec5be000 g6: ffff800047a08000 g7: 0000000000000000
o0: ffff800014feb000 o1: ffff800047a0b620 o2: 0000000000000011 o3: ffff800047a0b620
o4: 0000000000000080 o5: 0000000000000011 sp: ffff800047a0ad51 ret_pc: 00000000005f7128
RPC: <__pci_enable_msix_range+0x3cc/0x460>
l0: 000000000000000d l1: 000000000000c01f l2: ffff800014feb0a8 l3: 0000000000000020
l4: 000000000000c000 l5: 0000000000000001 l6: 0000000020000000 l7: ffff800047a0b734
i0: ffff800014feb000 i1: ffff800047a0b730 i2: 0000000000000001 i3: 000000000000000d
i4: 0000000000000000 i5: 0000000000000000 i6: ffff800047a0ae81 i7: 00000000101888b0
I7: <niu_try_msix.constprop.0+0xc0/0x130 [niu]>
Call Trace:
[<00000000101888b0>] niu_try_msix.constprop.0+0xc0/0x130 [niu]
[<000000001018f840>] niu_get_invariants+0x183c/0x207c [niu]
[<00000000101902fc>] niu_pci_init_one+0x27c/0x2fc [niu]
[<00000000005ef3e4>] local_pci_probe+0x28/0x74
[<0000000000469240>] work_for_cpu_fn+0x8/0x1c
[<000000000046b008>] process_scheduled_works+0x144/0x210
[<000000000046b518>] worker_thread+0x13c/0x1c0
[<00000000004710e0>] kthread+0xb8/0xc8
[<00000000004060c8>] ret_from_fork+0x1c/0x2c
[<0000000000000000>] 0x0
Kernel panic - not syncing: Non-resumable error.

Fixes: 7d5ec3d36123 ("PCI/MSI: Mask all unused MSI-X entries")
Cc: stable@vger.kernel.org
Signed-off-by: Jonathan Currier <dullfire@yahoo.com>
---
 drivers/net/ethernet/sun/niu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
index 41a27ae58ced..f5449b73b9a7 100644
--- a/drivers/net/ethernet/sun/niu.c
+++ b/drivers/net/ethernet/sun/niu.c
@@ -9058,6 +9058,8 @@ static void niu_try_msix(struct niu *np, u8 *ldg_num_map)
 		msi_vec[i].entry = i;
 	}
 
+	pdev->dev_flags |= PCI_DEV_FLAGS_MSIX_TOUCH_ENTRY_DATA_FIRST;
+
 	num_irqs = pci_enable_msix_range(pdev, msi_vec, 1, num_irqs);
 	if (num_irqs < 0) {
 		np->flags &= ~NIU_FLAGS_MSIX;
-- 
2.45.2

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

[Paolo Abeni]

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

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

[Dullfire]

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@yahoo.com/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

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

[Paolo Abeni]

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@yahoo.com/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

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

[Dullfire]

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@yahoo.com/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
> 

Thomas, does this work for you, or is there something else you would to see in this series?


Sincerely,
Jonathan Currier

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

[Dullfire]

On 1/20/25 6:38 AM, Dullfire wrote:
> 
> On 11/21/24 04:28, Paolo Abeni wrote:
[...]
>> 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?


Since it's been a few months, just wanted to give this a bump, and see if the series is lacking anything.
Thanks,

Sincerely,
Jonathan Currier

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

[Thomas Gleixner]

On Mon, Apr 14 2025 at 11:22, dullfire@yahoo.com wrote:
> On 1/20/25 6:38 AM, Dullfire wrote:
>> On 11/21/24 04:28, Paolo Abeni wrote:
> [...]
>>> 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?
>
> Since it's been a few months, just wanted to give this a bump, and see if the series is lacking anything.
> Thanks,

Sorry for letting this fall through the cracks. I put it back on my todo
list...

Thanks,

        tglx