Re: [PATCH v16 11/12] scsi: ibmvfc: register and use asynchronous sub CRQ for events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: ibmvfc: broken error unwind and MQ disable in ibmvfc_reg_sub_crqs()
--

commit 0ac96c5239b66a2d4b7e04178af6f76c367e358d
Author: Dave Marquardt <davemarq@linux.ibm.com>

scsi: ibmvfc: register and use asynchronous sub CRQ for events

This commit wires the asynchronous sub-CRQ into the ibmvfc sub-CRQ lifecycle
so that it is allocated, registered, and released alongside the SCSI and NVMe
channel queues. It also updates channel setup and login capability flags to
advertise async sub-CRQ support.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index af11726b76beb..4d4ebe5f6f123 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -988,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_reg_sub_crqs(vhost, &vhost->async_scrqs);
>  	ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
>  	ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs);
>  
[ ... ]
> @@ -1044,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_reg_sub_crqs(vhost, &vhost->async_scrqs);
>  	ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
>  	ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs);
>  
[ ... ]
> @@ -6976,25 +6994,45 @@ VISIBLE_IF_KUNIT void ibmvfc_deregister_channel(struct ibmvfc_host *vhost,
>  }
>  EXPORT_SYMBOL_IF_KUNIT(ibmvfc_deregister_channel);
>  
> -static void ibmvfc_reg_sub_crqs(struct ibmvfc_host *vhost,
[ ... ]
> +static int ibmvfc_reg_sub_crqs(struct ibmvfc_host *vhost,
>  				struct ibmvfc_channels *channels)
>  {
> +	unsigned long flags;
>  	int i, j;
>  
>  	ENTER;
>  	if (!vhost->mq_enabled || !channels->scrqs)
> -		return;
> +		return 0;
>  
>  	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);
> +			spin_lock_irqsave(&vhost->host->host_lock, flags);
>  			vhost->do_enquiry = 0;
> -			return;
> +			vhost->mq_enabled = 0;
> +			spin_unlock_irqrestore(&vhost->host->host_lock, flags);
> +			return -1;
>  		}
>  	}
>  
>  	LEAVE;
> +	return 0;
>  }
[ ... ]
> @@ -7083,9 +7102,74 @@ static void ibmvfc_release_channels(struct ibmvfc_host *vhost,
>  	}
>  }
>  
[ ... ]
> +static void ibmvfc_init_sub_crqs(struct ibmvfc_host *vhost)
> +{
> +	unsigned long flags;
> +
> +	ENTER;
> +	if (!vhost->mq_enabled)
> +		return;
> +
> +	/* register async_sub_crq channel */
> +	if (ibmvfc_alloc_channels(vhost, &vhost->async_scrqs))
> +		goto out;
> +
> +	if (ibmvfc_reg_sub_crqs(vhost, &vhost->async_scrqs))
> +		goto release_async_sub_crq;
> +
> +	if (ibmvfc_alloc_channels(vhost, &vhost->scsi_scrqs))
> +		goto deregister_async_sub_crq;
> +
> +	if (ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs))
> +		goto release_scsi_sub_crqs;
> +
> +	if (vhost->nvme_enabled) {
> +		if (ibmvfc_alloc_channels(vhost, &vhost->nvme_scrqs)) {
> +			spin_lock_irqsave(&vhost->host->host_lock, flags);
> +			vhost->nvme_enabled = 0;
> +			spin_unlock_irqrestore(&vhost->host->host_lock, flags);
> +		} else if (ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs)) {
> +			ibmvfc_release_channels(vhost, &vhost->nvme_scrqs);
> +			spin_lock_irqsave(&vhost->host->host_lock, flags);
> +			vhost->nvme_enabled = 0;
> +			spin_unlock_irqrestore(&vhost->host->host_lock, flags);
> +		}
> +	}

[Severity: High]
Does clearing vhost->do_enquiry and vhost->mq_enabled unconditionally inside
ibmvfc_reg_sub_crqs() cause inconsistent state and leaked sub-CRQ
registrations when a later channel set fails to register?

In ibmvfc_init_sub_crqs(), vhost->async_scrqs and vhost->scsi_scrqs are
allocated and registered first. When vhost->nvme_enabled is set and
ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs) fails, ibmvfc_init_sub_crqs()
appears to intend to only disable NVMe by releasing vhost->nvme_scrqs and
setting vhost->nvme_enabled = 0.

However, because ibmvfc_reg_sub_crqs() has already set vhost->do_enquiry = 0
and vhost->mq_enabled = 0, multi-queue and channel enquiry are disabled for
the entire host while vhost->async_scrqs and vhost->scsi_scrqs remain
allocated and registered with the hypervisor with active IRQ handlers.

Similarly, in ibmvfc_reenable_crq_queue() and ibmvfc_reset_crq():

	ibmvfc_reg_sub_crqs(vhost, &vhost->async_scrqs);
	ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
	ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs);

If registering vhost->async_scrqs succeeds, and a subsequent call to
ibmvfc_reg_sub_crqs() for vhost->scsi_scrqs or vhost->nvme_scrqs fails,
vhost->mq_enabled and vhost->do_enquiry are cleared to 0, but the already
registered channel sets are not deregistered via ibmvfc_dereg_sub_crqs().

Should the caller handle clearing vhost->mq_enabled and vhost->do_enquiry (or
unwinding previously registered channel sets) instead of clearing those flags
unconditionally in ibmvfc_reg_sub_crqs()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-ibmvfc-fpin-support-v16-0-a900aa657ed5@linux.ibm.com?part=11