* Re: [PATCH v2] powerpc/64s: Clarify copy_and_flush() cache sync loop comment
From: Aditya Gupta @ 2026-07-17 6:40 UTC (permalink / raw)
To: Nikhil Kumar Singh, linuxppc-dev, mkchauras
Cc: linux-kernel, maddy, mpe, npiggin, chleroy, mahesh
In-Reply-To: <20260706082708.43918-1-nikhilks@linux.ibm.com>
On 06/07/26 13:57, Nikhil Kumar Singh wrote:
> The value loaded into r0 in copy_and_flush() represents the number of
> 8-byte words processed between cache synchronization operations.
>
> The existing comment refers to cache line size, which can make it appear
> that the value is a cache line size in bytes rather than a loop count.
> Clarify the comment to explain that the loop processes 8 words (64 bytes)
> per cache synchronization iteration, and that increasing the value would
> skip cache maintenance for intermediate cache lines.
>
> This is a comment-only change with no functional impact.
>
> Signed-off-by: Nikhil Kumar Singh <nikhilks@linux.ibm.com>
> ---
> arch/powerpc/kernel/head_64.S | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
> index 63432a33ec49..a54f6f979173 100644
> --- a/arch/powerpc/kernel/head_64.S
> +++ b/arch/powerpc/kernel/head_64.S
> @@ -713,14 +713,18 @@ p_end: .8byte _end - copy_to_here
> _GLOBAL(copy_and_flush)
> addi r5,r5,-8
> addi r6,r6,-8
> -4: li r0,8 /* Use the smallest common */
> - /* denominator cache line */
> - /* size. This results in */
> - /* extra cache line flushes */
> - /* but operation is correct. */
> - /* Can't get cache line size */
> - /* from NACA as it is being */
> - /* moved too. */
> +4: li r0,8 /* r0 is the number of 8-byte words */
> + /* to copy per cache sync iteration. */
> + /* 8 words * 8 bytes = 64 bytes. 64B is */
> + /* the current default cache line size. */
> + /* This is a loop count, not a byte */
> + /* count. Increasing it may skip */
> + /* dcbst/icbi for lines in between and */
> + /* leave stale instructions in icache. */
> + /* This results in extra cache line */
> + /* flushes but operation is correct. */
> + /* Can't get cache line size from NACA */
> + /* as it is being moved too. */
>
> mtctr r0 /* put # words/line in ctr */
> 3: addi r6,r6,8 /* copy a cache line */
Makes sense. Feel free to keep the rb.
Reviewed-by: Aditya Gupta <adityag@linux.ibm.com>
Thanks,
- Aditya G
^ permalink raw reply
* [GIT PULL] Please pull powerpc/linux.git powerpc-7.2-2 tag
From: Madhavan Srinivasan @ 2026-07-17 5:01 UTC (permalink / raw)
To: Linus Torvalds
Cc: amachhiw, chleroy, christophe.leroy, enelsonmoore, gautam,
linux-kernel, linuxppc-dev, moonafterrain, mpe, naveen, npiggin,
rosenp, sshegde, thorsten.blum
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Hi Linus,
Please pull powerpc fixes for 7.2:
The following changes since commit 8cdeaa50eae8dad34885515f62559ee83e7e8dda:
Linux 7.2-rc2 (2026-07-05 14:44:06 -1000)
are available in the git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git
tags/powerpc-7.2-2
for you to fetch changes up to a2c02aa0c6ca3ec9fab6f1c99912a440c7b8bfdb:
powerpc: Remove dead non-preemption code (2026-07-15 08:04:13 +0530)
- ------------------------------------------------------------------
powerpc fixes for 7.2 #2
- Enable CONFIG_VPA_PMU to be used with KVM
- Initialize starttime at boot for native accounting
- Set CPU_FTR_P11_PVR for Power11 and later processors
- fix memory leak on krealloc failure in papr_init
- Misc fixes and cleanups
Thanks to:
Amit Machhiwal, Christophe Leroy (CS GROUP), Ethan Nelson-Moore, Gautam
Menghani, Harsh Prateek Bora, Junrui Luo, Mukesh Kumar Chaurasiya (IBM),
Ritesh
Harjani (IBM), Rosen Penev, Shrikanth Hegde, Thorsten Blum, Yuhao Jiang
- ------------------------------------------------------------------
Amit Machhiwal (1):
powerpc/dt_cpu_ftrs: Set CPU_FTR_P11_PVR for Power11 and later
processors
Christophe Leroy (CS GROUP) (1):
powerpc: Remove dead non-preemption code
Ethan Nelson-Moore (1):
powerpc/uaccess: correct check for CONFIG_PPC_E500 in
mask_user_address()
Gautam Menghani (1):
powerpc/pseries/Kconfig: Enable CONFIG_VPA_PMU to be used with KVM
Junrui Luo (1):
powerpc/spufs: fix out-of-bounds access in spufs_mem_mmap_access()
Rosen Penev (1):
powerpc/85xx: Add fsl,ifc to common device ids
Shrikanth Hegde (1):
powerpc/vtime: Initialize starttime at boot for native accounting
Thorsten Blum (1):
powerpc/pseries: fix memory leak on krealloc failure in papr_init
arch/powerpc/include/asm/preempt.h | 16
----------------
arch/powerpc/include/asm/uaccess.h | 2 +-
arch/powerpc/kernel/dt_cpu_ftrs.c | 9 +++++++++
arch/powerpc/kernel/time.c | 4 +++-
arch/powerpc/lib/vmx-helper.c | 2 +-
arch/powerpc/platforms/85xx/common.c | 2 ++
arch/powerpc/platforms/cell/spufs/file.c | 6 ++++--
arch/powerpc/platforms/pseries/Kconfig | 1 +
arch/powerpc/platforms/pseries/papr_platform_attributes.c | 8 +++-----
9 files changed, 24 insertions(+), 26 deletions(-)
delete mode 100644 arch/powerpc/include/asm/preempt.h
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCgAdFiEEqX2DNAOgU8sBX3pRpnEsdPSHZJQFAmpZtjMACgkQpnEsdPSH
ZJT+iBAAhKtQIVYpMtOUqzKjt0DwZaaayEMxKcgVCa07wolje/x50bC8l0D5Kpoi
QmQexojCT/00cfDkK7tiC+GUd1yr2eBMW/8AbBw3DqKBBdIiNNieWm//oRu5Zc7m
cPhrm17u0xTmZbzKeMp1fRsPZm7b+AwJmXoXj0B8wkfQiY1QKF8uR80xtNpx4Hsq
LOZQ4AjeOJOlMm/eprJlg657sLgPrBHIk+OZeO9LsU8UesRfpaA7swles4IwHRIU
3Gi7gQoAu9H7vl+9cSjbWbax7wCbmxAm94dNe0lPQ8awG0IyXKYRzYuE/J/By+hv
WjnHLFpWQYCjDwTrt4saZbplgzaiG7HO7GGHjcVMcmS6/qE41MiivRa8NHb/aMdJ
QO1vGw7QKwOE4zcvW6ZRZtYSjs544qr0Sxo46tfTg9iTTdkFDA4cmuqmZw7Msu2Q
EliKl7NTf/PY2BSYJzaPsIQyxIml0dQugAbHkjYbr/Uk4o+DztFOjapn3RbO1OLu
DpSW5N2Wc+ja3VDarr9oMAPUWt4IA6vfsGC9nXk2OhwAfK4P+h9asTLeRfNXTGFb
ZrzrfZkyxzgFsg/D2In/oT3OLOJqWneRvJkDXq22pR/kpYOkVfpw6kAat6hTT96U
9w2jPZF1L6o7b3PezyNqkJt80TfRrscJrUglYfZ/o+QHRDrqbLg=
=/Wf4
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: powerpc/pseries: EEH detected with xhci_hcd
From: Narayana Murty N @ 2026-07-17 4:56 UTC (permalink / raw)
To: Sourabh Jain, Mahesh J Salgaonkar, linuxppc-dev,
Ritesh Harjani (IBM), Venkat Rao Bagalkote, Nicholas Piggin,
Michael Ellerman, shivang upadhyay, sbhat
In-Reply-To: <29f33de0-0861-459f-b6a0-440354fac785@linux.ibm.com>
On 15/07/26 7:33 PM, Sourabh Jain wrote:
>
> This issue only occurs with the Radix MMU.
> Booting the same kernel with disable_radix=1 does not reproduce the
> issue.
>
> The system also has pmem.
>
> +CC Shivang and Shiva
>
> - Sourabh Jain
>
>
> On 15/07/26 15:01, Sourabh Jain wrote:
>> Hello,
>>
>> Upstream kernel with head at commit
>> 3b029c035b34bbc693405ddf759f0e9b920c27f1
>> hitting EEH with xhci_hcd during kernel boot.
>>
>> The issue was not observed on the distro kernel running on the the
>> same machine.
>>
>> Trace:
>> [ 2.130359] [ C8] ibmvscsi 30000066: SRP_LOGIN succeeded
>> [ 2.132280] [ T362] xhci_hcd 0014:01:00.0: xHCI Host Controller
>> [ 2.132289] [ T362] xhci_hcd 0014:01:00.0: new USB bus
>> registered, assigned bus number 1
>> [ 2.132449] [ T362] xhci_hcd 0014:01:00.0:
>> ibm,query-pe-dma-windows(53) 10000 8000000 20000014 returned 0,
>> lb=1000000 ps=103 wn=1
>> [ 2.132453] [ T362] xhci_hcd 0014:01:00.0: Skipping ibm,pmemory
>> [ 2.141861] [ T110] scsi 0:0:1:0: Direct-Access AIX VDASD
>> 0001 PQ: 0 ANSI: 3
>> [ 2.143707] [ T362] xhci_hcd 0014:01:00.0:
>> ibm,create-pe-dma-window(54) 10000 8000000 20000014 15 28 returned 0
>> (liobn = 0x70000014 starting addr = 8000000 0)
>> [ 2.147236] [ T362] xhci_hcd 0014:01:00.0: lsa_required: 0,
>> lsa_enabled: 0, direct mapping: 1
>> [ 2.147239] [ T362] xhci_hcd 0014:01:00.0: iommu: 64-bit OK but
>> direct DMA is limited by 800010000000000
>> [ 2.147250] [ T362] xhci_hcd 0014:01:00.0: lsa_required: 0,
>> lsa_enabled: 0, direct mapping: 1
>> [ 2.147252] [ T362] xhci_hcd 0014:01:00.0: iommu: 64-bit OK but
>> direct DMA is limited by 800010000000000
>> [ 2.147476] [ T362] xhci_hcd 0014:01:00.0: hcc params
>> 0x0270f06d hci version 0x96 quirks 0x0000000004000000
>> [ 2.149530] [ T362] xhci_hcd 0014:01:00.0: xHCI Host Controller
>> [ 2.149536] [ T362] xhci_hcd 0014:01:00.0: new USB bus
>> registered, assigned bus number 2
>> [ 2.149542] [ T362] xhci_hcd 0014:01:00.0: Host supports USB
>> 3.0 SuperSpeed
>> [ 2.149638] [ T362] usb usb1: New USB device found,
>> idVendor=1d6b, idProduct=0002, bcdDevice= 7.02
>> [ 2.149642] [ T362] usb usb1: New USB device strings: Mfr=3,
>> Product=2, SerialNumber=1
>> [ 2.149644] [ T362] usb usb1: Product: xHCI Host Controller
>> [ 2.149646] [ T362] usb usb1: Manufacturer: Linux
>> 7.2.0-rc3upstream+ xhci-hcd
>> [ 2.149647] [ T362] usb usb1: SerialNumber: 0014:01:00.0
>> [ 2.149817] [ T362] hub 1-0:1.0: USB hub found
>> [ 2.149937] [ T362] hub 1-0:1.0: 4 ports detected
>> [ 2.150020] [ T362] xhci_hcd 0014:01:00.0: xHCI host controller
>> not responding, assume dead
>> [ 2.150044] [ T362] xhci_hcd 0014:01:00.0: HC died; cleaning up
>> [ 2.150125] [ T118] EEH: Recovering PHB#14-PE#10000
>> [ 2.150135] [ T118] EEH: PE location: N/A, PHB location: N/A
>> [ 2.150143] [ T118] EEH: Frozen PHB#14-PE#10000 detected
>> [ 2.150149] [ T118] EEH: Call Trace:
>> [ 2.150154] [ T362] xhci_hcd 0014:01:00.0: HC died; cleaning up
>> [ 2.150154] [ T118] EEH: [0000000066e7b6f8]
>> __eeh_send_failure_event+0xa8/0x190
>> [ 2.150167] [ T362] xhci_hcd 0014:01:00.0: USB bus 2 deregistered
>> [ 2.150171] [ T362] xhci_hcd 0014:01:00.0: remove, state 1
>> [ 2.150174] [ T118] EEH: [000000004754a593]
>> eeh_dev_check_failure+0x408/0x7a0
>> [ 2.150177] [ T362] usb usb1: USB disconnect, device number 1
>> [ 2.150185] [ T118] EEH: [00000000063df7cc]
>> xhci_portsc_readl+0x64/0x90 [xhci_hcd]
>> [ 2.150216] [ T118] EEH: [00000000593ad484]
>> xhci_hub_control+0xe28/0x27f0 [xhci_hcd]
>> [ 2.150244] [ T118] EEH: [00000000190f1012]
>> usb_hcd_submit_urb+0x508/0xd30 [usbcore]
>> [ 2.150276] [ T118] EEH: [0000000072d7c35d]
>> usb_submit_urb+0x254/0x840 [usbcore]
>> [ 2.150303] [ T118] EEH: [0000000009787909]
>> usb_start_wait_urb+0x88/0x270 [usbcore]
>> [ 2.150331] [ T118] EEH: [00000000e6ec433e]
>> usb_control_msg+0x134/0x1f0 [usbcore]
>> [ 2.150357] [ T118] EEH: [00000000d77b1b30]
>> hub_probe+0x570/0x12b8 [usbcore]
>> [ 2.150385] [ T118] EEH: [0000000091c7d281]
>> usb_probe_interface+0x124/0x490 [usbcore]
>> [ 2.150412] [ T118] EEH: [00000000dc656a09]
>> really_probe+0x118/0x580
>> [ 2.150425] [ T118] EEH: [00000000131d8dfb]
>> __driver_probe_device+0xc4/0x270
>> [ 2.150434] [ T118] EEH: [00000000e26dfd1d]
>> driver_probe_device+0x5c/0x110
>> [ 2.150442] [ T118] EEH: [000000006312c397]
>> __device_attach_driver+0x10c/0x210
>> [ 2.150451] [ T118] EEH: [000000004edd640b]
>> bus_for_each_drv+0xb0/0x130
>> [ 2.150460] [ T118] EEH: [000000009b173e09]
>> __device_attach+0xdc/0x2a0
>> [ 2.150467] [ T118] EEH: [000000004588b69c]
>> device_initial_probe+0x70/0x80
>> [ 2.150473] [ T118] EEH: [000000003dba8fbb]
>> bus_probe_device+0x50/0xf0
>> [ 2.150479] [ T118] EEH: [00000000f1fd4a53] device_add+0x8f0/0xc00
>> [ 2.150486] [ T118] EEH: [00000000a74fc843]
>> usb_set_configuration+0x78c/0xb60 [usbcore]
>> [ 2.150526] [ T118] EEH: [000000001df4c94f]
>> usb_generic_driver_probe+0x78/0xd0 [usbcore]
>> [ 2.150556] [ T118] EEH: [00000000a7086db7]
>> usb_probe_device+0x60/0x200 [usbcore]
>> [ 2.150584] [ T118] EEH: [00000000dc656a09]
>> really_probe+0x118/0x580
>> [ 2.150591] [ T118] EEH: [00000000131d8dfb]
>> __driver_probe_device+0xc4/0x270
>> [ 2.150603] [ T118] EEH: [00000000e26dfd1d]
>> driver_probe_device+0x5c/0x110
>> [ 2.150611] [ T118] EEH: [000000006312c397]
>> __device_attach_driver+0x10c/0x210
>> [ 2.150620] [ T118] EEH: [000000004edd640b]
>> bus_for_each_drv+0xb0/0x130
>> [ 2.150629] [ T118] EEH: [000000009b173e09]
>> __device_attach+0xdc/0x2a0
>> [ 2.150636] [ T118] EEH: [000000004588b69c]
>> device_initial_probe+0x70/0x80
>> [ 2.150648] [ T118] EEH: [000000003dba8fbb]
>> bus_probe_device+0x50/0xf0
>> [ 2.150655] [ T118] EEH: [00000000f1fd4a53] device_add+0x8f0/0xc00
>> [ 2.150664] [ T118] EEH: [000000005bca6639]
>> usb_new_device+0x2f4/0x890 [usbcore]
>> [ 2.150693] [ T118] EEH: [0000000023d4a3a1]
>> register_root_hub+0x1e4/0x2a0 [usbcore]
>> [ 2.150721] [ T118] EEH: [00000000941149db]
>> usb_add_hcd+0x82c/0x8f0 [usbcore]
>> [ 2.150749] [ T118] EEH: [00000000f13842f6]
>> xhci_pci_common_probe+0x1e4/0x3d0 [xhci_pci]
>> [ 2.150760] [ T118] EEH: [00000000862a8445]
>> local_pci_probe+0x64/0x100
>> [ 2.150770] [ T118] EEH: [00000000c124ce3b]
>> pci_device_probe+0x330/0x370
>> [ 2.150778] [ T118] EEH: [00000000dc656a09]
>> really_probe+0x118/0x580
>> [ 2.150792] [ T118] EEH: [00000000131d8dfb]
>> __driver_probe_device+0xc4/0x270
>> [ 2.150800] [ T118] EEH: [00000000e26dfd1d]
>> driver_probe_device+0x5c/0x110
>> [ 2.150808] [ T118] EEH: [00000000f3819f2a]
>> __driver_attach+0x12c/0x2d0
>> [ 2.150816] [ T118] EEH: [00000000b7c6a37c]
>> bus_for_each_dev+0xa4/0x130
>> [ 2.150824] [ T118] EEH: [000000009bdf640c]
>> driver_attach+0x30/0x50
>> [ 2.150836] [ T118] EEH: [0000000039921381]
>> bus_add_driver+0x1c0/0x380
>> [ 2.150844] [ T118] EEH: [0000000034a0727d]
>> driver_register+0x9c/0x1b0
>> [ 2.150852] [ T362] xhci_hcd 0014:01:00.0: USB bus 1 deregistered
>> [ 2.150851] [ T118] EEH: [000000002faf9b88]
>> __pci_register_driver+0x64/0x80
>> [ 2.150861] [ T118] EEH: [000000005142b255]
>> xhci_pci_init+0x90/0xb0 [xhci_pci]
>> [ 2.150869] [ T118] EEH: [00000000a9fe4af6]
>> do_one_initcall+0x5c/0x40c
>> [ 2.150883] [ T118] EEH: [00000000e2b1b716]
>> do_init_module+0xac/0x3c0
>> [ 2.150893] [ T118] EEH: [00000000fc06f562]
>> init_module_from_file+0x120/0x1b0
>> [ 2.150902] [ T118] EEH: [000000004ca14a9d]
>> sys_finit_module+0x1b4/0x4d0
>> [ 2.150910] [ T118] EEH: [000000005e56c4e5]
>> system_call_exception+0x184/0x430
>> [ 2.150919] [ T362] xhci_hcd 0014:01:00.0: probe with driver
>> xhci_hcd failed with error -108
>> [ 2.150930] [ T118] EEH: [0000000024ebcbb3]
>> system_call_vectored_common+0x15c/0x2ec
>> [ 2.150941] [ T118] EEH: This PCI device has failed 1 times in
>> the last hour and will be permanently disabled after 5 failures.
>> [ 2.150943] [ T118] EEH: Notify device drivers to shutdown
>> [ 2.150947] [ T118] EEH: Beginning: 'error_detected(IO frozen)'
>>
>> Snip.....
>> And after sometime the device is disabled.
>>
>> [ 40.450010] [ T118] EEH: PHB#14-PE#10000 has failed 6 times in
>> the last hour and has been permanently disabled.
>> [ 40.450027] [ T118] EEH: Unable to recover from failure from
>> PHB#14-PE#10000.
>> Please try reseating or replacing it
>>
>> Attached the kernel config and dmesg.
>>
>> - Sourabh Jain
>
Is the device EEH capable? it seems
error_detected is not invoked any driver
callbacks.
Regards,
Narayana
^ permalink raw reply
* Re: [PATCH net-next v3 15/15] ibmveth: Fix MQ RX poll and shutdown hangs after queue resize
From: mingming cao @ 2026-07-17 3:08 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, bjking1, haren, ricklind, kuba, edumazet, pabeni,
linuxppc-dev, maddy, mpe, davemarq
In-Reply-To: <20260714131053.1844276-2-horms@kernel.org>
On 7/14/26 6:10 AM, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> ibmveth: Fix MQ RX poll and shutdown hangs after queue resize
>
> This commit fixes polling hangs in the ibmveth driver after queue resize
> by skipping over bad correlators instead of retrying endlessly. It also
> ensures NAPI completes when the interface is down or being disabled.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 50a332ab83fd3..eb84b4e5f69fd 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... ]
>
>> @@ -2959,8 +2978,23 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
>> __sum16 iph_check = 0;
>>
>> skb = ibmveth_rxq_get_buffer(adapter, queue_index);
>> - if (unlikely(!skb))
>> - break;
>> + if (unlikely(!skb)) {
>> + if (net_ratelimit())
>> + netdev_err(netdev,
> [Severity: High]
> This isn't a bug introduced by this patch, but can the unvalidated packet
> lengths and offsets from hardware descriptors trigger an skb_over_panic()
> here?
yes
> Further down in ibmveth_poll(), the driver reads length and offset directly
> from the hardware receive descriptor and passes them to skb_reserve() and
> skb_put() on the reused SKB:
>
> int length = ibmveth_rxq_frame_length(adapter, queue_index);
> int offset = ibmveth_rxq_frame_offset(adapter, queue_index);
> ...
> } else {
> rc = ibmveth_rxq_harvest_buffer(adapter, queue_index, false);
> if (unlikely(rc))
> break;
> skb_reserve(skb, offset);
> }
>
> skb_put(skb, length);
>
> If a faulty hypervisor provides an abnormally large length or offset,
> skb_put() will extend the tail past the end of the buffer, which triggers an
> skb_over_panic() and causes a crash.
>
> Should the driver validate that offset + length is within the known buffer
> size before modifying the boundaries?
**
*Agreed this needs guarding. I'll add in v4: validates*
*
offset + length against skb_tailroom() before skb_reserve/skb_put, and
skips bad correlators.
*
>
> [ ... ]
>
>> @@ -3034,8 +3068,11 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
>>
>> ibmveth_replenish_task(adapter, queue_index);
>>
>> - if (frames_processed == budget)
>> + if (frames_processed == budget) {
>> + if (!netif_running(netdev) || napi_disable_pending(napi))
>> + napi_complete_done(napi, frames_processed);
>> goto out;
>> + }
> [Severity: High]
> Does calling napi_complete_done() here conflict with the NAPI core contract?
>
> If the polling loop exhausts its budget (frames_processed == budget), returning
> the full budget after calling napi_complete_done() might cause issues with the
> NAPI state machine. The NAPI core (__napi_poll) will operate on the instance
> assuming it still holds the NAPIF_STATE_SCHED bit.
>
> If napi_disable_pending() is true, the core calls napi_complete(), which will
> hit a WARN_ON_ONCE.
>
> If napi_disable_pending() is false but !netif_running() is true, the core will
> add the NAPI instance back to the poll_list. Since the driver cleared
> NAPIF_STATE_SCHED, a subsequent interrupt can trigger another napi_schedule(),
> causing a double list_add_tail() and potentially leading to list corruption.
*
Good catch — completing and then
returning a full budget is wrong. In v4 I'll complete on
shutdown/disable and return a value < budget (same as at the top of
poll), and leave the normal budget-exhausted path returning budget
without completing.
Thanks,
Mingming
*
^ permalink raw reply
* Re: [PATCH net-next v3 14/15] ibmveth: Wire ethtool set_channels to MQ RX queue resize
From: mingming cao @ 2026-07-17 3:02 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, bjking1, haren, ricklind, kuba, edumazet, pabeni,
linuxppc-dev, maddy, mpe, davemarq
In-Reply-To: <20260714130752.1844153-1-horms@kernel.org>
On 7/14/26 6:07 AM, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> ibmveth: Wire ethtool set_channels to MQ RX queue resize
>
> Expose incremental RX resize through ethtool channel control.
> get_channels() reports rx_count from adapter->num_rx_queues and max_rx
> as IBMVETH_MAX_RX_QUEUES when MQ firmware is enabled, else 1.
> set_channels() validates rx_count is within 1..IBMVETH_MAX_RX_QUEUES.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index ac4d89a66a8db..50a332ab83fd3 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
>> @@ -2534,19 +2534,69 @@ static int ibmveth_set_channels(struct net_device *netdev,
>> struct ethtool_channels *channels)
>> {
>> struct ibmveth_adapter *adapter = netdev_priv(netdev);
> [ ... ]
>> - if (!(netdev->flags & IFF_UP))
>> + if (!(netdev->flags & IFF_UP)) {
>> + if (goal_rx > 1 && !adapter->multi_queue) {
> [ ... ]
>> + /* Stash desired RX count; open() publishes it via
>> + * netif_set_real_num_rx_queues() after queue registration.
>> + */
>> + if (goal_rx != adapter->num_rx_queues)
>> + adapter->num_rx_queues = goal_rx;
>> +
>> return netif_set_real_num_tx_queues(netdev, goal);
> [Severity: Medium]
> If netif_set_real_num_tx_queues() fails and returns an error, the TX
> configuration command will fail, but the RX queue count has already been
> permanently modified in adapter->num_rx_queues. Will this leave the device in
> an inconsistent state where the new RX count is applied on the next open
> despite the command failing?
*
Agreed — will only commit the stashed RX count after
netif_set_real_num_tx_queues() succeeds, or restore on failure.
*
>> + }
> [ ... ]
>> + if (goal_rx != old_rx) {
>> + rc = ibmveth_resize_rx_queues_incremental(adapter, goal_rx,
>> + rxq_entries);
> [Severity: High]
> Does this new ethtool scale-down path expose an interrupt storm?
>
> Looking at ibmveth_resize_rx_queues_incremental(), when scaling down queues it
> disables NAPI before disabling the hardware interrupt:
>
> drivers/net/ethernet/ibm/ibmveth.c:ibmveth_resize_rx_queues_incremental() {
> ...
> for (i = new_count; i < old_count; i++)
> napi_disable(&adapter->napi[i]);
>
> for (i = new_count; i < old_count; i++)
> ibmveth_drain_rx_queue(adapter, i);
>
> synchronize_net();
> ...
> /* Disable hypervisor interrupts and wait for handlers to complete
> * before updating num_rx_queues.
> */
> for (i = new_count; i < old_count; i++) {
> ibmveth_disable_irq(adapter, i);
> ...
> }
>
> If the hypervisor delivers a packet during this window, ibmveth_interrupt()
> will execute. Since NAPI is disabled, napi_schedule_prep() fails, causing the
> handler to return IRQ_HANDLED without disabling the hardware interrupt. Will
> this leave the virtual interrupt asserted and force an infinite interrupt loop?
Yeah, *Same issue as on the incremental resize patch — **
will fix IRQ/NAPI ordering in ibmveth_resize_rx_queues_incremental()
in v4.
*
>> + if (rc) {
>> + netdev_err(netdev, "Failed to resize RX queues: %d\n", rc);
>> + return rc;
>> + }
>> + }
>>
>> /* We have IBMVETH_MAX_QUEUES netdev_queue's allocated
>> * but we may need to alloc/free the ltb's.
>> */
> [Severity: High]
> This is a pre-existing issue, but does this function use an uninitialized
> variable in the error path?
>
> When decreasing the number of TX queues (goal < old), the allocation loop for
> (i = old; i < goal; i++) is skipped, leaving 'i' uninitialized. If
> netif_set_real_num_tx_queues() then fails, the error path assigns old = i;.
>
> The cleanup loop then uses this garbage value:
> for (i = old; i > goal; i--) {
> if (adapter->tx_ltb_ptr[i - 1])
>
> Will this access adapter->tx_ltb_ptr out of bounds and pass arbitrary pointers
> to ibmveth_free_tx_ltb()?
*
Good catch — pre-existing, but we’ll fix it while touching
set_channels in v4 (initialize i / don’t use it uninitialized on
TX scale-down failure).
*
>> @@ -2580,7 +2630,7 @@ static int ibmveth_set_channels(struct net_device *netdev,
>>
>> netif_tx_wake_all_queues(netdev);
>>
>> - return rc;
>> + return 0;
> [Severity: High]
> Does this unconditional return of 0 mask TX queue configuration failures?
>
> If applying the new TX queue count via netif_set_real_num_tx_queues() fails,
> the code handles it locally, but the function exits with 0 regardless. Will
> this mislead userspace into believing the TX channel count was successfully
> modified?
*
Agreed — that return 0 is a regression versus returning rc. Will
restore proper error reporting in v4 (and be careful about partial
RX-success / TX-failure).
*
*
Thanks for your review feedback.
Mingming
*
^ permalink raw reply
* Re: [PATCH net-next v3 13/15] ibmveth: Implement incremental MQ RX queue resize
From: mingming cao @ 2026-07-17 2:40 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, bjking1, haren, ricklind, kuba, edumazet, pabeni,
linuxppc-dev, maddy, mpe, davemarq
In-Reply-To: <20260714130344.1841551-3-horms@kernel.org>
On 7/14/26 6:03 AM, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> Date: Mon Jul 6 19:36:01 2026 +0000
>
> ibmveth: Implement incremental MQ RX queue resize
>
> Add helpers to register/deregister individual RX queues and scale
> queue count up or down while the netdev stays up. Surviving queues
> keep PHYP handles, buffer pools, and IRQ state across resize.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index ecc472ee8f71f..ac4d89a66a8db 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... ]
>
>> @@ -1080,6 +1132,138 @@ static void ibmveth_free_buffer_pools(struct ibmveth_adapter *adapter)
>> +static int
>> +ibmveth_alloc_single_rx_queue(struct ibmveth_adapter *adapter, int queue_idx,
>> + int rxq_entries)
>> +{
> [ ... ]
>
>> + for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
>> + adapter->rx_buff_pool[queue_idx][i].size =
>> + adapter->rx_buff_pool[0][i].size;
>> + adapter->rx_buff_pool[queue_idx][i].buff_size =
>> + adapter->rx_buff_pool[0][i].buff_size;
>> + adapter->rx_buff_pool[queue_idx][i].threshold =
>> + adapter->rx_buff_pool[0][i].threshold;
>> + adapter->rx_buff_pool[queue_idx][i].active =
>> + adapter->rx_buff_pool[0][i].active;
>> + }
> [Severity: Critical]
> Does this loop miss initializing pool->index?
>
> If pool->index remains 0 for all buffer pools on the new queue, when
> replenishing, 0 is shifted into the correlator.
>
> Then when receiving, ibmveth_remove_buffer_from_pool() will extract pool=0,
> which fetches from the wrong pool and could corrupt the free map of pool 0,
> leading to a use-after-free or leak.
Thanks — this was a real bug. We discovered this issue while doing
stress tests, and identified
this issue, Agreed copies
*
pool->index when cloning pools for incrementally added queues; I have
verified the fix. Thanks again,
*
>
> [ ... ]
>
>> @@ -1192,6 +1376,49 @@ static int ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter,
>> +static int
>> +ibmveth_drain_rx_queue(struct ibmveth_adapter *adapter, int queue_index)
>> +{
> [ ... ]
>
>> + while (drained < limit &&
>> + ibmveth_rxq_pending_buffer(adapter, queue_index)) {
>> + rc = ibmveth_rxq_harvest_buffer(adapter, queue_index, true);
> Simon says: I think we can consider the issue below in the context of
> possible follow-up, because I believe the use of this pattern
> predates this patch.
>
> [Severity: High]
> Is a read memory barrier needed between reading the toggle bit in
> ibmveth_rxq_pending_buffer() and reading the correlator in
> ibmveth_rxq_harvest_buffer()?
>
> Since the hypervisor could be actively writing new descriptors while this is
> called, could a weakly-ordered CPU reorder the correlator load before the
> toggle bit load, causing us to recycle the wrong buffer?
>
> [ ... ]
Ah, *
as this toggle/correlator ordering pattern
**
pre-dates the resize work. I’ll treat adding an explicit barrier
in the drain path in v4
*
>> @@ -1491,6 +1718,175 @@ ibmveth_deregister_single_rx_queue(struct ibmveth_adapter *adapter,
>> +static int
>> +ibmveth_resize_rx_queues_incremental(struct ibmveth_adapter *adapter,
>> + int new_count, int rxq_entries)
>> +{
> [ ... ]
>
>> + rc = ibmveth_enable_irq(adapter, i);
>> + if (rc) {
>> + netdev_err(netdev,
>> + "Failed to enable IRQ for queue %d: %d\n",
>> + i, rc);
>> + ibmveth_cleanup_single_rx_interrupt(adapter, i);
>> + ibmveth_deregister_single_rx_queue(adapter, i);
>> + ibmveth_free_single_rx_queue(adapter, i);
>> + goto cleanup_new_queues;
>> + }
>> +
>> + napi_enable(&adapter->napi[i]);
>> + }
>> +
>> + adapter->num_rx_queues = new_count;
> [Severity: High]
> What happens if an interrupt fires on the newly added queue before
> adapter->num_rx_queues is updated?
*agreed, v3 published the new queue count too late.*
> If an interrupt fires between ibmveth_enable_irq() and the update of
> adapter->num_rx_queues, ibmveth_interrupt() might evaluate:
> WARN_ON(qindex < 0 || qindex >= adapter->num_rx_queues)
> to true and return IRQ_NONE without masking the interrupt.
>
> Could this lead to an unhandled interrupt storm?
*
possible
**
I will work on in v4 so that each new queue is published and replenished
**
before PHYP enable and napi_enable(), so ibmveth_interrupt() will not
**
return IRQ_NONE on a queue we have not published yet, and NAPI
does not run on an empty queue.
*
>
> [ ... ]
>
>> + } else {
>> + netdev_dbg(netdev, "Scale-down: removing queues %d-%d\n",
>> + new_count, old_count - 1);
>> +
>> + for (i = new_count; i < old_count; i++)
>> + napi_disable(&adapter->napi[i]);
>> +
>> + for (i = new_count; i < old_count; i++)
>> + ibmveth_drain_rx_queue(adapter, i);
>> +
>> + synchronize_net();
> [Severity: High]
> Can disabling NAPI before disabling the hardware interrupts create an
> interrupt storm window?
*
Agreed — with the handler only masking on successful NAPI
schedule, napi_disable before PHYP mask can storm during drain.
*
>
> If the hypervisor asserts an interrupt during synchronize_net(), the hardware
> IRQ handler runs. Since NAPI is disabled, it returns IRQ_HANDLED without
> masking the interrupt source at the hypervisor, leading immediately to an
> IRQ storm and CPU lockup.
*
That is a load-dependent race (tests can pass and still miss it).
In v4 I will disable_irq + synchronize_irq before
napi_disable/drain on scale-down (and on scale-up fail cleanup).
*
>> +
>> + rc = netif_set_real_num_rx_queues(netdev, new_count);
>> + if (rc) {
>> + netdev_err(netdev, "Failed to set real RX queues to %d: %d\n",
>> + new_count, rc);
>> + for (i = new_count; i < old_count; i++)
>> + napi_enable(&adapter->napi[i]);
>> + return rc;
>> + }
> [Severity: High]
> Is it possible for this error path to permanently stall the RX queues?
>
> It calls napi_enable() but doesn't call ibmveth_replenish_task(). Since the
> queues were completely drained of their buffers by ibmveth_drain_rx_queue(),
> will the hypervisor have any buffers to place incoming packets?
>
> Without buffers, no interrupts will fire for these queues, and NAPI will
> never run again.
*
Good catch — after drain, that set_real_num_rx failure path must
replenish before re-enabling IRQ/NAPI, or those queues stall with
empty pools. Real rollback bug (rare path). Will fix in v4.
*
Thanks,
Mingming
^ permalink raw reply
* Re: [PATCH net-next v3 12/15] ibmveth: Expose per-queue buffer pool details via sysfs
From: mingming cao @ 2026-07-17 1:54 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, bjking1, haren, ricklind, kuba, edumazet, pabeni,
linuxppc-dev, maddy, mpe, davemarq
In-Reply-To: <20260714130142.1841365-1-horms@kernel.org>
On 7/14/26 6:01 AM, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> ibmveth: Expose per-queue buffer pool details via sysfs
>
> This commit adds a read-only sysfs attribute buffer_pools to the VIO device
> to expose per-queue buffer pool details dynamically. It outputs runtime per-queue
> buffer pressure metrics like size, active, and available buffers.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 4e3f49b6346fd..ecc472ee8f71f 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
>> @@ -2896,6 +2896,52 @@ static const struct net_device_ops ibmveth_netdev_ops = {
> [ ... ]
>> +static ssize_t buffer_pools_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct net_device *netdev = dev_get_drvdata(dev);
>> + struct ibmveth_adapter *adapter = netdev_priv(netdev);
>> + int len = 0;
>> + int i, j;
>> +
>> + len += scnprintf(buf + len, PAGE_SIZE - len,
>> + "Queue Pool Size BuffSize Active Available\n");
>> + len += scnprintf(buf + len, PAGE_SIZE - len,
>> + "----- ---- ---- -------- ------ ---------\n");
> [Severity: Medium]
> Does this buffer_pools_show() implementation violate the sysfs "one value
> per file" design rule?
>
> The attribute appears to dump a complex, multi-line ASCII table with headers
> and columns rather than exposing a single value per sysfs file.
*
Agreed this is more of a diagnostic dump than a sysfs one-value
attribute. In v4 I’ll move it to debugfs ?
*
>
> [ ... ]
>> @@ -3056,6 +3102,14 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
>>
>> netdev_dbg(netdev, "registered\n");
>>
>> + rc = sysfs_create_group(&dev->dev.kobj, &ibmveth_attr_group);
>> + if (rc) {
>> + netdev_err(netdev, "failed to create sysfs attributes rc=%d\n", rc);
>> + unregister_netdev(netdev);
>> + free_netdev(netdev);
>> + return rc;
>> + }
> [Severity: High]
> If userspace brings up the interface immediately after register_netdev()
> succeeds, could adapter->work be scheduled before sysfs_create_group() fails?
>
> If this race occurs, unregister_netdev() does not synchronously cancel the
> work.
>
> Could the global workqueue execute the pending adapter->work item on freed
> memory after free_netdev() is called in this ibmveth_probe() error path?
*
Good catch. remove() cancels the work, but this probe error path
did not. Will cancel_work_sync() (and clean pool kobjects) before
free_netdev() in v4.
*
> [Severity: High]
> This is a pre-existing issue in older error paths of ibmveth_probe(), but it
> is duplicated here: does this error path leak the adapter->rx_buff_pool
> kobjects?
>
> Since kobject_put() is not called on the pool kobjects before free_netdev()
> is executed, the kobjects remain attached to the VIO device with dangling
> pointers to the freed adapter memory.
>
> Could subsequent sysfs accesses to those pool kobjects trigger a
> use-after-free?
*
Yes — that path (and some older probe failure paths) skip
kobject_put() on the pool objects. I’ll add a common probe
cleanup helper in v4 so pool kobjects and the reset work are
always torn down before free_netdev().
*
Thanks,
Mingming
^ permalink raw reply
* Re: [PATCH net-next v3 11/15] ibmveth: Add per-queue RX and TX statistics collection and reporting
From: mingming cao @ 2026-07-17 1:39 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, bjking1, haren, ricklind, kuba, edumazet, pabeni,
linuxppc-dev, maddy, mpe, davemarq
In-Reply-To: <20260714125948.1841205-1-horms@kernel.org>
On 7/14/26 5:59 AM, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> ibmveth: Add per-queue RX and TX statistics collection and reporting
>
> This commit introduces per-queue statistics tracking for both receive and
> transmit paths in the ibmveth driver. The counters are aggregated for legacy
> interfaces and exposed individually via ethtool stringsets.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 863e5c68b42c7..4e3f49b6346fd 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... ]
>> @@ -1655,6 +1655,10 @@ static int ibmveth_open(struct net_device *netdev)
>> if (rc)
>> goto out_cleanup_rx_interrupts;
>>
>> + rc = ibmveth_alloc_tx_qstats(adapter);
>> + if (rc)
>> + goto out_free_tx_resources;
>> +
>> netif_tx_start_all_queues(netdev);
>>
>> netdev_dbg(netdev, "open complete\n");
> [ ... ]
>> out_cleanup_rx_interrupts:
>> ibmveth_cleanup_rx_interrupts(adapter);
>> out_free_tx_resources:
>> ibmveth_free_tx_resources(adapter);
> [Severity: High]
> Does the goto out_free_tx_resources on ibmveth_alloc_tx_qstats() failure
> bypass the cleanup for RX interrupts?
>
> Since out_free_tx_resources is physically below out_cleanup_rx_interrupts in
> ibmveth_open(), failing here leaves the IRQs requested and NAPI enabled. If
> an interrupt subsequently fires, could the handler access the freed RX queue
> memory and cause a kernel panic?
>
> [ ... ]
*
Confirmed — that goto skips RX interrupt cleanup. Will fix the
open() unwind order in v4 so TX-stat allocation failure still
disables NAPI and frees IRQs before tearing down queues.
*
>> @@ -1699,6 +1699,7 @@ static int ibmveth_close(struct net_device *netdev)
>> }
>> }
>>
>> + ibmveth_free_tx_qstats(adapter);
>> ibmveth_free_tx_resources(adapter);
>> ibmveth_cleanup_rx_interrupts(adapter);
>> ibmveth_update_rx_no_buffer(adapter);
> [ ... ]
>> ibmveth_free_rx_qstats(adapter);
> [Severity: High]
> If ibmveth_close() is called to bring down the interface, does this
> unconditionally free adapter->tx_qstats and adapter->rx_qstats without a
> grace period?
Confirmed.
>
> Since ibmveth_get_stats64() can be called concurrently under RCU, and
> ibmveth_start_xmit() can run concurrently if the datapath is not completely
> synchronized, could this lead to a use-after-free or NULL pointer dereference
> when these functions access the freed arrays?
>
> [ ... ]
*
Good point. ndo_get_stats64 can run without RTNL. In v4 I’ll give
the qstat arrays adapter lifetime (probe/remove) instead of
open/close, so close cannot free them under concurrent readers.
*
>> @@ -1967,22 +1967,107 @@ static int ibmveth_set_features(struct net_device *dev,
> [ ... ]
>> +static void ibmveth_aggregate_rx_qstats(struct ibmveth_adapter *adapter)
>> +{
>> + u64 total_invalid = 0;
>> + u64 total_large = 0;
>> + int i;
>> +
>> + if (!adapter->rx_qstats)
>> + return;
>> +
>> + for (i = 0; i < adapter->num_rx_queues; i++) {
>> + total_invalid += adapter->rx_qstats[i].invalid_buffers;
>> + total_large += adapter->rx_qstats[i].large_packets;
>> + }
>> +
>> + adapter->rx_invalid_buffer = total_invalid;
>> + adapter->rx_large_packets = total_large;
>> +}
> [Severity: High]
> Does overwriting adapter->rx_invalid_buffer and other persistent global
> counters with the dynamically allocated adapter->rx_qstats values cause a
> permanent loss of interface statistics?
yes
> When the interface is brought down and back up, ibmveth_close() frees the
> queue stats arrays and ibmveth_open() reallocates them as zeroed. Will this
> cause all historical statistics to be wiped out on every interface reset,
> breaking standard persistence behavior?
>
> [ ... ]
*
Agreed this regresses persistence vs pre-MQ.
**
In v4 per-queue qstats will stay for the adapter lifetime, and adapter-level
ethtool strings are summed on read (ibmvnic-style). No dual-updating
globals on the hot path, and no aggregate_* that wipes history on reopen.
*
>> +static void ibmveth_get_strings(struct net_device *dev, u32 stringset, u8 *data)
>> +{
> [ ... ]
>> + for (i = 0; i < adapter->num_rx_queues; i++) {
>> + ethtool_sprintf(&p, "rx%d_packets", i);
>> + ethtool_sprintf(&p, "rx%d_bytes", i);
> [Severity: Low]
> Are these private ethtool stringsets duplicating standard kernel statistics?
>
> According to the subsystem guidelines, per-queue packet and byte statistics
> that have a standard UAPI representation (like struct netdev_queue_stats_rx
> exported via netlink) should not be duplicated as private ethtool strings.
*
Thanks — **
*I can move those to standard*
*
netdev queue stats and drop the ethtool duplicates in a follow-up. For
now I'll keep device-specific counters (interrupts, polls, invalid, …)
as private ethtool strings.
*
*
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.h b/drivers/net/ethernet/ibm/ibmveth.h
>> index f7b20fd01acb5..390c660af979e 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.h
>> +++ b/drivers/net/ethernet/ibm/ibmveth.h
>> @@ -316,9 +316,21 @@ struct ibmveth_rx_queue_stats {
>> u64 no_buffer_drops;
>> };
>>
>> +struct ibmveth_tx_queue_stats {
>> + u64 packets;
>> + u64 bytes;
>> + u64 large_packets;
>> + u64 dropped_packets;
>> + u64 send_failures;
>> + u64 checksum_offload;
>> +};
> [Severity: Medium]
> Will this structure layout prevent cache line false sharing?
>
> The commit message mentions tracking transmit counters per TX queue to avoid
> cache line contention, but struct ibmveth_tx_queue_stats appears to be
> exactly 48 bytes and lacks cacheline alignment annotations like
> ____cacheline_aligned. When allocated as a contiguous array, will multiple
> CPUs processing different queues end up writing to the same hardware
> cacheline?
*Will add cacheline alignment for the per-queue stats structs in v4*
*Thanks again for your valuable feedback!*
*Mingming*
^ permalink raw reply
* Re: [PATCH 6.12.y 0/6] cBPF JIT spray hardening
From: Sasha Levin @ 2026-07-17 1:37 UTC (permalink / raw)
To: stable, Greg Kroah-Hartman
Cc: Sasha Levin, bpf, linux-arm-kernel, loongarch, linuxppc-dev,
linux-riscv, x86, Alexei Starovoitov, Daniel Borkmann,
Dave Hansen, Pawan Gupta
In-Reply-To: <20260715-cbpf-jit-spray-hardening-6-12-y-v1-0-d8585a9aed80@linux.intel.com>
On Wed, Jul 15, 2026 at 02:27:39PM -0700, Pawan Gupta wrote:
> These backports harden BPF JIT against spectre-v2 class of attacks. Without
> a predictor flush, execution of new BPF program may use stale prediction
> left behind by the freed one.
Queued the series for 6.12, thanks.
--
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH net-next v3 10/15] ibmveth: Enable multi-queue RX receive path
From: mingming cao @ 2026-07-17 1:27 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, bjking1, haren, ricklind, kuba, edumazet, pabeni,
linuxppc-dev, maddy, mpe, davemarq
In-Reply-To: <20260714125557.1837451-2-horms@kernel.org>
On 7/14/26 5:55 AM, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> ibmveth: Enable multi-queue RX receive path
>
> This is the first patch that sets multi_queue from H_ILLAN_ATTRIBUTES
> and switches registration, buffer posting, and receive to the MQ
> hcall path. It also raises num_rx_queues and enables per-queue NAPI.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index b3b3886c3eedb..863e5c68b42c7 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... ]
>
>> @@ -826,15 +841,23 @@ static void ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
>> */
>> static void ibmveth_update_rx_no_buffer(struct ibmveth_adapter *adapter)
>> {
>> - __be64 *p = adapter->buffer_list_addr[0] + 4096 - 8;
>> + int i;
>> +
>> + for (i = 0; i < adapter->num_rx_queues; i++) {
>> + __be64 *p = adapter->buffer_list_addr[i] + 4096 - 8;
>> + u64 drops = be64_to_cpup(p);
>>
>> - adapter->rx_no_buffer = be64_to_cpup(p);
>> + if (i == 0)
>> + adapter->rx_no_buffer = drops;
>> + }
>> }
> [Severity: Medium]
> Will this logic silently discard the hardware drop counts for all queues
> other than queue 0?
>
> Since ethtool -S reports the global adapter->rx_no_buffer statistic, shouldn't
> this loop accumulate the drops across all queues instead of only recording
> them when evaluating the first queue?
>
> [ ... ]
*
Good catch. Per-queue drops are already in rx_qstats
once that patch lands; the global rx_no_buffer still mirrors queue 0
only. I'll sum across queues into rx_no_buffer in v4.
*
>> @@ -842,6 +865,8 @@ static void ibmveth_replenish_task(struct ibmveth_adapter *adapter,
>> int queue_index)
>> {
>> + struct ibmveth_rx_q *rxq = &adapter->rx_queue[queue_index];
>> + unsigned long flags;
>> int i;
>>
>> if (queue_index >= adapter->num_rx_queues)
>> @@ -842,6 +865,8 @@ static void ibmveth_replenish_task(struct ibmveth_adapter *adapter,
>>
>> adapter->replenish_task_cycles++;
>>
>> + spin_lock_irqsave(&rxq->replenish_lock, flags);
>> +
> [Severity: Medium]
> With multi-queue RX enabled, can multiple NAPI threads execute this task
> concurrently for different queues?
>
> If so, does this non-atomic increment of the global replenish_task_cycles
> stat risk data corruption, torn reads on 32-bit platforms, or lost updates?
>
> Similarly, in ibmveth_replenish_buffer_pool():
>
> drivers/net/ethernet/ibm/ibmveth.c:ibmveth_replenish_buffer_pool() {
> ...
> buffers_added += filled;
> adapter->replenish_add_buff_success += filled;
> remaining -= filled;
> ...
> }
>
> Even though the caller acquires the per-queue rxq->replenish_lock,
> replenish_add_buff_success is a global adapter statistic. Does this update
> also race with other CPUs holding the locks for their respective queues?
>
> [ ... ]
*Same as on the buffer-submit patch: these are informational counters only.*
*replenish_lock serializes replenishment for a single RX queue, but it
does not *
*protect the adapter-wide counters across multiple queues.
*
*However, these counters are used only for diagnostics and observability, *
*not for correctness, synchronization, or resource management.While
stricter *
*accounting could be achieved with atomic operations, these counters are *
*updated in a hot path and the additional synchronization overhead is
difficult *
*to justify for diagnostic-only statistics, particularly on larger
systems with *
*many RX queues. If more accurate accounting is desired, a per-queue *
*statistics model would likely be a better follow-up approach, *
>> @@ -2268,34 +2299,40 @@ static void ibmveth_rx_csum_helper(struct sk_buff *skb,
>>
>> static int ibmveth_poll(struct napi_struct *napi, int budget)
>> {
> [ ... ]
>> } else {
>> struct sk_buff *skb, *new_skb;
>> - int length = ibmveth_rxq_frame_length(adapter);
>> - int offset = ibmveth_rxq_frame_offset(adapter);
>> - int csum_good = ibmveth_rxq_csum_good(adapter);
>> - int lrg_pkt = ibmveth_rxq_large_packet(adapter);
>> + int length = ibmveth_rxq_frame_length(adapter, queue_index);
>> + int offset = ibmveth_rxq_frame_offset(adapter, queue_index);
>> + int csum_good = ibmveth_rxq_csum_good(adapter, queue_index);
>> + int lrg_pkt = ibmveth_rxq_large_packet(adapter, queue_index);
> [Severity: High]
> This isn't a bug introduced by this patch, but does this code validate the
> length and offset provided by the hardware descriptor before modifying the
> SKB bounds?
>
> Later in ibmveth_poll(), these values are used directly:
>
> drivers/net/ethernet/ibm/ibmveth.c:ibmveth_poll() {
> ...
> skb_reserve(skb, offset);
> skb_put(skb, length);
> ...
> }
>
> If the hypervisor provides an untrusted or artificially large length, could
> the call to skb_put() trigger an skb_over_panic() and crash the guest
> kernel?
>
> [ ... ]
*Agreed, this needs guarding.I was already working on a follow-up
poll-hardening
**patch that adds correlator validation and verifies sufficient
skb_tailroom() before
calling skb_reserve() and skb_put(). I will keep those validations in v4.
If preferred for bisect safety, I can also move the checks earlier in
the series rather
than introducing them only in the final hardening patch.*
>
> Also, I noticed a potential memory leak in the error unwinding path of
> ibmveth_open():
>
> drivers/net/ethernet/ibm/ibmveth.c:ibmveth_open() {
> ...
> if (rc) {
> netdev_err(netdev, "failed to set number of rx queues\n");
> goto out_unregister_queues;
> }
> ...
> out_free_buffer_pools:
> ibmveth_free_buffer_pools(adapter);
> out_unregister_queues:
> ibmveth_dispose_subordinate_irq_mappings(adapter);
> ibmveth_free_all_queues(adapter);
> }
>
> [Severity: Medium]
> This is a pre-existing issue, but if netif_set_real_num_rx_queues() fails,
> the code jumps to the out_unregister_queues label.
>
> Does this bypass out_free_buffer_pools, leaving the RX buffer pools unfreed
> and causing a memory leak when cleaning up after a failure?
*
Yes — same open() unwind issue called out on the open/close
refactor patch. Will fix the label order in v4 so buffer pools
are always freed after unregister.
**
Thanks,
Mingming
*
^ permalink raw reply
* Re: [PATCH net-next v3 09/15] ibmveth: Add queue-aware RX buffer submit helper for MQ
From: mingming cao @ 2026-07-17 1:02 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, bjking1, haren, ricklind, kuba, edumazet, pabeni,
linuxppc-dev, maddy, mpe, davemarq
In-Reply-To: <20260714125019.1833862-2-horms@kernel.org>
On 7/14/26 5:50 AM, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> ibmveth: Add queue-aware RX buffer submit helper for MQ
>
> This patch updates the ibmveth driver's buffer replenishment path to
> be queue-aware for multi-queue receive support. It adds a helper to select
> the correct hypervisor call and handles fallback to single-buffer mode if
> batch operations are unsupported.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index fa2d4777ffc7b..b3b3886c3eedb 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
>> @@ -597,11 +597,73 @@ static inline void ibmveth_flush_buffer(void *addr, unsigned long length)
> [ ... ]
>
>> +static long ibmveth_add_logical_lan_buffers(struct ibmveth_adapter *adapter,
>> + union ibmveth_buf_desc *descs,
>> + int filled,
>> + unsigned long buff_size,
>> + int queue_index)
>> +{
>> + struct vio_dev *vdev = adapter->vdev;
>> + unsigned long rc;
>> +
>> + if (adapter->multi_queue) {
>> + unsigned long buffersznum = (buff_size << 32) | filled;
>> + unsigned long ioba[IBMVETH_MAX_RX_PER_HCALL / 2] = {0};
>> + int i;
>> +
>> + /* Pack descriptor addresses into ioba pairs.
>> + * Each ioba holds two 32-bit addresses packed into 64 bits:
>> + * - Even descriptors (0,2,4...) go in high 32 bits
>> + * - Odd descriptors (1,3,5...) go in low 32 bits
>> + */
>> + for (i = 0; i < filled && i < IBMVETH_MAX_RX_PER_HCALL; i++) {
>> + int pair_idx = i / 2; /* Which pair: 0-5 */
>> + int is_high = (i % 2 == 0); /* High or low 32 bits */
>> +
>> + if (is_high)
>> + ioba[pair_idx] = (unsigned long)descs[i].fields.address << 32;
>> + else
>> + ioba[pair_idx] |= descs[i].fields.address;
>> + }
>> +
>> + rc = h_add_logical_lan_buffers_queue(vdev->unit_address,
>> + adapter->queue_handle[queue_index],
>> + buffersznum,
>> + ioba[0], ioba[1], ioba[2],
>> + ioba[3], ioba[4], ioba[5]);
>> + adapter->hcall_stats.add_bufs_queue++;
> [Severity: Medium]
> Will this counter increment lose updates when multiple NAPI poll routines run
> concurrently on different CPUs?
*
Thanks — these are informational ethtool/debug counters and can
race under concurrent NAPI, as with many driver stats. Datapath
correctness does not depend on them.
*
> Since multi-queue enables concurrent execution, it seems global statistic
> counters like hcall_stats.add_bufs_queue and replenish_add_buff_success might
> need atomic or per-CPU operations to avoid read-modify-write data races.
*
I'm happy to harden selected counters with atomic64_t (or percpu counter)
in a follow-up; I don't think that needs to gate MQ...
*
>> + } else if (filled == 1) {
>> + rc = h_add_logical_lan_buffer(vdev->unit_address,
>> + descs[0].desc);
>> + adapter->hcall_stats.add_buf++;
>> + } else {
>> + rc = h_add_logical_lan_buffers(vdev->unit_address,
>> + descs[0].desc, descs[1].desc,
>> + descs[2].desc, descs[3].desc,
>> + descs[4].desc, descs[5].desc,
>> + descs[6].desc, descs[7].desc);
>> + adapter->hcall_stats.add_bufs++;
>> + }
>> +
>> + return rc;
>> +}
> [ ... ]
>
>> @@ -745,24 +798,19 @@ static void ibmveth_replenish_buffer_pool(struct ibmveth_adapter *adapter,
>> }
>> adapter->replenish_add_buff_failure += filled;
>>
>> - /*
>> - * If multi rx buffers hcall is no longer supported by FW
>> - * e.g. in the case of Live Partition Migration
>> - */
>> - if (batch > 1 && lpar_rc == H_FUNCTION) {
>> - /*
> [ ... ]
>
>> + if (lpar_rc == H_FUNCTION) {
>> + if (adapter->multi_queue) {
>> + netdev_err(adapter->netdev,
>> + "Unexpected H_FUNCTION from multi-queue buffer add (queue=%d, batch=%d)\n",
>> + queue_index, batch);
>> + break;
>> + } else if (batch > 1) {
>> + netdev_warn(adapter->netdev,
>> + "H_FUNCTION from legacy batch buffer add (batch=%d), falling back to single buffer mode\n",
>> + batch);
>> + adapter->rx_buffers_per_hcall = 1;
>> + continue;
> [Severity: High]
> Does this continue statement cause an infinite loop?
*Good catch! yes.*
>
> Looking at ibmveth_replenish_buffer_pool(), the local batch variable is set
> before the while loop:
>
> batch = adapter->rx_buffers_per_hcall;
> while (remaining > 0) {
> ...
>
> If the fallback updates adapter->rx_buffers_per_hcall to 1 and issues a continue,
> the loop will restart but the local batch variable will retain its original
> value.
>
> Would this lead to the loop repeatedly attempting the unsupported batch size
> and failing with H_FUNCTION forever?
You are correct, the MQ replenish rewrite accidentally used continue
instead of break on the non-MQ multi-buffer path (!multi_queue && batch
> 1 && H_FUNCTION). The MQ H_FUNCTION arm already hard-fails and*
breaks — that wasn't the bug. I'll restore the pre-MQ break in v4.
*
>> + }
>> }
>> break;
>> }
>> @@ -784,18 +832,24 @@ static void ibmveth_update_rx_no_buffer(struct ibmveth_adapter *adapter)
>> }
>>
>> /* replenish routine */
>> -static void ibmveth_replenish_task(struct ibmveth_adapter *adapter)
>> +static void ibmveth_replenish_task(struct ibmveth_adapter *adapter,
>> + int queue_index)
>> {
>> int i;
>>
>> + if (queue_index >= adapter->num_rx_queues)
>> + return;
>> +
>> adapter->replenish_task_cycles++;
> [Severity: Medium]
> Similar to the hcall_stats counters above, could this increment suffer from data
> races when multiple queues are processed concurrently across different CPUs?Ah yes
Ah yes, *
Same as the hcall_stats note — racy informational counter; optional
**
atomic/per-queue follow-up, not a functional MQ bug. (since this series
is already large enough)
*
Thanks for your review again!
Mingming
^ permalink raw reply
* Re: [PATCH net-next v3 08/15] ibmveth: Refactor open/close into MQ-ready resource pipeline
From: mingming cao @ 2026-07-17 0:53 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, bjking1, haren, ricklind, kuba, edumazet, pabeni,
linuxppc-dev, maddy, mpe, Dave Marquardt
In-Reply-To: <20260714124736.GK1364329@horms.kernel.org>
On 7/14/26 5:47 AM, Simon Horman wrote:
> On Mon, Jul 06, 2026 at 12:35:56PM -0700, Mingming Cao wrote:
>
> ...
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> ...
*
Hi, Simon
Thanks for the careful read on open/close — these were all fair points.
*
>> /**
>> * ibmveth_register_logical_lan_queue - Register subordinate queue with hypervisor
>> * @adapter: ibmveth adapter structure
>> @@ -1466,208 +1479,108 @@ ibmveth_register_rx_queues(struct ibmveth_adapter *adapter, u64 mac_address)
>> static int ibmveth_open(struct net_device *netdev)
>> {
>> struct ibmveth_adapter *adapter = netdev_priv(netdev);
>> - u64 mac_address;
>> + u64 mac_address = ether_addr_to_u64(netdev->dev_addr);
>> int rxq_entries = 1;
>> - unsigned long lpar_rc;
>> int rc;
>> - union ibmveth_buf_desc rxq_desc;
>> int i;
>> - struct device *dev;
>>
>> netdev_dbg(netdev, "open starting\n");
>>
>> - napi_enable(&adapter->napi[0]);
>> -
>> - for(i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
>> + for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
>> rxq_entries += adapter->rx_buff_pool[0][i].size;
>>
>> - rc = -ENOMEM;
>> - adapter->buffer_list_addr[0] = (void *)get_zeroed_page(GFP_KERNEL);
>> - if (!adapter->buffer_list_addr[0]) {
>> - netdev_err(netdev, "unable to allocate list pages\n");
>> + rc = ibmveth_alloc_rx_qstats(adapter);
>> + if (rc)
>> goto out;
>> - }
>>
>> - adapter->filter_list_addr = (void*) get_zeroed_page(GFP_KERNEL);
>> - if (!adapter->filter_list_addr) {
>> - netdev_err(netdev, "unable to allocate filter pages\n");
>> - goto out_free_buffer_list;
>> - }
>> -
>> - dev = &adapter->vdev->dev;
>> + rc = ibmveth_alloc_filter_list(adapter);
>> + if (rc)
>> + goto out_free_rx_qstats;
>>
>> - adapter->rx_queue[0].queue_len = sizeof(struct ibmveth_rx_q_entry) *
>> - rxq_entries;
>> - adapter->rx_queue[0].queue_addr =
>> - dma_alloc_coherent(dev, adapter->rx_queue[0].queue_len,
>> - &adapter->rx_queue[0].queue_dma, GFP_KERNEL);
>> - if (!adapter->rx_queue[0].queue_addr)
>> + rc = ibmveth_alloc_rx_queues(adapter, rxq_entries);
>> + if (rc)
>> goto out_free_filter_list;
>>
>> - adapter->buffer_list_dma[0] = dma_map_single(dev,
>> - adapter->buffer_list_addr[0],
>> - 4096, DMA_BIDIRECTIONAL);
>> - if (dma_mapping_error(dev, adapter->buffer_list_dma[0])) {
>> - netdev_err(netdev, "unable to map buffer list pages\n");
>> + rc = ibmveth_alloc_buffer_pools(adapter);
>> + if (rc)
>> goto out_free_queue_mem;
>> - }
>>
>> - adapter->filter_list_dma = dma_map_single(dev,
>> - adapter->filter_list_addr, 4096, DMA_BIDIRECTIONAL);
>> - if (dma_mapping_error(dev, adapter->filter_list_dma)) {
>> - netdev_err(netdev, "unable to map filter list pages\n");
>> - goto out_unmap_buffer_list;
>> - }
>> + rc = ibmveth_register_rx_queues(adapter, mac_address);
>> + if (rc)
>> + goto out_free_buffer_pools;
>>
>> - for (i = 0; i < netdev->real_num_tx_queues; i++) {
>> - if (ibmveth_allocate_tx_ltb(adapter, i))
>> - goto out_free_tx_ltb;
>> + rc = netif_set_real_num_rx_queues(netdev, adapter->num_rx_queues);
>> + if (rc) {
>> + netdev_err(netdev, "failed to set number of rx queues\n");
>> + goto out_unregister_queues;
>> }
>>
>> - adapter->rx_queue[0].index = 0;
>> - adapter->rx_queue[0].num_slots = rxq_entries;
>> - adapter->rx_queue[0].toggle = 1;
>> -
>> - mac_address = ether_addr_to_u64(netdev->dev_addr);
>> -
>> - rxq_desc.fields.flags_len = IBMVETH_BUF_VALID |
>> - adapter->rx_queue[0].queue_len;
>> - rxq_desc.fields.address = adapter->rx_queue[0].queue_dma;
>> -
>> - netdev_dbg(netdev, "buffer list @ 0x%p\n", adapter->buffer_list_addr[0]);
>> - netdev_dbg(netdev, "filter list @ 0x%p\n", adapter->filter_list_addr);
>> - netdev_dbg(netdev, "receive q @ 0x%p\n", adapter->rx_queue[0].queue_addr);
>> -
>> - h_vio_signal(adapter->vdev->unit_address, VIO_IRQ_DISABLE);
>> -
>> - lpar_rc = ibmveth_register_logical_lan(adapter, rxq_desc, mac_address);
>> -
>> - if (lpar_rc != H_SUCCESS) {
>> - netdev_err(netdev, "h_register_logical_lan failed with %ld\n",
>> - lpar_rc);
>> - netdev_err(netdev, "buffer TCE:0x%llx filter TCE:0x%llx rxq "
>> - "desc:0x%llx MAC:0x%llx\n",
>> - adapter->buffer_list_dma[0],
>> - adapter->filter_list_dma,
>> - rxq_desc.desc,
>> - mac_address);
>> - rc = -ENONET;
>> - goto out_unmap_filter_list;
>> - }
>> + rc = ibmveth_setup_rx_interrupts(adapter);
>> + if (rc)
>> + goto out_unregister_queues;
>>
>> - for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
>> - if (!adapter->rx_buff_pool[0][i].active)
>> - continue;
>> - if (ibmveth_alloc_buffer_pool(&adapter->rx_buff_pool[0][i])) {
>> - netdev_err(netdev, "unable to alloc pool\n");
>> - adapter->rx_buff_pool[0][i].active = 0;
>> - rc = -ENOMEM;
>> - goto out_free_buffer_pools;
>> + if (adapter->num_rx_queues > 1) {
>> + for (i = 0; i < adapter->num_rx_queues; i++) {
>> + netdev_dbg(netdev, "initial replenish cycle for queue %d\n", i);
>> + ibmveth_replenish_task(adapter, i);
> ibmveth_replenish_task() only has one parameter
> until a later patch in this series.
*
Agreed. Will address this In v4.
the multi-arg replenish call moves to the patch that introduces
the queue-index parameter (or that patch lands
before this open() wiring).
*
>> }
>> + } else {
>> + netdev_dbg(netdev, "initial replenish cycle\n");
>> + ibmveth_interrupt(adapter->queue_irq[0], &adapter->napi[0]);
>> }
>>
>> - netdev_dbg(netdev, "registering irq 0x%x\n", netdev->irq);
>> - rc = request_irq(netdev->irq, ibmveth_interrupt, 0, netdev->name,
>> - netdev);
>> - if (rc != 0) {
>> - netdev_err(netdev, "unable to request irq 0x%x, rc %d\n",
>> - netdev->irq, rc);
>> - do {
>> - lpar_rc = h_free_logical_lan(adapter->vdev->unit_address);
>> - } while (H_IS_LONG_BUSY(lpar_rc) || (lpar_rc == H_BUSY));
>> -
>> - goto out_free_buffer_pools;
>> - }
>> -
>> - rc = -ENOMEM;
>> -
>> - netdev_dbg(netdev, "initial replenish cycle\n");
>> - ibmveth_interrupt(netdev->irq, netdev);
>> + rc = ibmveth_alloc_tx_resources(adapter);
>> + if (rc)
>> + goto out_cleanup_rx_interrupts;
>>
>> netif_tx_start_all_queues(netdev);
>>
>> netdev_dbg(netdev, "open complete\n");
>> -
>> return 0;
>>
>> +out_cleanup_rx_interrupts:
>> + ibmveth_cleanup_rx_interrupts(adapter);
>> +out_free_tx_resources:
>> + ibmveth_free_tx_resources(adapter);
> The out_free_tx_resources label is unused until a later patch of this
> series, so it should be added in that patch rather than this one.
*
Agreed — will introduce out_free_tx_resources only when a caller
needs that jump target.
*
>
> And it's not clear to me that ibmveth_free_tx_resources() should
> be called when jumping to out_cleanup_rx_interrupts as
> in that case ibmveth_alloc_tx_resources() hasn't run successfully.
>
> The AI-generated review on sashiko.dev also highlights the error handling
> here:
>
> "Are the unwind labels ordered incorrectly here?
>
> "If ibmveth_setup_rx_interrupts() fails, it jumps to
> out_unregister_queues, which is placed after out_free_buffer_pools. Does
> this mean we skip freeing the buffer pools and leak memory?
*
Yes — those gotos currently skip free_buffer_pools(). Will fix
unwind ordering in v4 so buffer pools are always released.
*
>
> "Also, if ibmveth_alloc_tx_resources() fails, it internally frees
> partially allocated LTBs. It then jumps to out_cleanup_rx_interrupts and
> falls through to out_free_tx_resources. Because ibmveth_free_tx_ltb()
> calls dma_unmap_single() unconditionally without checking or zeroing
> tx_ltb_dma, will this cause a double free and an invalid DMA unmap?
**
*Correct — on alloc_tx_resources() failure TX is already partially*
*
cleaned inside the helper, so falling through to
free_tx_resources() is wrong and can double-unmap. Will fix the
open() unwind graph in v4.
*
>
> "Finally, in the fall-through path from out_cleanup_rx_interrupts,
> out_free_buffer_pools is executed before out_unregister_queues (which
> calls ibmveth_free_all_queues() to unregister the logical LAN). Does this
> free and unmap the RX buffers while the hypervisor's logical LAN is still
> active, potentially allowing the hypervisor to DMA incoming packets into
> freed memory?
>
**
*Agreed — freeing RX pools before h_free_logical_lan() is unsafe.*
*
v4 will unregister/free the logical LAN before releasing buffer
pools on both open failure and close
**
*Thanks,*
*
Mingming
*
*
>> out_free_buffer_pools:
>> - while (--i >= 0) {
>> - if (adapter->rx_buff_pool[0][i].active)
>> - ibmveth_free_buffer_pool(adapter,
>> - &adapter->rx_buff_pool[0][i]);
>> - }
>> -out_unmap_filter_list:
>> - dma_unmap_single(dev, adapter->filter_list_dma, 4096,
>> - DMA_BIDIRECTIONAL);
>> -
>> -out_free_tx_ltb:
>> - while (--i >= 0) {
>> - ibmveth_free_tx_ltb(adapter, i);
>> - }
>> -
>> -out_unmap_buffer_list:
>> - dma_unmap_single(dev, adapter->buffer_list_dma[0], 4096,
>> - DMA_BIDIRECTIONAL);
>> + ibmveth_free_buffer_pools(adapter);
>> +out_unregister_queues:
>> + ibmveth_dispose_subordinate_irq_mappings(adapter);
>> + ibmveth_free_all_queues(adapter);
>> out_free_queue_mem:
>> - dma_free_coherent(dev, adapter->rx_queue[0].queue_len,
>> - adapter->rx_queue[0].queue_addr,
>> - adapter->rx_queue[0].queue_dma);
>> + ibmveth_cleanup_rx_resources(adapter);
>> out_free_filter_list:
>> - free_page((unsigned long)adapter->filter_list_addr);
>> -out_free_buffer_list:
>> - free_page((unsigned long)adapter->buffer_list_addr[0]);
>> + ibmveth_free_filter_list(adapter);
>> +out_free_rx_qstats:
>> + ibmveth_free_rx_qstats(adapter);
>> out:
>> - napi_disable(&adapter->napi[0]);
>> return rc;
>> }
> ...
^ permalink raw reply
* Re: [PATCH net-next v3 07/15] ibmveth: Add RX queue register/deregister helpers for MQ
From: mingming cao @ 2026-07-17 0:33 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, bjking1, haren, ricklind, kuba, edumazet, pabeni,
linuxppc-dev, maddy, mpe, davemarq
In-Reply-To: <20260714124338.1831986-1-horms@kernel.org>
On 7/14/26 5:43 AM, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> ibmveth: Add RX queue register/deregister helpers for MQ
>
> This patch introduces new helpers to handle RX queue registration and
> deregistration for MQ support. It replaces the single adapter-level
> register/free paradigm with per-queue register calls to the hypervisor,
> fetching queue handles and IRQs. These new paths are only fully exercised
> when multi-queue mode is enabled.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 63b0184c622a4..7fc11a4e1f61d 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... ]
>> @@ -401,6 +401,28 @@ ibmveth_dispose_subordinate_irq_mappings(struct ibmveth_adapter *adapter)
> [ ... ]
>> +static int __maybe_unused
>> +ibmveth_setup_rx_interrupts(struct ibmveth_adapter *adapter)
>> +{
>> + struct net_device *netdev = adapter->netdev;
>> + int i, rc;
>> +
>> + for (i = 0; i < adapter->num_rx_queues; i++) {
>> + if (!adapter->queue_irq[i]) {
>> + netdev_err(netdev, "queue %d has invalid IRQ (0)\n", i);
>> + rc = -EINVAL;
>> + goto err_free_irqs;
>> + }
>> +
>> + rc = request_irq(adapter->queue_irq[i], ibmveth_interrupt,
>> + 0, netdev->name, &adapter->napi[i]);
>> + if (rc) {
>> + netdev_err(netdev,
>> + "request_irq() failed for irq 0x%x queue %d: %d\n",
>> + adapter->queue_irq[i], i, rc);
>> + goto err_free_irqs;
>> + }
>> + }
>> +
>> + for (i = 0; i < adapter->num_rx_queues; i++)
>> + napi_enable(&adapter->napi[i]);
> [Severity: Medium]
> Could this sequence race with packet reception during interface bring-up?
> By calling request_irq() before napi_enable(), the hardware interrupt is
> unmasked while the NAPI state is still NAPI_STATE_DISABLE. If an interrupt
> fires in this window, could ibmveth_interrupt() fail to schedule the NAPI
> poll, potentially leading to dropped events or an interrupt storm?
>
> [ ... ]
**
*On the request_irq vs napi_enable ordering: on open, ibmveth_enable_irq()*
*
(h_vio_signal / H_VIOCTL) runs only after napi_enable(), so PHYP should
not deliver in the request_irq→napi_enable window. That matches what
peers like ibmvnic do (NAPI before firmware unmask). I'll keep the
tested open order (request_irq → napi_enable → PHYP enable) in v4.
For teardown, v4 will follows the hard rule "mask PHYP before
napi_disable": scale-down does that, and close/open-fail cleanup does
too via cleanup_rx_interrupts(). Scale-up still does enable_irq before
napi_enable after publish+replenish — I'm leaving that order for this
v4 and can revisit as a follow-up if needed.
*
>> @@ -1136,6 +1176,293 @@ static void __maybe_unused ibmveth_free_rx_qstats(struct ibmveth_adapter *adapte
> [ ... ]
>> +static int
>> +ibmveth_register_logical_lan_queue(struct ibmveth_adapter *adapter,
>> + union ibmveth_buf_desc rxq_desc,
>> + int queue_index)
>> +{
> [ ... ]
>> + if (lpar_rc == H_FUNCTION) {
>> + if (adapter->multi_queue) {
>> + netdev_info(adapter->netdev,
>> + "Multi queue mode not supported by firmware, falling back to single queue\n");
>> + adapter->multi_queue = 0;
>> + } else {
>> + netdev_err(adapter->netdev,
>> + "Unexpected H_FUNCTION for queue %d registration (MQ mode already disabled)\n",
>> + queue_index);
>> + }
>> + return lpar_rc;
> [Severity: Medium]
> Does returning lpar_rc here cause a regression for the single queue fallback?
> Since this error propagates up to ibmveth_register_rx_queues(), it seems it
> will unconditionally jump to err_unregister and fail the interface open
> rather than allowing it to continue with a single queue.
*
Good catch — clearing multi_queue still returned the
error and failed open, so the "falling back to single queue" log was
misleading. In v4 I'll treat subordinate H_FUNCTION as a hard error:
honest H_FUNCTION-specific netdev_err, then the existing generic logs,
without clearing multi_queue or claiming fallback.
*
>> + }
>> +
>> + if (try_again) {
>> + try_again = 0;
>> + goto retry;
> [Severity: Low]
> Does this retry loop provide any benefit? Unlike the primary queue which
> calls h_free_logical_lan() before retrying to clear stale state, this
> jumps back to retry the exact same hypercall with the exact same arguments,
> which seems like it will immediately fail again.
*
The subordinate try_again path was indeed empty logic. Queue 0 retries
only after h_free_logical_lan() clears stale PHYP state (kexec). The
subqueue path retried the same H_REG_LOGICAL_LAN_QUEUE with the same
args and no free, so the second attempt could not succeed for any reason
we implemented — and H_FUNCTION already returned before the retry.
In v4 I'll drop that blind retry and report the failure once per queue.
*
>> + }
> [ ... ]
>> +static int
>> +ibmveth_register_rx_queues(struct ibmveth_adapter *adapter, u64 mac_address)
>> +{
> [ ... ]
>> + for (i = 1; i < adapter->num_rx_queues; i++) {
>> + rc = ibmveth_register_single_rx_queue(adapter, i, mac_address);
>> + if (rc) {
>> + if (!adapter->queue_handle[i] || !adapter->queue_irq[i]) {
>> + netdev_err(netdev,
>> + "Invalid hypervisor return for queue %d: handle=0x%llx irq=%u\n",
>> + i, adapter->queue_handle[i],
>> + adapter->queue_irq[i]);
>> + }
> [Severity: Low]
> Is this validation check placed correctly? Because it sits inside the
> if (rc) error block, it will log "Invalid hypervisor return" whenever
> registration fails normally (e.g., due to resource constraints). It appears
> the hypervisor is not expected to populate the handle and irq on failure.
**
*Good catch. on ordinary register failure handle/irq are still zero, so*
*
that caller message falsely claimed an invalid hypervisor return and
duplicated ibmveth_register_single_rx_queue() (which already logs the
hcall rc). In v4 I'll drop that extra log at the caller.
Thanks for the reviewing!
Mingming
*
>
>> + goto err_unregister;
>> + }
>> + }
^ permalink raw reply
* Re: [PATCH net-next v3 05/15] ibmveth: Refactor RX interrupt control for MQ RX queues
From: mingming cao @ 2026-07-17 0:17 UTC (permalink / raw)
To: Simon Horman
Cc: netdev, bjking1, haren, ricklind, kuba, edumazet, pabeni,
linuxppc-dev, maddy, mpe, Dave Marquardt
In-Reply-To: <20260714124327.GJ1364329@horms.kernel.org>
On 7/14/26 5:43 AM, Simon Horman wrote:
> On Mon, Jul 06, 2026 at 12:35:53PM -0700, Mingming Cao wrote:
>> Queue 0 and subordinate RX queues use different interrupt control
>> interfaces in PHYP:
>>
>> - queue 0: h_vio_signal() after h_register_logical_lan()
>> - queue N: H_VIOCTL against the queue handle/hwirq mapping
>>
>> The current code is single-queue oriented and cannot safely scale to
>> multiple RX queues in poll completion and open/close IRQ setup.
>>
>> Introduce queue-indexed interrupt helpers:
>>
>> ibmveth_enable_irq(adapter, queue_index)
>> ibmveth_disable_irq(adapter, queue_index)
>> ibmveth_setup_rx_interrupts()
>> ibmveth_cleanup_rx_interrupts()
>>
>> These helpers centralize queue0-vs-subordinate dispatch and make IRQ
>> lifecycle symmetric across open/close and future resize paths.
>>
>> request_irq() is wired with &adapter->napi[i] as dev_id per queue, so
>> interrupt ownership follows the NAPI instance that services that RX
>> queue.
>>
>> Signed-off-by: Mingming Cao <mmc@linux.ibm.com>
>> Reviewed-by: Dave Marquardt <davemarq@linux.ibm.com>
**
*Hi, Simon*
*
Thanks for the review of this series!
*
> Although it is added later in this patch set, I believe linux/irqdomain.h
> needs to be included in this patch so that irq_dispose_mapping is defined.
*
Thanks, will fix in v4 — move linux/irqdomain.h into this patch
where irq_dispose_mapping() is first used
*
>
> Also, I think it would be best to add these helpers in the same patch(es)
> that they are first used. As this will avoid temporal compiler warnings
> about declared but otherwise unused functions. Similarly for patch 13/15.
*
Agreed — will fix in v4. Helpers will be introduced in the same
patch that first uses them (rather than ahead of the open/close
wiring), and I will apply the same rule to patches 13/15.
*
>
> Overall, please make sure that when each patch of the series is applied
> no new warnings or errors are introduced for allmodconfig W=1 builds.
>
> FWIIW, I exercised this using gcc 16.1 from
> https://www.kernel.org/pub/tools/crosstool/
>
> ...
*
Will do — v4 will be checked so each patch applies cleanly under
allmodconfig W=1 (no new warnings/errors), including with a
current crosstool gcc.
Thanks,
Mingming
*
^ permalink raw reply
* [powerpc:merge] BUILD SUCCESS 0cf9f8680543a8e62f5bf7b73f1fba7739bf5e9a
From: kernel test robot @ 2026-07-16 23:11 UTC (permalink / raw)
To: Madhavan Srinivasan; +Cc: linuxppc-dev
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git merge
branch HEAD: 0cf9f8680543a8e62f5bf7b73f1fba7739bf5e9a Automatic merge of 'fixes' into merge (2026-07-15 08:07)
elapsed time: 1927m
configs tested: 105
configs skipped: 16
The following configs have been built successfully.
More configs may be tested in the coming days.
tested configs:
alpha allnoconfig gcc-16.1.0
alpha allyesconfig gcc-16.1.0
alpha defconfig gcc-16.1.0
arc allmodconfig gcc-16.1.0
arc allnoconfig gcc-16.1.0
arc allyesconfig gcc-16.1.0
arc randconfig-001-20260716 gcc-14.3.0
arc randconfig-002-20260716 gcc-16.1.0
arm allnoconfig clang-17
arm allyesconfig gcc-16.1.0
arm randconfig-001-20260716 gcc-8.5.0
arm randconfig-002-20260716 clang-17
arm randconfig-003-20260716 gcc-12.5.0
arm randconfig-004-20260716 clang-24
arm64 allmodconfig clang-23
arm64 allnoconfig gcc-16.1.0
arm64 randconfig-001-20260717 gcc-11.5.0
arm64 randconfig-002-20260717 gcc-8.5.0
arm64 randconfig-003-20260717 gcc-8.5.0
arm64 randconfig-004-20260717 gcc-9.5.0
csky allmodconfig gcc-16.1.0
csky allnoconfig gcc-16.1.0
csky randconfig-001-20260717 gcc-13.4.0
csky randconfig-002-20260717 gcc-15.2.0
hexagon allnoconfig clang-23
hexagon randconfig-001-20260717 clang-24
hexagon randconfig-002-20260717 clang-17
i386 allmodconfig gcc-14
i386 allnoconfig gcc-14
i386 allyesconfig gcc-14
i386 buildonly-randconfig-001-20260716 clang-22
i386 buildonly-randconfig-002-20260716 clang-22
i386 buildonly-randconfig-003-20260716 clang-22
i386 buildonly-randconfig-004-20260716 clang-22
i386 buildonly-randconfig-005-20260716 clang-22
i386 buildonly-randconfig-006-20260716 clang-22
i386 randconfig-016-20260717 gcc-14
loongarch allmodconfig clang-19
loongarch allnoconfig clang-20
loongarch defconfig clang-24
loongarch randconfig-001-20260717 gcc-16.1.0
loongarch randconfig-002-20260717 clang-24
m68k allmodconfig gcc-16.1.0
m68k allnoconfig gcc-16.1.0
m68k allyesconfig gcc-16.1.0
m68k mvme147_defconfig gcc-16.1.0
microblaze allnoconfig gcc-16.1.0
microblaze allyesconfig gcc-16.1.0
mips allmodconfig gcc-16.1.0
mips allnoconfig gcc-16.1.0
mips allyesconfig gcc-16.1.0
nios2 allmodconfig gcc-11.5.0
nios2 allnoconfig gcc-11.5.0
nios2 randconfig-001-20260717 gcc-8.5.0
nios2 randconfig-002-20260717 gcc-8.5.0
openrisc allmodconfig gcc-16.1.0
openrisc allnoconfig gcc-16.1.0
openrisc defconfig gcc-16.1.0
parisc allmodconfig gcc-16.1.0
parisc allnoconfig gcc-16.1.0
parisc allyesconfig gcc-16.1.0
parisc defconfig gcc-16.1.0
parisc randconfig-001 gcc-8.5.0
parisc randconfig-001-20260716 gcc-15.2.0
parisc randconfig-002 gcc-12.5.0
parisc randconfig-002-20260716 gcc-8.5.0
powerpc allmodconfig gcc-16.1.0
powerpc allnoconfig gcc-16.1.0
powerpc randconfig-001 gcc-10.5.0
powerpc randconfig-001-20260716 clang-21
powerpc randconfig-002 gcc-8.5.0
powerpc randconfig-002-20260716 clang-19
powerpc64 randconfig-001 clang-17
powerpc64 randconfig-001-20260716 clang-22
powerpc64 randconfig-002 clang-24
powerpc64 randconfig-002-20260716 clang-24
riscv allmodconfig clang-23
riscv allnoconfig gcc-16.1.0
riscv allyesconfig clang-23
riscv defconfig clang-24
s390 allmodconfig clang-23
s390 allnoconfig clang-23
s390 allyesconfig gcc-16.1.0
s390 defconfig clang-18
s390 randconfig-001-20260717 clang-24
sh allmodconfig gcc-16.1.0
sh allnoconfig gcc-16.1.0
sh allyesconfig gcc-16.1.0
sh defconfig gcc-16.1.0
sh randconfig-001-20260717 gcc-9.5.0
sh randconfig-002-20260717 gcc-16.1.0
sparc allnoconfig gcc-16.1.0
sparc defconfig gcc-16.1.0
sparc randconfig-001-20260717 gcc-8.5.0
sparc randconfig-002-20260717 gcc-14.3.0
sparc64 allmodconfig clang-20
um allmodconfig clang-17
um allnoconfig clang-17
x86_64 allmodconfig clang-22
x86_64 allnoconfig clang-22
x86_64 allyesconfig clang-22
x86_64 rhel-9.4-rust clang-22
xtensa allnoconfig gcc-16.1.0
xtensa allyesconfig gcc-16.1.0
xtensa randconfig-001-20260717 gcc-16.1.0
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH 01/42] of: reserved_mem: Introduce devres-managed initialization functions
From: Rob Herring @ 2026-07-16 20:00 UTC (permalink / raw)
To: Mukesh Ojha
Cc: Bjorn Andersson, Konrad Dybcio, Liviu Dudau, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Joel Stanley, Andrew Jeffery, Paul Cercueil, Anitha Chrisanthus,
Paul Kocialkowski, Linus Walleij, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Alexey Brodkin, Laurent Pinchart, Tomi Valkeinen,
Michal Simek, Daniel Scally, Jacopo Mondi, Mauro Carvalho Chehab,
Eddie James, Tiffany Lin, Andrew-CT Chen, Yunfei Dong,
Minghsiu Tsai, Houlong Wei, Matthias Brugger,
AngeloGioacchino Del Regno, Joseph Liu, Marvin Lin,
Dmitry Osipenko, Krzysztof Kozlowski, Thierry Reding,
Jonathan Hunter, Srinivas Kandagatla, Arnd Bergmann,
Greg Kroah-Hartman, Ge Gordon, Adrian Hunter, Ulf Hansson,
Saravana Kannan, Mathieu Poirier, Jaroslav Kysela, Takashi Iwai,
Shengjiu Wang, Xiubo Li, Liam Girdwood, Mark Brown, Frank Li,
Sascha Hauer, Peter Ujfalusi, Bard Liao, Daniel Baluta,
Orson Zhai, Baolin Wang, Peter Chen, Fugang Duan, Ekansh Gupta,
BST Linux Kernel Upstream Group, Fabio Estevam, Nicolin Chen,
Pengutronix Kernel Team, Kai Vehmanen, Pierre-Louis Bossart,
Vijendar Mukunda, Chunyan Zhang, CIX Linux Kernel Upstream Group,
linux-arm-msm, linux-kernel, dri-devel, linux-aspeed,
linux-arm-kernel, linux-mips, linux-sunxi, linux-media, openbmc,
linux-mediatek, kernel, linux-tegra, linux-mmc, devicetree,
linux-remoteproc, linux-staging, linux-sound, linuxppc-dev, imx,
sound-open-firmware, Konrad Dybcio
In-Reply-To: <20260703193855.110619-2-mukesh.ojha@oss.qualcomm.com>
On Sat, Jul 04, 2026 at 01:08:14AM +0530, Mukesh Ojha wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>
> Introduce devres-based helpers for of_reserved_mem_device_init(_by_idx)
> to help fight dangling references and ever so slightly reduce the
> number of boilerplate deinitialization calls.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
> drivers/of/of_reserved_mem.c | 41 +++++++++++++++++++++++++++++++++
> include/linux/of_reserved_mem.h | 25 ++++++++++++++++++++
> 2 files changed, 66 insertions(+)
>
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 82222bd45ac6..b35541e9fbe8 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -787,6 +787,47 @@ void of_reserved_mem_device_release(struct device *dev)
> }
> EXPORT_SYMBOL_GPL(of_reserved_mem_device_release);
>
> +static void devm_of_reserved_mem_device_release(struct device *dev, void *res)
> +{
> + of_reserved_mem_device_release(*(struct device **)res);
> +}
> +
> +/**
> + * devm_of_reserved_mem_device_init_by_idx() - Resource managed of_reserved_mem_device_init_by_idx()
> + * @dev: Pointer to the device to configure
> + * @np: Pointer to the device node with 'memory-region' property
> + * @idx: Index of selected region
> + *
> + * This is a resource managed version of of_reserved_mem_device_init_by_idx().
> + * The reserved memory region will be released automatically when the device
> + * is unbound.
> + *
> + * Returns: Negative errno on failure or zero on success.
> + */
> +int devm_of_reserved_mem_device_init_by_idx(struct device *dev,
> + struct device_node *np, int idx)
It looks to me like the only 2? users of this could be converted to
devm_of_reserved_mem_device_init() instead. The mmc one obviously can.
The imx one depends if np is dev->of_node. I'm not a fan of APIs with
only a few users.
Rob
^ permalink raw reply
* Re: [PATCH] powerpc/970: fix nap return address corruption on async interrupt exit
From: John Ogness @ 2026-07-16 16:34 UTC (permalink / raw)
To: Mukesh Kumar Chaurasiya (IBM), maddy, mpe, npiggin, chleroy,
sshegde, mchauras, mkchauras, ruanjinjie, thuth, linuxppc-dev,
linux-kernel
Cc: Andreas Schwab
In-Reply-To: <20260707172430.790040-1-mkchauras@gmail.com>
On 2026-07-07, "Mukesh Kumar Chaurasiya (IBM)" <mkchauras@gmail.com> wrote:
> On PowerMac G5 (PPC970, CONFIG_PPC_970_NAP) the system panics shortly
> after boot with symptoms including instruction fetch faults, kernel data
> access faults, and stack corruption, predominantly on SMP and always
> somewhere inside softirq processing.
>
> The PPC970 idle path works by setting _TLF_NAPPING in the current
> thread's local flags before entering the MSR_POW nap loop. When any
> async interrupt wakes the CPU, nap_adjust_return() is expected to detect
> _TLF_NAPPING, clear it, and rewrite regs->NIP to power4_idle_nap_return
> so that the interrupt returns cleanly to the caller of power4_idle_nap()
> rather than back into the nap spin loop.
>
> DEFINE_INTERRUPT_HANDLER_ASYNC generates the following sequence:
>
> irq_enter_rcu();
> ____func(regs); /* timer_interrupt / do_IRQ body */
> irq_exit_rcu(); /* softirqs run here, irqs re-enabled */
> arch_interrupt_async_exit_prepare(regs); /* nap_adjust_return was here */
> irqentry_exit(regs, state);
>
> irq_exit_rcu() calls invoke_softirq() -> do_softirq_own_stack(), which
> runs softirqs with hardware interrupts re-enabled. A nested async
> interrupt can therefore arrive while _TLF_NAPPING is still set. That
> nested interrupt reaches nap_adjust_return() in its own
> arch_interrupt_async_exit_prepare() call, finds _TLF_NAPPING set, and
> redirects *its own* regs->NIP to power4_idle_nap_return. Returning via
> that blr with an unrelated LR on the softirq stack jumps to a garbage
> address, causing the observed crashes.
>
> The comment that previously lived in arch_interrupt_async_exit_prepare()
> even described this exact hazard ("must come before irq_exit()"), but
> nap_adjust_return() was placed after irq_exit_rcu() in the macro, so
> the protection was never effective.
>
> Fix this by calling nap_adjust_return() inside DEFINE_INTERRUPT_HANDLER_ASYNC
> immediately before irq_exit_rcu(), ensuring _TLF_NAPPING is cleared and
> regs->NIP is adjusted before any code that can re-enable interrupts or
> invoke softirqs runs. Move the explanatory comment into
> nap_adjust_return() itself and remove it from arch_interrupt_async_exit_prepare().
>
> Fixes: bee25f97ad24 ("powerpc: Enable GENERIC_ENTRY feature")
> Closes: https://lore.kernel.org/all/87wlvazrdy.fsf@igel.home/
> Reported-by: Andreas Schwab <schwab@linux-m68k.org>
> Signed-off-by: Mukesh Kumar Chaurasiya (IBM) <mkchauras@gmail.com>
Tested-by: John Ogness <john.ogness@linutronix.de>
Would be nice to see this in 7.2-rc4
^ permalink raw reply
* Re: [PATCH v7 16/22] dma-direct: make dma_direct_map_phys() honor DMA_ATTR_CC_SHARED
From: Jason Gunthorpe @ 2026-07-16 14:52 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Aneesh Kumar K.V, Catalin Marinas, iommu, linux-arm-kernel,
linux-kernel, linux-coco, Robin Murphy, Marek Szyprowski,
Will Deacon, Marc Zyngier, Steven Price, Suzuki K Poulose,
Jiri Pirko, Mostafa Saleh, Petr Tesarik, Dan Williams, Xu Yilun,
linuxppc-dev, linux-s390, Madhavan Srinivasan, Michael Ellerman,
Nicholas Piggin, Christophe Leroy (CS GROUP), Alexander Gordeev,
Gerald Schaefer, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Sven Schnelle, x86, Jiri Pirko,
Michael Kelley
In-Reply-To: <9d83ec13-3b1e-4c1b-8bab-16ea057cfba0@amd.com>
On Thu, Jul 16, 2026 at 11:55:20AM +1000, Alexey Kardashevskiy wrote:
> > > For AMD/SME, on host with memory encryption we now end up setting the C
> > > bit for DMA_ATTR_MMIO. This is fine for RAM but not sure whether
> > > some other MMIO bus understands this attribute. Maybe we should stick to
> > > something like __phys_to_dma() for the !CC_SHARED && MMIO path. Or,
> > > since this is not universally defined, just use the old dma_addr = phys
> > > if MMIO and ignore any unlikely DMA offsets.
> > >
> >
> > Considering for AMD/SME system an unencrypted dma addr is one without C
> > bit, will this be good?
>
> Normally both encrypted and unencrypted DMA addresses do not have
> Cbit, with the only exception of "iommu=pt" (which is not the
> default afaik). And in this case, having Cbit in DMA handles only
> makes sense if p2p trafic goes via the root port (== IOMMU in
> passthrough mode, and I am not sure even about if the root port will
> convert this Cbit to T=1 MMIO in this mode) but if it goes via some
> PCI bridge - then Cbit won't mean encryption for sure. But I do not
> know much about p2p (never touched). But in any case
> force_dma_unencrypted() seems to make no sense here.
True p2p through a switch does not call the DMA API at all, it
directly stuffs the physical MMIO BAR address of the target into the
dma_addr_t - which won't have a C bit.
The DMA API is only involved if the p2p will transit through the root
port, so adding the C bit is correct.
AFAICT it is iommu=!pt that doesn't work at all since the AMD IOMMU
driver always does __sme_set() even for MMIO when forming the IOPTE,
which I guess is wrong for MMIO.
Jason
^ permalink raw reply
* Re: [PATCH mm-unstable RFC v4 2/7] arm64/mm: use PTE-level pgprot for huge PFN helpers
From: Will Deacon @ 2026-07-16 14:35 UTC (permalink / raw)
To: Yin Tirui
Cc: Andrew Morton, Matthew Wilcox, David Hildenbrand, Lorenzo Stoakes,
Juergen Gross, Jonathan Cameron, Catalin Marinas, Peter Xu,
Luiz Capitulino, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, H . Peter Anvin, Andy Lutomirski, Peter Zijlstra,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Liam R . Howlett, Zi Yan, Baolin Wang,
Nico Pache, Ryan Roberts, Dev Jain, Barry Song, Lance Yang,
Vlastimil Babka, Mike Rapoport, Suren Baghdasaryan, Michal Hocko,
Anshuman Khandual, Rohan McLure, Kevin Brodsky, Alistair Popple,
Andrew Donnellan, Pasha Tatashin, Baoquan He, Thomas Huth,
Coiby Xu, Dan Williams, Yu-cheng Yu, Lu Baolu, Conor Dooley,
Rik van Riel, wangkefeng.wang, chenjun102, linux-mm, linux-kernel,
x86, linux-arm-kernel, linuxppc-dev, linux-pm
In-Reply-To: <20260526145003.88445-3-yintirui@huawei.com>
On Tue, May 26, 2026 at 10:49:58PM +0800, Yin Tirui wrote:
> Make the arm64 PMD/PUD PFN helpers use PTE-level pgprot_t as the basic
> format.
>
> pfn_pmd() and pfn_pud() now translate PTE-level attributes into block
> entries. pmd_pgprot() and pud_pgprot() translate block descriptor
> attributes back into PTE-level attributes.
>
> Remove mk_pmd_sect_prot() and mk_pud_sect_prot().
>
> Signed-off-by: Yin Tirui <yintirui@huawei.com>
> ---
> arch/arm64/include/asm/pgtable.h | 48 ++++++++++++++++++++++----------
> arch/arm64/mm/mmu.c | 4 +--
> 2 files changed, 36 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 4dfa42b7d053..c3ee12e14f86 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -511,16 +511,6 @@ static inline pmd_t pte_pmd(pte_t pte)
> return __pmd(pte_val(pte));
> }
>
> -static inline pgprot_t mk_pud_sect_prot(pgprot_t prot)
> -{
> - return __pgprot((pgprot_val(prot) & ~PUD_TYPE_MASK) | PUD_TYPE_SECT);
> -}
> -
> -static inline pgprot_t mk_pmd_sect_prot(pgprot_t prot)
> -{
> - return __pgprot((pgprot_val(prot) & ~PMD_TYPE_MASK) | PMD_TYPE_SECT);
> -}
> -
> static inline pte_t pte_swp_mkexclusive(pte_t pte)
> {
> return set_pte_bit(pte, __pgprot(PTE_SWP_EXCLUSIVE));
> @@ -628,7 +618,13 @@ static inline pmd_t pmd_mkspecial(pmd_t pmd)
> #define __pmd_to_phys(pmd) __pte_to_phys(pmd_pte(pmd))
> #define __phys_to_pmd_val(phys) __phys_to_pte_val(phys)
> #define pmd_pfn(pmd) ((__pmd_to_phys(pmd) & PMD_MASK) >> PAGE_SHIFT)
> -#define pfn_pmd(pfn,prot) __pmd(__phys_to_pmd_val((phys_addr_t)(pfn) << PAGE_SHIFT) | pgprot_val(prot))
> +static inline pmd_t pfn_pmd(unsigned long pfn, pgprot_t prot)
> +{
> + pmd_t pmd = __pmd(__phys_to_pmd_val((phys_addr_t)pfn << PAGE_SHIFT) |
> + pgprot_val(prot));
> +
> + return pmd_mkhuge(pmd);
> +}
There's a slight potential difference in behaviour here, in that the old
mk_pXd_sect_prot() helpers would always return a pgprot_t with the valid
bit set, whereas pXd_mkhuge() preserve it from the input prot.
I couldn't find any places that cared about this, though, so:
Acked-by: Will Deacon <will@kernel.org>
Will
^ permalink raw reply
* Re: [PATCH 17/22] mm: pagewalk: drop redundant address check for kernel mm walks
From: Kevin Brodsky @ 2026-07-16 10:24 UTC (permalink / raw)
To: linux-mm
Cc: Andrew Morton, David Hildenbrand, Lorenzo Stoakes,
Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Pasha Tatashin, Russell King,
Catalin Marinas, Will Deacon, Ryan Roberts, linux-arm-kernel,
Huacai Chen, loongarch, James E.J. Bottomley, Helge Deller,
linux-parisc, Madhavan Srinivasan, Michael Ellerman, linuxppc-dev,
Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Gerald Schaefer,
linux-s390, David S. Miller, Andreas Larsson, sparclinux,
Richard Weinberger, Anton Ivanov, Johannes Berg, linux-um,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Ning Sun, x86,
tboot-devel, Ard Biesheuvel, Ilias Apalodimas, linux-efi,
Vishal Moola, Alistair Popple, Matthew Wilcox (Oracle),
linux-kernel, linux-arch
In-Reply-To: <20260714-remove_pgtable_cdtor-v1-17-44be8a7685d7@arm.com>
On 14/07/2026 16:04, Kevin Brodsky wrote:
> Commit 8b1cb4a2e819 ("mm/pagewalk: fix EFI_PGT_DUMP of
> espfix area") made no-VMA page table walks use
> pte_offset_kernel() for any address above TASK_SIZE. This avoided
> pte_offset_map() validation on EFI page table dumps, where efi_mm
> could walk x86 espfix PMDs that intentionally look bad to pmd_bad().
Sashiko rightly doesn't like this patch. Turns out 8b1cb4a2e819's commit
message and the comment it added are narrower than what the problem
really is: on x86, due to things like
/sys/kernel/debug/page_tables/current_kernel, we may be walking kernel
page tables even when walk->mm is a regular user mm. Just checking the
mm is therefore not sufficient.
Will replace this patch with a rewording of the comment to avoid
suggesting the issue is specific to efi_mm.
- Kevin
> efi_mm is now marked with MMF_KERNEL, and no-VMA walks use
> mm_is_kernel() to select pte_offset_kernel() for kernel mm's. As a
> result we no longer need to handle the EFI_PGT_DUMP case explicitly.The
> address check is therefore redundant for EFI_PGT_DUMP and makes the
> condition harder to reason about.
>
> Remove the address check and associated comment; mm_is_kernel()
> should be sufficient on its own.
>
> Tested by booting x86 QEMU with CONFIG_EFI_PGT_DUMP=y and reading
> /sys/kernel/debug/page_tables/efi.
>
> Assisted-by: Codex:GPT-5.5
> Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
> ---
> mm/pagewalk.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/mm/pagewalk.c b/mm/pagewalk.c
> index a3f47d1de3c5..0dd9d4ad9b68 100644
> --- a/mm/pagewalk.c
> +++ b/mm/pagewalk.c
> @@ -68,17 +68,14 @@ static int walk_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
> if (walk->no_vma) {
> /*
> * pte_offset_map() might apply user-specific validation.
> - * Indeed, on x86_64 the pmd entries set up by init_espfix_ap()
> - * fit its pmd_bad() check (_PAGE_NX set and _PAGE_RW clear),
> - * and CONFIG_EFI_PGT_DUMP efi_mm goes so far as to walk them.
> */
> - if (mm_is_kernel(walk->mm) || addr >= TASK_SIZE)
> + if (mm_is_kernel(walk->mm))
> pte = pte_offset_kernel(pmd, addr);
> else
> pte = pte_offset_map(pmd, addr);
> if (pte) {
> err = walk_pte_range_inner(pte, addr, end, walk);
> - if (!mm_is_kernel(walk->mm) && addr < TASK_SIZE)
> + if (!mm_is_kernel(walk->mm))
> pte_unmap(pte);
> }
> } else {
>
^ permalink raw reply
* Re: [PATCH 14/22] mm: set page table page type when allocated
From: Kevin Brodsky @ 2026-07-16 9:49 UTC (permalink / raw)
To: Vishal Moola
Cc: linux-mm, Andrew Morton, David Hildenbrand, Lorenzo Stoakes,
Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Pasha Tatashin, Russell King,
Catalin Marinas, Will Deacon, Ryan Roberts, linux-arm-kernel,
Huacai Chen, loongarch, James E.J. Bottomley, Helge Deller,
linux-parisc, Madhavan Srinivasan, Michael Ellerman, linuxppc-dev,
Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Gerald Schaefer,
linux-s390, David S. Miller, Andreas Larsson, sparclinux,
Richard Weinberger, Anton Ivanov, Johannes Berg, linux-um,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Ning Sun, x86,
tboot-devel, Ard Biesheuvel, Ilias Apalodimas, linux-efi,
Alistair Popple, Matthew Wilcox (Oracle), linux-kernel,
linux-arch
In-Reply-To: <alZSwAM4pwmMOeaD@fedora>
On 14/07/2026 17:16, Vishal Moola wrote:
> On Tue, Jul 14, 2026 at 03:04:03PM +0100, Kevin Brodsky wrote:
>> As a step towards removing pagetable ctor/dtor calls for kernel
>> mm's, move the page type updates from the ctor/dtor to
>> pagetable_{alloc,free}.
> Thanks for doing this patchset.
Thanks for suggesting it :) It's turning out to be quite a bit more
involved than I expected though! (There are multiple cases where we call
the ctor/dtor without using pagetable_{alloc,free}, see my reply to the
cover letter.)
> We should switch over from __folio_set_pgtable() to __SetPagePgtable()
> in this patch. Ptdescs and folios should be treated as distinctly
> independent of each other.
I sure remember the previous discussion, but my reading of your comments
[1] is that we cannot really change this as long as pagetable_alloc()
can be called with order > 0, right? Multiple archs still rely on that.
> Eventually, we want to replace the memcg accounting as well, but
> there's no existing API for that right now. Lets leave that as is for
> now; this patchset is big enough already ;)
Yep, looks like there was an attempt to improve this but it had to be
reverted [2].
- Kevin
[1] https://lore.kernel.org/all/aha321ndGSdReBJR@fedora/
[2]
https://lore.kernel.org/all/20260225002434.2953895-1-axelrasmussen@google.com/
^ permalink raw reply
* Re: [PATCH 04/22] mm: use mm_is_kernel() in generic page table code
From: Kevin Brodsky @ 2026-07-16 9:35 UTC (permalink / raw)
To: linux-mm
Cc: Andrew Morton, David Hildenbrand, Lorenzo Stoakes,
Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Pasha Tatashin, Russell King,
Catalin Marinas, Will Deacon, Ryan Roberts, linux-arm-kernel,
Huacai Chen, loongarch, James E.J. Bottomley, Helge Deller,
linux-parisc, Madhavan Srinivasan, Michael Ellerman, linuxppc-dev,
Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Gerald Schaefer,
linux-s390, David S. Miller, Andreas Larsson, sparclinux,
Richard Weinberger, Anton Ivanov, Johannes Berg, linux-um,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Ning Sun, x86,
tboot-devel, Ard Biesheuvel, Ilias Apalodimas, linux-efi,
Vishal Moola, Alistair Popple, Matthew Wilcox (Oracle),
linux-kernel, linux-arch
In-Reply-To: <20260714-remove_pgtable_cdtor-v1-4-44be8a7685d7@arm.com>
On 14/07/2026 16:03, Kevin Brodsky wrote:
> diff --git a/mm/memory.c b/mm/memory.c
> index d5e87624f692..c0244c0b0756 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3394,13 +3394,13 @@ static int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd,
> spinlock_t *ptl;
>
> if (create) {
> - mapped_pte = pte = (mm == &init_mm) ?
> + mapped_pte = pte = mm_is_kernel(mm) ?
> pte_alloc_kernel_track(pmd, addr, mask) :
> pte_alloc_map_lock(mm, pmd, addr, &ptl);
As noted by Sashiko, this is a little problematic because if
apply_to_page_range() is called on efi_mm, then we will end up with
__pte_alloc_kernel() taking the init_mm lock, since that's hardcoded.
That probably means we should pass the mm to __pte_alloc_kernel().
FWIW calling the kernel alloc function for efi_mm is the correct thing
to do: this ensures we use pmd_populate_kernel() and therefore kernel
page table attributes (just like efi_create_mapping()). This path is
currently never hit because EFI regions whose permissions are modified
are PTE-mapped, but it is clearly more correct to use the kernel path
for such mm's.
- Kevin
^ permalink raw reply
* Re: [PATCH 03/22] mm: introduce MMF_KERNEL flag and set it for init_mm
From: Kevin Brodsky @ 2026-07-16 9:33 UTC (permalink / raw)
To: Lorenzo Stoakes (ARM), Dave Hansen
Cc: linux-mm, Andrew Morton, David Hildenbrand, Liam R. Howlett,
Vlastimil Babka, Mike Rapoport, Suren Baghdasaryan, Michal Hocko,
Pasha Tatashin, Russell King, Catalin Marinas, Will Deacon,
Ryan Roberts, linux-arm-kernel, Huacai Chen, loongarch,
James E.J. Bottomley, Helge Deller, linux-parisc,
Madhavan Srinivasan, Michael Ellerman, linuxppc-dev,
Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Gerald Schaefer,
linux-s390, David S. Miller, Andreas Larsson, sparclinux,
Richard Weinberger, Anton Ivanov, Johannes Berg, linux-um,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Ning Sun, x86,
tboot-devel, Ard Biesheuvel, Ilias Apalodimas, linux-efi,
Vishal Moola, Alistair Popple, Matthew Wilcox (Oracle),
linux-kernel, linux-arch
In-Reply-To: <alZOs30ziqa7l_Lq@lucifer>
On 14/07/2026 17:04, Lorenzo Stoakes (ARM) wrote:
> On Tue, Jul 14, 2026 at 07:47:43AM -0700, Dave Hansen wrote:
>> On 7/14/26 07:03, Kevin Brodsky wrote:
>>> +static inline bool mm_is_kernel(const struct mm_struct *mm)
>>> +{
>>> + return mm && mm_flags_test(MMF_KERNEL, mm);
>>> +}
>> Could we give this some nice comments explaining what a kernel mm is,
>> please? Part of the problem with the init_mm checks is that they're
>> magic and it's not always clear what's special about init_mm.
Agreed, we need to define what this property means exactly, and your
comments on patch 14 show that extending it to efi_mm isn't necessarily
as benign as it appeared to me at first.
My motivation really is about how page tables are handled (in particular
whether ptlocks are used), so as you suggested on patch 19 maybe this
flag should be narrower in scope, and the naming should reflect it.
Possibly MMF_KERNEL_PGTABLES? That's at least one thing that should be
true of all of init_mm, efi_mm and tboot_mm: their page tables use
kernel permissions, not user, and should follow the same rules including
not using ptlocks.
>> Maybe start with this list?
>>
>> 1. There's only one of them.
>> 2. All kernel threads share it. tsk->mm is the same for all kernel
>> threads.
>> 3. It holds the reference copy of the kernel page tables
>> 4. Userspace can't be entered when it is the current mm
>> 5. It has different TLB flushing rules than userspace mms
>>
>> I _think_ those are universal across all architectures.
> Well point 1 isn't true of efimm or tboot_mm so we possibly need a better
> name :)
>
> "Special" is overloaded too much already. I quite like "eternal" so:
>
> static inline bool mm_is_eternal(const struct mm_struct *mm)
> {
> return mm && mm_flags_test(MMF_ETERNAL, mm);
> }
Cheeky but why not! I do wonder whether this conveys the right idea
though. The main point of this flag is that page tables are
allocated/initialised/locked differently; in principle you could have a
temporary non-user mm.
- Kevin
^ permalink raw reply
* Re: [PATCH v2 3/3] powerpc/numa: Support coregroup on PowerNV
From: Ritesh Harjani @ 2026-07-16 6:37 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: linuxppc-dev, Madhavan Srinivasan, Michael Ellerman,
Nicholas Piggin, Christophe Leroy, Naveen N Rao, skiboot, arbab,
mahesh, chleroy, linux-kernel
In-Reply-To: <alYBOcxjv6DqxUyT@linux.ibm.com>
Srikar Dronamraju <srikar@linux.ibm.com> writes:
> * Ritesh Harjani <ritesh.list@gmail.com> [2026-07-10 11:18:12]:
>
>> Srikar Dronamraju <srikar@linux.ibm.com> writes:
>>
>> > Coregroup support on powerpc has so far been limited to PowerVM LPARs.
>> > However, PowerNV can also support coregroups when firmware exposes the
>> > required coregroup information through the associativity hierarchy.
>> >
>> > Detect coregroup support by checking whether primary_domain_index is the
>> > penultimate domain in the CPU node's ibm,associativity property. On
>> > PowerNV, a non-penultimate primary_domain_index indicates that firmware
>> > provides an additional level for coregroup information.
>> >
>> > This keeps the logic compatible with PowerVM systems, where
>> > primary_domain_index is likewise not the penultimate associativity
>> > domain.
>> >
>> > Signed-off-by: Srikar Dronamraju <srikar@linux.ibm.com>
>> > ---
>> > Changelog from v1: https://lkml.kernel.org/r/20260524010017.140408-1-srikar@linux.ibm.com
>> > - Handle comments from Christophe Leroy; make code more flat
>> >
>> > arch/powerpc/mm/numa.c | 56 ++++++++++++++++++++++++++++++++++--------
>> > 1 file changed, 46 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
>> > index 9aa71eb7e96b..e97b624203ea 100644
>> > --- a/arch/powerpc/mm/numa.c
>> > +++ b/arch/powerpc/mm/numa.c
>> > @@ -889,12 +889,32 @@ static int __init numa_setup_drmem_lmb(struct drmem_lmb *lmb,
>> > return 0;
>> > }
>
> Hi Ritesh,
>
> Thanks for the review.
>
>> >
>> > +/*
>> > + * If hierarchy extends beyond primary_domain_index + 1, then next
>> > + * level corresponds to coregroup.
>> > + */
>> > +static int detect_and_enable_coregroup(const __be32 *associativity, int index)
>>
>> Do we care about it's return value? We are not reading that in the
>> patch.
>
> Yes, We do care about the return value. If the index is set to -1, we don't
> retry enabling the coregroup.
>
yes, my bad. Agreed it is being used.
>> this function is mainly only needed in __init, can we mark it so.
>
> Yes, this will be done.
>
>>
>> > +{
>> > + if (!associativity || index == -1)
>> > + goto out;
>> > +
>> > + index = of_read_number(associativity, 1);
>> > +
>> > + if (index > primary_domain_index + 1) {
>> > + coregroup_enabled = 1;
>> > + return index;
>> > + }
>> > +out:
>> > + coregroup_enabled = 0;
>> > + return -1;
>> > +}
>>
>> For PowerVM, we now have two places which will enable coregroup_enabled
>> during mem_topology_setup(). Is there some way we can unify that?
>>
>
> For PowerVM, we have two extra associativity properties
> ibm,ibm,current-associativity-domains and ibm,max-associativity-domains.
> On PowerNV, these two properties are not used/exported.
>
> All we depend is the layout of these properties to determine if coregroup is
> enabled. If the layout tells us that there is place after
> primary_domain_index for coregroup, we assume coregroup is enabled.
>
> So in this patch, we hook at the place we look at each of the CPU
> associativity. This should work for both PowerVM and PowerNV.
>
> So, I can think of two options.
> 1. Remove the previous logic of depending on PowerVM specific code.
> 2. Allow the previous logic to be around. Since its not going to hurt
> functionally or performance wise.
>
>> This also means we enable coregroup in case of PowerVM with SPLPAR when
>> per-cpu VPHN associativity index > primary_domain_index+1. But this
>> isn't reflected in your commit msg. The commit msg only says this
>> affects PowerNV.
>
> I don't think, I said this affects PowerNV only. But I still don't think
> the logic would change. The logic to enable coregroup remains the same.
> Just that we may now be depending on the 1st CPU associativity instead of
> the PowerVM specific properties.
>
So let's just add this info in the commit msg please. Because it was not
clear otherwise.
-ritesh
^ permalink raw reply
* Re: powerpc/pseries: EEH detected with xhci_hcd
From: Ritesh Harjani @ 2026-07-16 4:51 UTC (permalink / raw)
To: Sourabh Jain, Mahesh J Salgaonkar, linuxppc-dev,
Mahesh J Salgaonkar, Venkat Rao Bagalkote, Narayana Murty N,
Nicholas Piggin, Michael Ellerman, shivang upadhyay, sbhat
In-Reply-To: <29f33de0-0861-459f-b6a0-440354fac785@linux.ibm.com>
Sourabh Jain <sourabhjain@linux.ibm.com> writes:
> This issue only occurs with the Radix MMU.
> Booting the same kernel with disable_radix=1 does not reproduce the issue.
>
> The system also has pmem.
@Sourabh,
Few requests -
1. Could you also attach the boot logs from the same system with the
same kernel but with disable_radix=1?
2. I can't really tell on how is MMU related to this issue? But I think
if you could share the working dmesg logs from Hash that might give
some more info. I am suspecting what if the firmware negotiates
different page sizes even for TCEs for Radix v/s Hash.
3. Would it be possible for you to try git bisect and identify the
culprit commit?
Btw, to clarify I think there are two issues here:
1. EEH triggered for the USB adapter - I think you meant that with Hash
you don't even see EEH error being triggered at all.
2. Second problem is - EEH recovery causing this WARNING.
[ 17.375534] [ T118] WARNING: arch/powerpc/kernel/eeh_pe.c:439 at eeh_pe_tree_remove+0xf8/0x2f0, CPU#10: eehd/118
<...>
[ 17.375599] [ T118] CPU: 10 UID: 0 PID: 118 Comm: eehd Tainted: G E 7.2.0-rc3upstream+ #3 PREEMPT(lazy) f29942c1738362234fefd88de0d1ed711f1a557e
[ 17.375603] [ T118] Tainted: [E]=UNSIGNED_MODULE
[ 17.375604] [ T118] Hardware name: IBM,9080-HEX Power11 (architected) 0x820200 0xf000007 of:IBM,FW1120.00 (NH1120_135) hv:phyp pSeries
<...>
[ 17.375650] [ T118] NIP [c00000000004a048] eeh_pe_tree_remove+0xf8/0x2f0
[ 17.375654] [ T118] LR [c00000000004ba6c] eeh_pe_detach_dev+0x5c/0xa0
[ 17.375658] [ T118] Call Trace:
[ 17.375659] [ T118] [c00000000a357c90] [c00000000dd32800] 0xc00000000dd32800 (unreliable)
[ 17.375668] [ T118] [c00000000a357d30] [c00000000004ba6c] eeh_pe_detach_dev+0x5c/0xa0
[ 17.375672] [ T118] [c00000000a357d60] [c0000000000496ec] eeh_pe_traverse+0x6c/0xf0
[ 17.375676] [ T118] [c00000000a357da0] [c00000000004e100] eeh_reset_device+0x1f8/0x298
[ 17.375680] [ T118] [c00000000a357e50] [c00000000004ce5c] eeh_handle_normal_event+0x61c/0xa60
[ 17.375685] [ T118] [c00000000a357f30] [c00000000004e298] eeh_event_handler+0xf8/0x190
[ 17.375689] [ T118] [c00000000a357f90] [c00000000025dab4] kthread+0x194/0x1b0
-ritesh
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox