Re: [PATCH v5 4/7] scsi: ibmvfc: allocate asynchronous sub-queue

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing Setup and Registration (Dead Code / Incomplete Implementation)
- [High] Erroneous Per-Protocol Allocation of Asynchronous Sub-Queue
- [Medium] Inefficient Resource Allocation for Async Sub-Queue
--

--- Patch [4]: [PATCH v5 4/7] scsi: ibmvfc: allocate asynchronous sub-queue ---
commit 7efcd61f3c0261f3a8cbf890ed40ef64412880a3
Author: Dave Marquardt <[email protected]>

scsi: ibmvfc: allocate asynchronous sub-queue

This patch allocates and initializes the asynchronous sub-queue required for
receiving full and extended FPIN events from VIOS.

> Update ibmvfc_channel_setup() to pass async_subq_handle to VIOS during
> channel setup, and ibmvfc_channel_setup_done() to store the VIOS cookie
> for the async sub-queue.

The commit message states that ibmvfc_channel_setup() and
ibmvfc_channel_setup_done() are updated, but those modifications are missing
from the actual patch code.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -6796,6 +6796,17 @@ static int ibmvfc_alloc_channels(struct ibmvfc_host *vhost,
>  	if (!channels->scrqs)
>  		return -ENOMEM;
>  
> +	channels->async_scrq = kzalloc_obj(*channels->async_scrq, GFP_KERNEL);

[Severity: High]
Is async_scrq placed in the correct structure?

It appears async_scrq is added to struct ibmvfc_channels, which exists
per protocol (vhost->scsi_scrqs and vhost->nvme_scrqs). Because this allocation
path is executed once per protocol, does this result in allocating an async
queue for each protocol instead of a single async queue per host, which the
VIOS channel setup expects?

> +	if (!channels->async_scrq) {
> +		rc = -ENOMEM;
> +		goto free_scrqs;
> +	}
> +
> +	rc = ibmvfc_alloc_queue(vhost, channels->async_scrq,
> +				IBMVFC_SUB_CRQ_FMT);

[Severity: High]
Are we missing the hypervisor registration and setup steps for this queue?

Without the updates to ibmvfc_channel_setup() and ibmvfc_channel_setup_done()
mentioned in the commit message, the async queue is allocated but never
registered with the hypervisor via h_reg_sub_crq() or communicated to the VIOS,
leaving it non-functional.

[Severity: Medium]
Does this queue need a full event pool allocation?

Calling ibmvfc_alloc_queue() with IBMVFC_SUB_CRQ_FMT sets queue->total_depth
to a non-zero value, which allocates a large command event pool and associated
DMA memory. Since this async queue is intended solely for receiving unsolicited
events, could we avoid allocating an unused outbound event pool?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4