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] ibmvfc_find_target incorrectly uses a logical OR condition, causing it to skip all valid targets and unconditionally return NULL.
- [Medium] Reference count leak of the SCSI host and remote port devices in the `ibmvfc_async_fpin_test` KUnit test.
- [Low] KUnit tests use undefined structures, macros, and incompatible function signatures from a future patch, breaking the build.
- [Low] Missing '&' operator when acquiring/releasing the host_lock, causing compilation to fail.
--

commit e20d55240604f534a630e6df915a24fb9e9b543b
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
> index aa7ae81df41b1..5259a80958ce1 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c

[ ... ]

> @@ -3411,6 +3450,25 @@ 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]
Should this check use a logical AND (&&) instead of a logical OR (||)?

Since the target lists are separated into SCSI and NVMe, it seems every
target will have either rport or nvme_remote_port set to NULL. Using
a logical OR here evaluates to true for every target, causing the loop
to unconditionally skip all valid targets and return NULL.

> +		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]
Does this line cause a build failure due to a missing '&' operator?

In struct Scsi_Host, host_lock is an inline spinlock_t rather than a
pointer. Passing it by value to spin_lock_irqsave() causes the compiler
to fail with an incompatible type error.

Should this be written as:
	spin_lock_irqsave(&vhost->host->host_lock, flags);

> +	tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, crq->scsi_id,
> +				 crq->wwpn, crq->node_name);

[ ... ]

> 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]
Do these undefined types and macros break the build when compiling with
CONFIG_SCSI_IBMVFC_KUNIT_TEST=y?

struct ibmvfc_async_crq_event and IBMVFC_ASYNC_CRQ_MAIN are not defined
in this commit, which prevents the tests from compiling. It appears they
might have leaked from a subsequent patch in the series.

> +	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;
> +
> +	vhost = ibmvfc_get_first_vhost();
> +	if (!vhost)
> +		kunit_skip(test, "No ibmvfc devices available");
> +
> +	spin_lock_irqsave(vhost->host->host_lock, flags);

[Severity: Low]
This also seems to be missing the '&' operator before vhost->host->host_lock,
resulting in the same build failure mentioned above.

> +	if (vhost->scsi_scrqs.num_targets < 1) {

[ ... ]

> +		ae[fs].async_crq.node_name = cpu_to_be64(tgt->ids.node_name);
> +		ibmvfc_handle_async(&ae[fs], vhost);

[Severity: Low]
Is there a signature mismatch here?

ibmvfc_handle_async() is defined to take a struct ibmvfc_async_crq pointer
as its first argument, but the test passes a struct ibmvfc_async_crq_event
pointer instead.

> +		ae[fs].async_crq.valid = 0;
> +		wmb(); /* ensure valid bit clear is visible before checking stats */
> +	}

[ ... ]

> +	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]
Are we leaking device and host references in the successful exit path?

The test acquires references via ibmvfc_get_first_vhost() and
get_device(&rport->dev) during setup, but unlike the other tests in this
file (e.g., ibmvfc_full_fpin_test), it does not call put_device(&rport->dev)
and scsi_host_put(vhost->host) before returning. Leaking these references
can prevent module unloading and device cleanup.

> +
> +/**
> + * 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]
Does this undefined structure break the build?

struct ibmvfc_async_subq_fpin is not defined in this commit, causing a
compilation error.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911021910.1038128-1-tyreld@linux.ibm.com?part=1