Re: [PATCH v8 1/9] scsi: ibmvfc: add basic FPIN support

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

On 9/9/26 12:29 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Reordering `scsi_remove_host()` to execute after `ibmvfc_release_crq_queue()` causes a reliable deadlock and an infinite command requeue loop during module removal.
> - [Medium] NVMe over FC targets will not process FPIN events because they are incorrectly filtered out by `ibmvfc_find_target`.
> - [Medium] Missing `put_device()` and `scsi_host_put()` in the KUnit test `ibmvfc_async_fpin_test` causes resource leaks.
> --
> 
> commit bba4f1f01d900a3f01556bca91e5e90f72a7c959
> Author: Dave Marquardt <davemarq@linux.ibm.com>
> Subject: scsi: ibmvfc: add basic FPIN support
> 
> This commit implements support for a basic level of Fabric Performance
> Impact Notifications (FPIN) in the ibmvfc driver. It adds an async event
> handler for IBMVFC_AE_FPIN events that offloads processing to a dedicated
> workqueue, along with FPIN conversion routines and a KUnit test module.
> 
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index b3bc3ce872d6d..0dde8dcf43de3 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> 
> [ ... ]
> 
>> @@ -3261,16 +3266,259 @@ static const char *ibmvfc_get_link_state(enum ibmvfc_ae_link_state state)
>> +static struct ibmvfc_target *ibmvfc_find_target(struct list_head *target_list,
>> +						__be64 scsi_id, __be64 wwpn,
>> +						__be64 node_name)
>> +{
>> +	struct ibmvfc_target *tgt;
>> +
>> +	list_for_each_entry(tgt, target_list, queue) {
>> +		if (scsi_id && cpu_to_be64(tgt->scsi_id) != scsi_id)
>> +			continue;
>> +		if (wwpn && cpu_to_be64(tgt->ids.port_name) != wwpn)
>> +			continue;
>> +		if (node_name && cpu_to_be64(tgt->ids.node_name) != node_name)
>> +			continue;
>> +		if (!tgt->rport)
>> +			continue;

This should be (!tgt->rport || !tgt->nvme_remote_port)>
> [Severity: Medium]
> Does this check unintentionally filter out NVMe over FC targets?
> 
> Since NVMe targets populate tgt->nvme_remote_port instead of tgt->rport,
> this check forces ibmvfc_find_target() to always skip them.
> 
>> +		return tgt;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * ibmvfc_process_async_work - Process IBMVFC_AE_FPIN async CRQ from work queue
>> + * @work: pointer to work_struct
>> + */
>> +static void ibmvfc_process_async_work(struct work_struct *work)
>> +{
> 
> [ ... ]
> 
>> +	spin_lock_irqsave(vhost->host->host_lock, flags);
>> +	tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, crq->scsi_id,
>> +				 crq->wwpn, crq->node_name);
>> +	if (!tgt) {
>> +		/* Target not found in scsi_scrqs, search nvme_scrqs */
>> +		tgt = ibmvfc_find_target(&vhost->nvme_scrqs.targets,
>> +					 crq->scsi_id, crq->wwpn,
>> +					 crq->node_name);
>> +	}
> 
> [Severity: Medium]
> Because of the rport check in ibmvfc_find_target() mentioned above, won't
> this fallback search for NVMe targets always return NULL?
> 
> This appears to be dead code that will cause Fabric Performance Impact
> Notifications for NVMe over FC targets to be silently dropped.

See the change above.
> 
> [ ... ]
> 
>> @@ -6952,8 +7221,6 @@ static void ibmvfc_remove(struct vio_dev *vdev)
>>  	ibmvfc_wait_while_resetting(vhost);
>>  	kthread_stop(vhost->work_thread);
>>  	flush_work(&vhost->rport_add_work_q);
>> -	fc_remove_host(vhost->host);
>> -	scsi_remove_host(vhost->host);
>>  
>>  	spin_lock_irqsave(vhost->host->host_lock, flags);
>>  	ibmvfc_purge_requests(vhost, DID_ERROR);
>> @@ -6962,6 +7229,10 @@ static void ibmvfc_remove(struct vio_dev *vdev)
>>  	ibmvfc_complete_purge(&purge);
>>  	ibmvfc_release_sub_crqs(vhost);
>>  	ibmvfc_release_crq_queue(vhost);
>> +	destroy_workqueue(vhost->fpin_workq);
>> +	vhost->fpin_workq = NULL;
>> +	fc_remove_host(vhost->host);
>> +	scsi_remove_host(vhost->host);
> 
> [Severity: High]
> Can this reordering of scsi_remove_host() cause an infinite stall during
> module removal?
> 
> By the time scsi_remove_host() is called here, ibmvfc_release_crq_queue()
> has already run, overwriting the host state to IBMVFC_NO_CRQ. When the
> SCSI midlayer attempts to flush devices during removal,
> ibmvfc_host_chkready() will return DID_REQUEUE. The midlayer will retry
> the command repeatedly, leading to a deadlock since the hardware
> communication queues are already gone.
> 

Yeah, this is wrong. There is a reason that xxx_remove_host() calls were ahead
of the rest of the driver cleanup. The destroy_workqueue() call should have just
been moved ahead of those calls.

Seeing as Dave is now on vacation and I've already Acked the rest of the series
I'll send a v9 for this particular patch seeing as the rest of the series
depends on it.

-Tyrel