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

[Dave Marquardt <davemarq@linux.ibm.com>]

Tyrel Datwyler <tyreld@linux.ibm.com> writes:

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

Or use alloc_ordered_workqueue() here instead.

-Dave