Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The MQ teardown design is only safe if CMO and MQ firmware
never run on the same partition, and the code never checks this.
--
ibmveth: Enable multi-queue RX receive path
This commit turns on multi-queue RX when firmware advertises
IBMVETH_ILLAN_RX_MULTI_QUEUE_SUPPORT in H_ILLAN_ATTRIBUTES. Each
subordinate queue 1..N gets its own IRQ, NAPI instance and buffer pools.
It also changes teardown: in multi-queue mode, a hard H_FREE_LOGICAL_LAN
failure now leaves the LAN memory mapped, and open() and remove() retry
the free later.
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c
> b/drivers/net/ethernet/ibm/ibmveth.c
> index b12bfa1e485eb..a0c3c06d2b9d9 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
[ ... ]
> @@ -1630,17 +1864,25 @@ static long ibmveth_register_logical_lan(struct
> ibmveth_adapter *adapter,
[ ... ]
> + * Retries only H_BUSY and H_IS_LONG_BUSY. Other failures are logged.
> + * In multi-queue mode a failure other than H_PARAMETER (already free)
> + * is fail-before-start: PHYP may still DMA into the LAN memory, so
> + * return -EIO with the slots still registered. The caller skips the
> + * unmap, and the next open() or remove() retries. Single-queue mode
> + * keeps what close() always did, log and let the caller unmap; one
> + * reason it cannot simply keep the memory mapped is that on a CMO
> + * partition vio_cmo_bus_remove() BUG()s on an outstanding entitlement.
[ ... ]
> @@ -1654,29 +1896,45 @@ static void ibmveth_free_all_queues(struct
> ibmveth_adapter *adapter)
> if (lpar_rc != H_SUCCESS) {
> netdev_err(adapter->netdev,
> "h_free_logical_lan failed: %ld\n", lpar_rc);
> + if (lpar_rc != H_PARAMETER && adapter->multi_queue)
> + return -EIO;
> }
[ ... ]
> @@ -3131,16 +3527,30 @@ static int ibmveth_probe(struct vio_dev *dev, const
> struct vio_device_id *id)
> netdev->features |= NETIF_F_FRAGLIST;
> }
>
> - /* Initialize queue count - always 1 for now */
> - adapter->multi_queue = 0;
> - adapter->num_rx_queues = IBMVETH_DEFAULT_RX_QUEUES;
> + if (ret == H_SUCCESS &&
> + (ret_attr & IBMVETH_ILLAN_RX_MULTI_QUEUE_SUPPORT)) {
> + adapter->multi_queue = true;
[Severity: Medium]
The new -EIO path in ibmveth_free_all_queues() is only safe if CMO and
multi-queue are never active on the same partition. The kernel-doc above
uses the CMO BUG() to explain why single-queue must still unmap, and the
commit message says:
CMO (Power9 and earlier) and MQ firmware (Power11+) do not coexist
Does anything in the driver enforce that? Here multi_queue is set from
the IBMVETH_ILLAN_RX_MULTI_QUEUE_SUPPORT bit alone. Yet ibmveth_probe()
checks FW_FEATURE_CMO a few lines further down to pick pool_count_cmo:
if (firmware_has_feature(FW_FEATURE_CMO))
memcpy(pool_count, pool_count_cmo, sizeof(pool_count));
Suppose firmware does report the MQ bit on a CMO partition and
H_FREE_LOGICAL_LAN hard-fails. ibmveth_remove() then returns with the
DMA mappings still live:
if (ibmveth_lan_resources_live(adapter)) {
if (ibmveth_free_all_queues(adapter))
dev_err(&dev->dev,
"h_free_logical_lan still failing at remove;
leaking LAN buffers\n");
...
}
Would the bus remove path then hit this BUG()?
arch/powerpc/platforms/pseries/vio.c:vio_cmo_bus_remove() {
...
if (viodev->cmo.allocated) {
dev_err(&viodev->dev, "%s: device had %lu bytes of IO "
"allocated after remove operation.\n",
__func__, viodev->cmo.allocated);
BUG();
}
...
}
The entitlement is also mismatched. vio_cmo_bus_probe() computes the
CMO desired entitlement before the driver probes, so it covers one
queue. Probe does not refresh it, but MQ maps up to 8 times the RX
buffers.
Could multi_queue be gated on !firmware_has_feature(FW_FEATURE_CMO)
here, so the driver checks this instead of assuming it? At the end of
the series, probe still enables MQ the same way, and remove() still has
the "leaking LAN buffers" path.
> + ibmveth_publish_num_rx_queues(adapter,
> + min(num_online_cpus(),
> + IBMVETH_DEFAULT_QUEUES));
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109235556.60604-1-mmc%40linux.ibm.com
