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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The target search function `ibmvfc_find_target` incorrectly uses a logical OR (`||`) when checking if a target has a remote port, causing it to return NULL for all valid single-protocol targets and dropping all FPIN events.
- [Medium] The `ibmvfc_async_fpin_test` KUnit test leaks references to the `rport` device and the SCSI host upon successful completion.
- [Low] The KUnit tests rely on undeclared structures (`ibmvfc_async_crq_event` and `ibmvfc_async_subq_fpin`), leading to immediate compilation failure. This regression is fixed later in the patch series.
- [Low] Missing address-of operator '&' when passing 'vhost->host->host_lock' to spinlock macros in `ibmvfc_process_async_work`, causing a compilation error.
--

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

scsi: ibmvfc: add basic FPIN support

Implement support for a basic level of Fabric Performance Impact
Notifications (FPIN) in the ibmvfc driver to enable monitoring of
fabric congestion and link integrity events.

> 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
[ ... ]
> @@ -3522,6 +3526,23 @@ static struct fc_els_fpin *
>  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 || !tgt->nvme_remote_port)
>  			continue;

[Severity: High]
Does this skip all valid targets? A target is typically either a SCSI or NVMe
target, but not both simultaneously. Checking with a logical OR means we
continue the loop whenever either one is NULL, causing ibmvfc_find_target()
to always return NULL and subsequently drop all FPIN events.

>  		return tgt;
>  	}
>  
>  	return NULL;
>  }
[ ... ]
> +static void ibmvfc_process_async_work(struct work_struct *work)
> +{
> +	struct ibmvfc_async_work *aw;
> +	struct ibmvfc_async_crq *crq;
> +	struct ibmvfc_target *tgt;
> +	struct ibmvfc_host *vhost;
> +	struct fc_els_fpin *fpin;
> +	unsigned long flags;
> +
> +	aw = container_of_const(work, struct ibmvfc_async_work, async_work_s);
> +	vhost = aw->vhost;
> +	crq = &aw->crq;
> +
> +	if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
> +		goto free;
> +
> +	spin_lock_irqsave(vhost->host->host_lock, flags);

[Severity: Low]
Can this compile successfully? The spinlock macros expect a pointer, but
vhost->host->host_lock is passed by value here, which will result in an
incompatible type compilation error. Should this use the address-of operator
(&vhost->host->host_lock)?

> +	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);
> +	}
> +
> +	if (tgt) {
> +		kref_get(&tgt->kref);
> +		spin_unlock_irqrestore(vhost->host->host_lock, flags);

[Severity: Low]
This is also missing the address-of operator and will cause a compilation
error.

> +	} else {
> +		spin_unlock_irqrestore(vhost->host->host_lock, flags);

[Severity: Low]
This is also missing the address-of operator.

> +		dev_err_ratelimited(vhost->dev, "Invalid target for FPIN\n");
> +		goto free;
> +	}
> +
[ ... ]
> 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
> @@ -0,0 +1,408 @@
[ ... ]
> +static void ibmvfc_async_fpin_test(struct kunit *test)
> +{
> +	u64 post[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1];
> +	u64 pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1];
> +	struct ibmvfc_async_crq_event ae[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1] = {
> +		[0 ... IBMVFC_AE_FPIN_CONGESTION_CLEARED] = { .type = IBMVFC_ASYNC_CRQ_MAIN },
> +	};

[Severity: Low]
Does this test rely on an undeclared structure? The struct
ibmvfc_async_crq_event doesn't appear to be declared yet in this patch,
which breaks the build if KUnit tests are enabled. I noticed this gets
resolved in a subsequent commit, but it breaks the build here.

> +	enum ibmvfc_ae_fpin_status fs;
> +	struct fc_host_attrs *fc_host;
> +	struct ibmvfc_target *tgt;
> +	struct ibmvfc_host *vhost;
> +	struct fc_rport *rport;
> +	unsigned long flags;
[ ... ]
> +	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 test leak device references? Early in ibmvfc_async_fpin_test(),
scsi_host_get() and get_device(&rport->dev) are called to acquire references,
but they don't seem to have corresponding scsi_host_put(vhost->host) and
put_device(&rport->dev) cleanup calls at the end of the test function.

> +
> +/**
> + * ibmvfc_full_fpin_test - unit test for IBMVFC_AE_FPIN parts of ibmvfc_handle_async
[ ... ]
> +static void ibmvfc_extended_fpin_test(struct kunit *test)
> +{
> +	enum ibmvfc_ae_fpin_status fs;
> +	struct ibmvfc_async_subq_fpin crq[IBMVFC_AE_FPIN_CONGESTION_CLEARED+1] = {};

[Severity: Low]
Similar to the ibmvfc_async_crq_event array above, this instantiates an array
of struct ibmvfc_async_subq_fpin which is currently undeclared in this
patchset, causing a build failure.

> +	struct ibmvfc_async_subq_fpin
> +		crqcn[IBMVFC_AE_FPIN_PORT_CONGESTED][FPIN_CONGN_DEVICE_SPEC+1] = {};

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