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

[Dave Marquardt <davemarq@linux.ibm.com>]

sashiko-bot@kernel.org writes:

>> 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()?

The callers will handle deregistering sub CRQs and clearing
vhost->mq_enabled and vhost->do_enquiry in the next revision.

-Dave