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

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

On 9/10/26 11:06 PM, sashiko-bot@kernel.org wrote:
> 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.

Makes sense. See response below.
>     
> 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?

I think max_active should be set to 1 here to enforce ordered processing.

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

If we are going to bother to take a reference might as well drop it.

-Tyrel