Re: [PATCH net-next v8 10/15] ibmveth: Enable multi-queue RX receive path

[mingming cao <mmc@linux.ibm.com>]

On 11/12/26 3:57 PM, netdev-bot+sashiko@kernel.org wrote:
> 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));
  The split comes from the platform support matrix, not from that bit in
the driver. CMO / Active Memory Sharing is supported on Power9 and
earlier, and PHYP advertises ILLAN multi-queue on Power11 and later. So
a partition can't see both FW_FEATURE_CMO and the multi-queue bit, and
the scenario in the example can't happen on supported firmware.

> 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));
> [ ... ]

I don't think this needs to block the series, since the combination
isn't possible. If there's a respin I'll add an explicit
!firmware_has_feature(FW_FEATURE_CMO) gate in probe; otherwise I'll
send it as a follow-up hardening patch.

Thanks,
Mingming