Re: [PATCH v10 8/9] scsi: ibmvfc: register and use asynchronous sub CRQ for events

[Tyrel Datwyler <tyreld@linux.ibm.com>]

On 9/10/26 10:48 PM, Tyrel Datwyler wrote:
> From: Dave Marquardt <davemarq@linux.ibm.com>
> 
> Wire the async sub-CRQ into the sub-CRQ lifecycle so it is allocated,
> registered, and freed alongside the SCSI channel queues.
> 
> In ibmvfc_init_sub_crqs(), allocate the async sub-CRQ queue buffer via
> ibmvfc_alloc_queue() before allocating the SCSI channels. Register it
> with the VIOS by calling ibmvfc_register_channel() with index -1 (the
> negative-index sentinel introduced in the previous commit). Either
> failure disables multi-queue and aborts init.
> 
> In ibmvfc_release_sub_crqs(), ibmvfc_reenable_crq_queue(), and
> ibmvfc_reset_crq(), deregister and re-register the async sub-CRQ
> alongside the SCSI channel queues.
> 
> In ibmvfc_channel_setup_done(), capture the async sub-CRQ handle
> returned by the VIOS in the channel setup response and store it in
> vhost->async_sub_crq.vios_cookie.
> 
> In ibmvfc_set_login_info(), advertise IBMVFC_USE_ASYNC_SUBQ,
> IBMVFC_CAN_HANDLE_FPIN, and IBMVFC_YES_SCSI capabilities whenever
> multi-queue channels are enabled. IBMVFC_YES_SCSI was previously only
> set for NVMe-enabled configurations; move it to the common multi-queue
> path so it is always advertised when channels are in use.
> 
> Fix a variable-shadowing bug in ibmvfc_register_channel() where the
> irq_failed cleanup loop reused rc for the H_FREE_SUB_CRQ hcall result,
> clobbering the error code returned to the caller. Introduce hcall_rc
> for the cleanup loop instead.

I think this should be its own patch.

> 
> Guard the memset() in ibmvfc_deregister_channel() behind a check of
> scrq->msgs.handle to prevent a NULL dereference when the function is
> called on a queue that was never allocated, such as async_sub_crq when
> multi-queue is disabled or initialization failed before
> ibmvfc_alloc_queue() was reached.

Same with this chunk.

-Tyrel

> 
> Signed-off-by: Dave Marquardt <davemarq@linux.ibm.com>
> Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
> ---
>  drivers/scsi/ibmvscsi/ibmvfc-core.c | 51 +++++++++++++++++++++++++----
>  1 file changed, 44 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index c78e221f182a..70a3046135c7 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -192,6 +192,8 @@ static void ibmvfc_tgt_move_login(struct ibmvfc_target *);
>  
>  static void ibmvfc_dereg_sub_crqs(struct ibmvfc_host *, struct ibmvfc_channels *);
>  static void ibmvfc_reg_sub_crqs(struct ibmvfc_host *, struct ibmvfc_channels *);
> +static void ibmvfc_deregister_channel(struct ibmvfc_host *, struct ibmvfc_channels *, int);
> +static int ibmvfc_register_channel(struct ibmvfc_host *, struct ibmvfc_channels *, int);
>  
>  static const char *unknown_error = "unknown error";
>  
> @@ -964,6 +966,7 @@ static int ibmvfc_reenable_crq_queue(struct ibmvfc_host *vhost)
>  	struct vio_dev *vdev = to_vio_dev(vhost->dev);
>  	unsigned long flags;
>  
> +	ibmvfc_deregister_channel(vhost, &vhost->scsi_scrqs, -1);
>  	ibmvfc_dereg_sub_crqs(vhost, &vhost->scsi_scrqs);
>  	ibmvfc_dereg_sub_crqs(vhost, &vhost->nvme_scrqs);
>  
> @@ -986,6 +989,7 @@ static int ibmvfc_reenable_crq_queue(struct ibmvfc_host *vhost)
>  	spin_unlock(vhost->crq.q_lock);
>  	spin_unlock_irqrestore(&vhost->host->host_lock, flags);
>  
> +	ibmvfc_register_channel(vhost, &vhost->scsi_scrqs, -1);
>  	ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
>  	ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs);
>  
> @@ -1006,6 +1010,7 @@ static int ibmvfc_reset_crq(struct ibmvfc_host *vhost)
>  	struct vio_dev *vdev = to_vio_dev(vhost->dev);
>  	struct ibmvfc_queue *crq = &vhost->crq;
>  
> +	ibmvfc_deregister_channel(vhost, &vhost->scsi_scrqs, -1);
>  	ibmvfc_dereg_sub_crqs(vhost, &vhost->scsi_scrqs);
>  	ibmvfc_dereg_sub_crqs(vhost, &vhost->nvme_scrqs);
>  
> @@ -1042,6 +1047,7 @@ static int ibmvfc_reset_crq(struct ibmvfc_host *vhost)
>  	spin_unlock(vhost->crq.q_lock);
>  	spin_unlock_irqrestore(&vhost->host->host_lock, flags);
>  
> +	ibmvfc_register_channel(vhost, &vhost->scsi_scrqs, -1);
>  	ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
>  	ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs);
>  
> @@ -1584,9 +1590,11 @@ static void ibmvfc_set_login_info(struct ibmvfc_host *vhost)
>  
>  	if (vhost->mq_enabled || vhost->using_channels) {
>  		login_info->capabilities |= cpu_to_be64(IBMVFC_CAN_USE_CHANNELS);
> +		login_info->capabilities |= cpu_to_be64(IBMVFC_USE_ASYNC_SUBQ);
> +		login_info->capabilities |= cpu_to_be64(IBMVFC_CAN_HANDLE_FPIN);
> +		login_info->capabilities |= cpu_to_be64(IBMVFC_YES_SCSI);
>  		if (vhost->nvme_enabled) {
>  			login_info->capabilities |= cpu_to_be64(IBMVFC_YES_NVMEOF);
> -			login_info->capabilities |= cpu_to_be64(IBMVFC_YES_SCSI);
>  			login_info->capabilities |= cpu_to_be64(IBMVFC_CAN_USE_WWPN_ALL);
>  		}
>  	}
> @@ -5811,6 +5819,7 @@ static void ibmvfc_channel_setup_done(struct ibmvfc_event *evt)
>  		for (i = 0; i < nvme->active_queues; i++)
>  			nvme->scrqs[i].vios_cookie =
>  				be64_to_cpu(setup->channel_handles[scsi->active_queues + i]);
> +		vhost->async_sub_crq.vios_cookie = be64_to_cpu(setup->async_sub_crq_handle);
>  
>  		ibmvfc_dbg(vhost, "Using %u SCSI channels\n",
>  			   scsi->active_queues);
> @@ -5870,6 +5879,7 @@ static void ibmvfc_channel_setup(struct ibmvfc_host *vhost)
>  		for (i = 0; i < nvme_channels; i++)
>  			setup_buf->channel_handles[scsi_channels + i] =
>  				cpu_to_be64(nvme->scrqs[i].cookie);
> +		setup_buf->async_sub_crq_handle = cpu_to_be64(vhost->async_sub_crq.cookie);
>  	}
>  
>  	ibmvfc_init_event(evt, ibmvfc_channel_setup_done, IBMVFC_MAD_FORMAT);
> @@ -6834,6 +6844,7 @@ static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
>  	bool is_async = index < 0;
>  	struct ibmvfc_queue *scrq = !is_async ? &channels->scrqs[index] : &vhost->async_sub_crq;
>  	int rc = -ENOMEM;
> +	int hcall_rc;
>  
>  	ENTER;
>  
> @@ -6902,8 +6913,8 @@ static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
>  
>  irq_failed:
>  	do {
> -		rc = plpar_hcall_norets(H_FREE_SUB_CRQ, vdev->unit_address, scrq->cookie);
> -	} while (rc == H_BUSY || H_IS_LONG_BUSY(rc));
> +		hcall_rc = plpar_hcall_norets(H_FREE_SUB_CRQ, vdev->unit_address, scrq->cookie);
> +	} while (hcall_rc == H_BUSY || H_IS_LONG_BUSY(hcall_rc));
>  reg_failed:
>  	LEAVE;
>  	return rc;
> @@ -6953,8 +6964,10 @@ static void ibmvfc_deregister_channel(struct ibmvfc_host *vhost,
>  	}
>  
>  	/* Clean out the queue */
> -	memset(scrq->msgs.crq, 0, PAGE_SIZE);
> -	scrq->cur = 0;
> +	if (scrq->msgs.handle) {
> +		memset(scrq->msgs.crq, 0, PAGE_SIZE);
> +		scrq->cur = 0;
> +	}
>  
>  	LEAVE;
>  }
> @@ -6971,7 +6984,9 @@ static void ibmvfc_reg_sub_crqs(struct ibmvfc_host *vhost,
>  	for (i = 0; i < channels->max_queues; i++) {
>  		if (ibmvfc_register_channel(vhost, channels, i)) {
>  			for (j = i; j > 0; j--)
> -				ibmvfc_deregister_channel(vhost, channels, j - 1);
> +				ibmvfc_deregister_channel(
> +					vhost, channels, j - 1);
> +
>  			vhost->do_enquiry = 0;
>  			return;
>  		}
> @@ -7026,16 +7041,26 @@ static int ibmvfc_alloc_channels(struct ibmvfc_host *vhost,
>  
>  static void ibmvfc_init_sub_crqs(struct ibmvfc_host *vhost)
>  {
> +	int rc = 0;
> +
>  	ENTER;
>  	if (!vhost->mq_enabled)
>  		return;
>  
> -	if (ibmvfc_alloc_channels(vhost, &vhost->scsi_scrqs)) {
> +	rc = ibmvfc_alloc_queue(vhost, &vhost->async_sub_crq, IBMVFC_SUB_CRQ_FMT);
> +	if (rc) {
>  		vhost->do_enquiry = 0;
>  		vhost->mq_enabled = 0;
>  		return;
>  	}
>  
> +	/* register async_sub_crq channel */
> +	if (ibmvfc_register_channel(vhost, &vhost->scsi_scrqs, -1))
> +		goto free_async_sub_crq;
> +
> +	if (ibmvfc_alloc_channels(vhost, &vhost->scsi_scrqs))
> +		goto deregister_async_sub_crq;
> +
>  	ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
>  
>  	if (vhost->nvme_enabled) {
> @@ -7046,6 +7071,15 @@ static void ibmvfc_init_sub_crqs(struct ibmvfc_host *vhost)
>  	}
>  
>  	LEAVE;
> +	return;
> +
> + deregister_async_sub_crq:
> +	ibmvfc_deregister_channel(vhost, &vhost->scsi_scrqs, -1);
> +free_async_sub_crq:
> +	ibmvfc_free_queue(vhost, &vhost->async_sub_crq);
> +	vhost->do_enquiry = 0;
> +	vhost->mq_enabled = 0;
> +	return;
>  }
>  
>  static void ibmvfc_release_channels(struct ibmvfc_host *vhost,
> @@ -7069,6 +7103,9 @@ static void ibmvfc_release_channels(struct ibmvfc_host *vhost,
>  static void ibmvfc_release_sub_crqs(struct ibmvfc_host *vhost)
>  {
>  	ENTER;
> +	ibmvfc_deregister_channel(vhost, &vhost->scsi_scrqs, -1);
> +	ibmvfc_free_queue(vhost, &vhost->async_sub_crq);
> +
>  	if (!vhost->scsi_scrqs.scrqs)
>  		return;
>