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

[sashiko-bot@kernel.org]

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

commit ed8465cac668f337315cf039e8fff2bc929555e8
Author: Dave Marquardt <davemarq@linux.ibm.com>

scsi: ibmvfc: handle extended FPIN events

This commit implements support for extended FPIN messages received via the
asynchronous sub-queue. It adds conversion logic to populate complete
descriptor information and includes KUnit tests to exercise the extended
event handling.

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

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

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-ibmvfc-fpin-support-v12-0-53a3fff7d77b@linux.ibm.com?part=11