Re: [PATCH v12 11/11] scsi: ibmvfc: handle extended FPIN events

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

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] scsi: ibmvfc: unprocessed FPIN severity in extended FPIN events
> - [Medium] scsi: ibmvfc: KUnit tests for extended FPIN silently lack assertions and bypass core logic

>> 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
> [ ... ]
>> @@ -3471,6 +3472,41 @@ ibmvfc_full_fpin_to_desc(struct ibmvfc_async_sub_crq *ibmvfc_fpin, u64 wwpn)
>> +static struct fc_els_fpin *
>> +ibmvfc_ext_fpin_to_desc(struct ibmvfc_async_subq_fpin *ibmvfc_fpin)
>> +{
>> +	u8 flags = ibmvfc_fpin->fpin_data.flags;
>> +	__be32 threshold = cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_THRESHOLD);
>> +	__be16 modifier = 0;
>> +	__be32 count = cpu_to_be32(1);
>> +	__be16 type = 0;
>> +
>> +	if (flags & IBMVFC_FPIN_EVENT_TYPE_VALID)
>> +		type = ibmvfc_fpin->fpin_data.event_type;
>> +	if (flags & IBMVFC_FPIN_MODIFIER_VALID)
>> +		modifier = ibmvfc_fpin->fpin_data.event_type_modifier;
>> +	if (flags & IBMVFC_FPIN_THRESHOLD_VALID)
>> +		threshold = ibmvfc_fpin->fpin_data.event_threshold;
>> +	if (flags & IBMVFC_FPIN_EVENT_COUNT_VALID)
>> +		count = ibmvfc_fpin->fpin_data.event_data.event_count;
>> +
>> +	return ibmvfc_common_fpin_to_desc(ibmvfc_fpin->fpin_status,
>
> [Severity: Medium]
> Does this code intentionally discard the event severity?
>
> The extended FPIN payload can specify severity when the
> IBMVFC_FPIN_SEVERITY_VALID flag is set, but that flag appears to be ignored
> during this unpacking process.
>
> Furthermore, looking at the existing ibmvfc_common_fpin_to_desc() function
> that this calls, it hardcodes the generated severity:
>
> static struct fc_els_fpin *
> ibmvfc_common_fpin_to_desc(...)
> {
> ...
>         cdesc->severity = FPIN_CONGN_SEVERITY_WARNING;
> ...
> }
>
> Will this cause higher severity events sent by the VIOS to be permanently
> downgraded and incorrectly reported to the FC transport layer?

For the next revision I'm updating ibmvfc_common_fpin_to_desc() to have
a __u8 severity parameter, and its wrappers to pass either
FPIN_CONGN_SEVERITY_WARNING as a default or in the case of
ibmvfc_ext_fpin_to_desc() using the passed severity if
the severity passed in the ibmvfc_async_subq_fpin data is set.

> [ ... ]
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>> --- a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> [ ... ]
>> @@ -266,9 +267,147 @@ static void ibmvfc_full_fpin_test(struct kunit *test)
>> +#define IBMVFC_TEST_FPIN_EXT(fs, ev, stat, crq) {				\
>> +	struct ibmvfc_async_crq_event ae = { .type = IBMVFC_ASYNC_CRQ_SUB };	\
>> +	(crq).valid = 0x80;							\
>> +	(crq).flags = IBMVFC_ASYNC_IS_FPIN_EXT;					\
>> +	(crq).link_state = IBMVFC_AE_LS_LINK_UP;				\
>> +	(crq).fpin_status = (fs);						\
>> +	(crq).event = cpu_to_be16(IBMVFC_AE_FPIN);				\
>> +	(crq).wwpn = cpu_to_be64(tgt->wwpn);					\
>> +	(crq).fpin_data.flags = IBMVFC_FPIN_EVENT_TYPE_VALID;			\
>> +	(crq).fpin_data.event_type = cpu_to_be16((ev));				\
>> +	ae.subq = *(struct ibmvfc_async_sub_crq *)&(crq);			\
>> +	pre = READ_ONCE(rport->fpin_stats.stat);				\
>> +	spin_lock_irqsave(vhost->host->host_lock, flags);			\
>> +	ibmvfc_handle_async(&ae, vhost);					\
>> +	spin_unlock_irqrestore(vhost->host->host_lock, flags);			\
>> +	flush_workqueue(vhost->fpin_workq);					\
>> +	post = READ_ONCE(rport->fpin_stats.stat);				\
>> +}
>
> [Severity: Medium]
> Are we missing the KUnit assertions in this macro?
>
> The test records the pre and post statistics, but there are no checks
> (such as KUNIT_EXPECT_GT(test, post, pre)) to verify the statistics actually
> incremented.

Yes. Adding KUNIT_EXPECT_GT here.

> Because this test uses a live global vhost via ibmvfc_get_first_vhost()
> without mocking the IBMVFC_SUPPORT_FPIN_EXT capability, a test run on an
> adapter lacking this capability will silently fall through to this bypass path
> in ibmvfc_process_async_work():
>
>     } else if (!ibmvfc_check_caps(vhost, IBMVFC_SUPPORT_FPIN_EXT)) {
>             dev_err_ratelimited(vhost->dev,
>                                 "Unexpected extended FPIN event received\n");
>     }
>
> Since there are no assertions, does this mean the test still reports a pass
> despite completely failing to process the events?

I've added the same check in ibmvfc_extended_fpin_test() and will skip
the test if IBMVFC_SUPPORT_FPIN_EXT is not set.

-Dave