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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The KUnit test `ibmvfc_async_fpin_test` leaks references to the `Scsi_Host` and the `fc_rport` device on its successful completion path.
- [Medium] Unordered workqueue allows FPIN events to be processed concurrently, causing data races in FC transport statistics and FPIN event reordering.
--

commit 79377784bc96c798354f5b0f5ce5007a8d857d16
Author: Dave Marquardt <davemarq@linux.ibm.com>
    
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 that
offloads FPIN processing to a dedicated workqueue and passes the converted
messages to the FC transport layer. A KUnit test module is also introduced to
validate the FPIN event handling and statistics updates.

> 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
> @@ -3547,6 +3547,56 @@ static void ibmvfc_process_async_work(struct work_struct *work)
[ ... ]
> +	if (fpin) {
> +		fc_host_fpin_rcv(tgt->vhost->host,
> +				 sizeof(*fpin) + be32_to_cpu(fpin->desc_len),
> +				 (char *)fpin, 0);
> +		kfree(fpin);
> +	} else
    
[Severity: Medium]
Can the lockless modifications to fpin_stats in the FC transport layer here
cause data races and lost statistic counts?
    
Multiple FPIN events arriving rapidly from the adapter are queued to the
fpin_workq. Because the workqueue is created below as WQ_UNBOUND with
max_active=0, these events can be processed concurrently.
    
Furthermore, since FPIN events are stateful (e.g., congestion detected vs
cleared), could concurrent processing deliver events out of order to the
midlayer and userspace, leaving the system reflecting an incorrect state?

[ ... ]
> @@ -7135,6 +7135,15 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id)
[ ... ]
> +	vhost->fpin_workq = alloc_workqueue("%s-fpin-workq-%u", WQ_UNBOUND, 0,
> +						 IBMVFC_NAME, shost->host_no);
    
[Severity: Medium]
By setting the max_active parameter to 0, which defaults to 256 concurrent
execution threads, does this allow the unordered processing that triggers
the data races mentioned above?
    
[ ... ]
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> --- /dev/null
> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> @@ -0,0 +1,148 @@
[ ... ]
> +static void ibmvfc_async_fpin_test(struct kunit *test)
> +{
[ ... ]
> +	vhost = ibmvfc_get_first_vhost();
> +	if (!vhost)
[ ... ]
> +	rport = tgt->rport;
> +	get_device(&rport->dev);
[ ... ]
> +	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 code leak the references to the Scsi_Host and the fc_rport device
on the successful completion path?
    
The setup code earlier in this test acquires references via
ibmvfc_get_first_vhost (which takes the host reference) and get_device, but
this success path fails to drop them before returning. This could potentially
prevent the driver from unloading after running the KUnit test module.

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