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

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

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] scsi: ibmvfc: double registration of scsi_scrqs and missing async_scrqs registration
> --
>
> commit d44dbe8300fc3639dbfcef8f1950443462606f5b
> Author: Dave Marquardt <davemarq@linux.ibm.com>
>
> scsi: ibmvfc: register and use asynchronous sub CRQ for events
>
> This commit wires the async sub-CRQ into the sub-CRQ lifecycle so it is
> allocated, registered, and freed alongside the SCSI channel queues.
>
>> 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
> [ ... ]
>> @@ -986,6 +987,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->scsi_scrqs);
>>  	ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
>>  	ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs);
>>  
>
> [Severity: High]
> Could this introduce a regression where scsi_scrqs is accidentally registered
> twice instead of registering async_scrqs?
>
> Because the first call was added as ibmvfc_reg_sub_crqs(vhost,
> &vhost->scsi_scrqs), it appears to skip the async sub-CRQ registration
> entirely. This leaves the async sub-CRQ permanently disabled after a CRQ
> re-enablement, which could prevent asynchronous event handling (like FPINs)
> after recovery.

Yes, this is broken. The added call should be

	ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);

and will be fixed in v12.

> Also, wouldn't the double registration of scsi_scrqs cause
> ibmvfc_register_channel() to fail? If H_REG_SUB_CRQ returns an error for an
> already registered channel, this will abort the registration and clear
> vhost->do_enquiry = 0, functionally breaking multi-queue capabilities during
> standard operational recovery paths.

-Dave