Re: [PATCH v11 04/11] scsi: ibmvfc: add basic FPIN support

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

Dave Marquardt <davemarq@linux.ibm.com> writes:

> Dave Marquardt <davemarq@linux.ibm.com> writes:
>
>> sashiko-bot@kernel.org writes:
>>
>>> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>>> - [Medium] scsi: ibmvfc: missing scsi_host_put() in test success path
>>> - [Medium] scsi: ibmvfc: missing host_lock around ibmvfc_handle_async() in KUnit test
>>> --
>>>
>>> commit 29ab8a9a78c94679b8187ae357d2978336830599
>>> 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. This includes async event
>>> handling and KUnit test infrastructure.
>>>
>>>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>>>> new file mode 100644
>>>> index 0000000000000..ec52285b0aa22
>>>> --- /dev/null
>>>> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>>>> @@ -0,0 +1,149 @@
>>> [ ... ]
>>>> +static void ibmvfc_async_fpin_test(struct kunit *test)
>>>> +{
>>> [ ... ]
>>>> +	vhost = ibmvfc_get_first_vhost();
>>>> +	if (!vhost)
>>>> +		kunit_skip(test, "No ibmvfc devices available");
>>> [ ... ]
>>>> +	for (fs = IBMVFC_AE_FPIN_LINK_CONGESTED; fs <= IBMVFC_AE_FPIN_CONGESTION_CLEARED; fs++) {
>>>> +		ae[fs].valid = 0x80;
>>>> +		ae[fs].link_state = IBMVFC_AE_LS_LINK_UP;
>>>> +		ae[fs].fpin_status = fs;
>>>> +		ae[fs].event = cpu_to_be64(IBMVFC_AE_FPIN);
>>>> +		ae[fs].scsi_id = cpu_to_be64(tgt->scsi_id);
>>>> +		ae[fs].wwpn = cpu_to_be64(tgt->wwpn);
>>>> +		ae[fs].node_name = cpu_to_be64(tgt->ids.node_name);
>>>> +		ibmvfc_handle_async(&ae[fs], vhost);
>>>
>>> [Severity: Medium]
>>> Does this invocation of ibmvfc_handle_async() need to be protected by
>>> the host_lock?
>>>
>>> If a concurrent device removal via ibmvfc_remove() sets vhost->state to
>>> IBMVFC_HOST_OFFLINE and destroys fpin_workq just before this handles the
>>> async event, could queue_work() be called on a freed workqueue?
>>
>> Yes, this routine should acquire the host_lock before calling ibmvfc_handle_async.
>
> So, the host_lock is taken early in the routine and held in this case.
> I'm getting mixed messages in my research on whether return or something
> like KUNIT_FAIL() is required after kunit_skip(). I will add return
> statements.

Contemplating this a little longer, I don't think any change is needed
here. I think this is a false positive by Sashiko. Is it confused by
kunit_skip() calls?

-Dave