Re: [PATCH v8 9/9] 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] Extended FPIN severity information provided by the hardware is ignored and always downgraded to a warning.
- [Low] Unused variables `pre` and `post` in `ibmvfc_extended_fpin_test` cause a `-Wunused-but-set-variable` compiler warning.
--

commit 6820bc74ced043fe70cb0c57ff71b54e4764889c
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 functions to convert extended FPIN messages to fc_els_fpin
structures, set capability bits during login, and add KUnit tests.

> 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

[ ... ]

> +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,
> +					  ibmvfc_fpin->wwpn, type,
> +					  modifier, threshold, count);
> +}

[Severity: Medium]
Does this code accidentally ignore the hardware-provided FPIN severity? 

When an extended FPIN event with the IBMVFC_FPIN_SEVERITY_VALID flag is
received, the payload parsing seems to ignore the severity field. Because
ibmvfc_common_fpin_to_desc() unconditionally hardcodes the severity to
FPIN_CONGN_SEVERITY_WARNING, does this lead to losing critical fabric
congestion event severity information (such as Error or Fatal statuses)
from the hardware?

[ ... ]

> 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

[ ... ]

> +#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);				\
> +	ibmvfc_handle_async(&ae, vhost);					\
> +	flush_workqueue(vhost->fpin_workq);					\
> +	post = READ_ONCE(rport->fpin_stats.stat);				\
> +}

[Severity: Low]
Are the pre and post variables missing evaluation in the test assertions?

They are assigned via READ_ONCE() but never used subsequently in the macro or
calling functions, which triggers a -Wunused-but-set-variable compiler
warning when KUnit is enabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-ibmvfc-fpin-support-v8-0-b27183b055af@linux.ibm.com?part=9