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

[sashiko-bot@kernel.org]

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;

[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.

[ ... ]

> @@ -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.

[ ... ]

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> new file mode 100644
> index 0000000000000..80e7f8e0fd700
> --- /dev/null
> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c

[ ... ]

> +static void ibmvfc_async_fpin_test(struct kunit *test)
> +{

[ ... ]

> +	KUNIT_EXPECT_EQ(test, pre[IBMVFC_AE_FPIN_PORT_DEGRADED],
> +			post[IBMVFC_AE_FPIN_PORT_DEGRADED]);
> +	KUNIT_EXPECT_EQ(test, pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED],
> +			post[IBMVFC_AE_FPIN_CONGESTION_CLEARED]);
> +
> +	kref_put(&tgt->kref, ibmvfc_release_tgt);
> +}

[Severity: Medium]
Does this exit path leak the references to vhost->host and rport->dev?

The test acquires references via ibmvfc_get_first_vhost() and
get_device(&rport->dev) during initialization, but appears to omit the
corresponding scsi_host_put() and put_device() calls at the end of the
test, unlike the sibling tests in this module.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-ibmvfc-fpin-support-v8-0-b27183b055af@linux.ibm.com?part=1