Re: [PATCH v7 8/8] 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 1 potential issue(s) to consider:
> - [Medium] The `ibmvfc_extended_fpin_test` test suite is entirely invalid due to missing KUnit assertions and incorrect statistics targeting.
> --
>
> commit 7f2470a8cbfd29182f9253c971ce356e69e43c8e
> Author: Dave Marquardt <davemarq@linux.ibm.com>
>
> scsi: ibmvfc: handle extended FPIN events
>
> Implement support for extended FPIN messages received via the asynchronous 
> sub-queue. Add parsing and processing logic to convert these into 
> fc_els_fpin structures. Include comprehensive KUnit tests to validate the 
> extended FPIN event handling.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>> index 47f7c4e79cd81..b016c7da24cd6 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>
> [ ... ]
>
>> +#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(tgt->rport->fpin_stats.stat);				\
>> +	ibmvfc_handle_async(&ae, vhost);					\
>> +	flush_workqueue(vhost->fpin_workq);					\
>> +	post = READ_ONCE(tgt->rport->fpin_stats.stat);				\
>> +}
>
> [Severity: Medium]
> Does this macro actually verify the statistics have changed? It captures the
> pre and post state, but doesn't appear to perform any KUnit assertions
> (like KUNIT_EXPECT_GE) to validate the test results.

This test is about exercising the extended FPIN functionality, and does
not test for correctness. I will add a comment about this in the next
patch revision.

> Additionally, this macro always queries tgt->rport->fpin_stats for all events.
> Is this correct for host-level link congestion events which might update
> fc_host->fpin_stats instead?
>
>> +
>> +/**
>> + * ibmvfc_extended_fpin_test - unit test for extended FPIN events
>> + * @test: pointer to kunit structure
>> + *
>> + * Tests
>> + *
>> + * Return: void
>> + */
>> +static void ibmvfc_extended_fpin_test(struct kunit *test)
>> +{
>> +	enum ibmvfc_ae_fpin_status fs;
>
> [ ... ]
>
>> +	for (fs = IBMVFC_AE_FPIN_LINK_CONGESTED; fs <= IBMVFC_AE_FPIN_CONGESTION_CLEARED; fs++) {
>> +		switch (fs) {
>> +		case IBMVFC_AE_FPIN_PORT_CLEARED:
>> +		case IBMVFC_AE_FPIN_CONGESTION_CLEARED: {
>> +			struct ibmvfc_async_crq_event ae = { .type = IBMVFC_ASYNC_CRQ_SUB };
>> +
>> +			crq[fs].valid = 0x80;
>> +			crq[fs].flags = IBMVFC_ASYNC_IS_FPIN_EXT;
>> +			crq[fs].link_state = IBMVFC_AE_LS_LINK_UP;
>> +			crq[fs].fpin_status = fs;
>> +			crq[fs].event = cpu_to_be16(IBMVFC_AE_FPIN);
>> +			crq[fs].wwpn = cpu_to_be64(tgt->wwpn);
>> +			crq[fs].fpin_data.flags = IBMVFC_FPIN_EVENT_TYPE_VALID;
>> +			crq[fs].fpin_data.event_type = cpu_to_be16(FPIN_CONGN_CLEAR);
>> +			ae.subq = *(struct ibmvfc_async_sub_crq *)&crq[fs];
>> +			pre = READ_ONCE(tgt->rport->fpin_stats.cn_clear);
>> +			ibmvfc_handle_async(&ae, vhost);
>> +			flush_workqueue(vhost->fpin_workq);
>> +			post = READ_ONCE(tgt->rport->fpin_stats.cn_clear);
>> +			break;
>> +		}
>
> [Severity: Medium]
> Similar to the macro above, is there a missing KUnit assertion here in
> ibmvfc_extended_fpin_test() to validate that the statistics were properly
> updated?