Re: [PATCH v2 1/7] ibmvfc: add basic FPIN support

[Tyrel Datwyler <tyreld@linux.ibm.com>]

On 6/8/26 11:30 AM, Dave Marquardt via B4 Relay wrote:
> From: Dave Marquardt <davemarq@linux.ibm.com>
> 
> Add support for basic FPIN messages to the ibmvfc driver. This includes
> 
> - adding FPIN handling support to the async event handler
> - offloading processing of FPIN messages to a work queue
> - converting the VIOS FPIN message to a struct fc_els_fpin as used by
>   the Linux kernel
> - passing the converted struct fc_els_fpin to fc_host_fpin_rcv for
>   processing
> 
> The FPIN message conversion routines include a common routine that
> will also be used in patches 6 and 7, which add full and extended FPIN
> support.

You are missing a Signed-off-by tag here.

> ---
>  drivers/scsi/Kconfig                 |  10 ++
>  drivers/scsi/ibmvscsi/Makefile       |   1 +
>  drivers/scsi/ibmvscsi/ibmvfc.c       | 226 ++++++++++++++++++++++++++++++++++-
>  drivers/scsi/ibmvscsi/ibmvfc.h       |  15 +++
>  drivers/scsi/ibmvscsi/ibmvfc_kunit.c | 131 ++++++++++++++++++++
>  5 files changed, 379 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index c3042393af23..d5fc7eb2ebb1 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -758,6 +758,16 @@ config SCSI_IBMVFC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ibmvfc.
>  
> +config SCSI_IBMVFC_KUNIT_TEST
> +	tristate "KUnit tests for the IBM POWER Virtual FC Client" if !KUNIT_ALL_TESTS
> +	depends on SCSI_IBMVFC && KUNIT
> +	default KUNIT_ALL_TESTS
> +	help
> +	  Compile IBM POWER Virtual FC client KUnit tests. These tests
> +	  specifically test FPIN functionality. To compile this driver
> +	  as a module, choose M here: the module will be called
> +	  ibmvfc_kunit.
> +
>  config SCSI_IBMVFC_TRACE
>  	bool "enable driver internal trace"
>  	depends on SCSI_IBMVFC
> diff --git a/drivers/scsi/ibmvscsi/Makefile b/drivers/scsi/ibmvscsi/Makefile
> index 5eb1cb1a0028..75dc7aee15a0 100644
> --- a/drivers/scsi/ibmvscsi/Makefile
> +++ b/drivers/scsi/ibmvscsi/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  obj-$(CONFIG_SCSI_IBMVSCSI)	+= ibmvscsi.o
>  obj-$(CONFIG_SCSI_IBMVFC)	+= ibmvfc.o
> +obj-$(CONFIG_SCSI_IBMVFC_KUNIT_TEST)	+= ibmvfc_kunit.o
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index 3dd2adda195e..9e5f0c0f0369 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
> @@ -30,6 +30,9 @@
>  #include <scsi/scsi_tcq.h>
>  #include <scsi/scsi_transport_fc.h>
>  #include <scsi/scsi_bsg_fc.h>
> +#include <kunit/visibility.h>
> +#include <scsi/fc/fc_els.h>
> +#include <linux/overflow.h>
>  #include "ibmvfc.h"
>  
>  static unsigned int init_timeout = IBMVFC_INIT_TIMEOUT;
> @@ -3137,6 +3140,7 @@ static const struct ibmvfc_async_desc ae_desc [] = {
>  	{ "Halt",	IBMVFC_AE_HALT,		IBMVFC_DEFAULT_LOG_LEVEL },
>  	{ "Resume",	IBMVFC_AE_RESUME,	IBMVFC_DEFAULT_LOG_LEVEL },
>  	{ "Adapter Failed", IBMVFC_AE_ADAPTER_FAILED, IBMVFC_DEFAULT_LOG_LEVEL },
> +	{ "FPIN",	IBMVFC_AE_FPIN,		IBMVFC_DEFAULT_LOG_LEVEL },
>  };
>  
>  static const struct ibmvfc_async_desc unknown_ae = {
> @@ -3185,17 +3189,211 @@ static const char *ibmvfc_get_link_state(enum ibmvfc_ae_link_state state)
>  	return "";
>  }
>  
> +#define IBMVFC_FPIN_CONGN_DESC_SZ (sizeof(struct fc_els_fpin) + sizeof(struct fc_fn_congn_desc))
> +#define IBMVFC_FPIN_LI_DESC_SZ (sizeof(struct fc_els_fpin) + \
> +				struct_size_t(struct fc_fn_li_desc, pname_list, 1))
> +#define IBMVFC_FPIN_PEER_CONGN_DESC_SZ (sizeof(struct fc_els_fpin) + \
> +					struct_size_t(struct fc_fn_peer_congn_desc, pname_list, 1))
> +
> +/**
> + * ibmvfc_fpin_size_helper(): compute fpin structure size based on fpin status
> + * @fpin_status: status value
> + *
> + * Return:
> + * 0: invalid fpin_status
> + * other: valid size
> + */
> +static size_t ibmvfc_fpin_size_helper(u8 fpin_status)
> +{
> +	size_t size = 0;
> +
> +	switch (fpin_status) {
> +	case IBMVFC_AE_FPIN_LINK_CONGESTED:
> +	case IBMVFC_AE_FPIN_CONGESTION_CLEARED:
> +		size = IBMVFC_FPIN_CONGN_DESC_SZ;
> +		break;
> +	case IBMVFC_AE_FPIN_PORT_CONGESTED:
> +	case IBMVFC_AE_FPIN_PORT_CLEARED:
> +		size = IBMVFC_FPIN_PEER_CONGN_DESC_SZ;
> +		break;
> +	case IBMVFC_AE_FPIN_PORT_DEGRADED:
> +		size = IBMVFC_FPIN_LI_DESC_SZ;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return size;
> +}
> +
> +/**
> + * ibmvfc_common_fpin_to_desc(): allocate and populate a struct fc_els_fpin struct
> + * containing a descriptor.
> + *
> + * Allocate a struct fc_els_fpin containing a descriptor and populate
> + * based on data from *ibmvfc_fpin.
> + *
> + * Return:
> + * NULL     - unable to allocate structure
> + * non-NULL - pointer to populated struct fc_els_fpin
> + */
> +static struct fc_els_fpin *
> +ibmvfc_common_fpin_to_desc(u8 fpin_status, __be64 wwpn, __be16 modifier,
> +			   __be32 period, __be32 threshold, __be32 event_count)
> +{
> +	struct fc_fn_peer_congn_desc *pdesc;
> +	struct fc_fn_congn_desc *cdesc;
> +	struct fc_fn_li_desc *ldesc;
> +	struct fc_els_fpin *fpin;
> +	size_t size;
> +
> +	size = ibmvfc_fpin_size_helper(fpin_status);
> +	if (size == 0)

To be consistent with other places in the driver !size instead of size == 0 check.

> +		return NULL;
> +
> +	fpin = kzalloc(size, GFP_KERNEL);
> +	if (fpin == NULL)

Same nit here !fpin instead of fpin == NULL.

> +		return NULL;
> +
> +	fpin->fpin_cmd = ELS_FPIN;
> +
> +	switch (fpin_status) {
> +	case IBMVFC_AE_FPIN_CONGESTION_CLEARED:
> +	case IBMVFC_AE_FPIN_LINK_CONGESTED:
> +		fpin->desc_len = cpu_to_be32(sizeof(struct fc_fn_congn_desc));
> +		cdesc = (struct fc_fn_congn_desc *)fpin->fpin_desc;
> +		cdesc->desc_tag = cpu_to_be32(ELS_DTAG_CONGESTION);
> +		cdesc->desc_len = cpu_to_be32(FC_TLV_DESC_LENGTH_FROM_SZ(*cdesc));
> +		if (fpin_status == IBMVFC_AE_FPIN_CONGESTION_CLEARED)
> +			cdesc->event_type = cpu_to_be16(FPIN_CONGN_CLEAR);
> +		else
> +			cdesc->event_type = cpu_to_be16(FPIN_CONGN_DEVICE_SPEC);
> +		cdesc->event_modifier = modifier;
> +		cdesc->event_period = period;
> +		cdesc->severity = FPIN_CONGN_SEVERITY_WARNING;
> +		break;
> +	case IBMVFC_AE_FPIN_PORT_CONGESTED:
> +	case IBMVFC_AE_FPIN_PORT_CLEARED:
> +		fpin->desc_len =
> +			cpu_to_be32(struct_size_t(struct fc_fn_peer_congn_desc, pname_list, 1));
> +		pdesc = (struct fc_fn_peer_congn_desc *)fpin->fpin_desc;
> +		pdesc->desc_tag = cpu_to_be32(ELS_DTAG_PEER_CONGEST);
> +		pdesc->desc_len = cpu_to_be32(FC_TLV_DESC_LENGTH_FROM_SZ(*pdesc));
> +		if (fpin_status == IBMVFC_AE_FPIN_PORT_CLEARED)
> +			pdesc->event_type = cpu_to_be16(FPIN_CONGN_CLEAR);
> +		else
> +			pdesc->event_type = cpu_to_be16(FPIN_CONGN_DEVICE_SPEC);
> +		pdesc->event_modifier = modifier;
> +		pdesc->event_period = period;
> +		pdesc->detecting_wwpn = cpu_to_be64(0);

While not wrong 0 is 0 regardless of endianiess. Is this value always going to
be zero here or is this place holder code that changes later in this patchset?
If its the case this isn't a place holder the original fpin struct was allocated
with kzalloc so this value should be zero already.

> +		pdesc->attached_wwpn = wwpn;
> +		pdesc->pname_count = cpu_to_be32(1);
> +		pdesc->pname_list[0] = wwpn;
> +		break;
> +	case IBMVFC_AE_FPIN_PORT_DEGRADED:
> +		fpin->desc_len = cpu_to_be32(struct_size_t(struct fc_fn_li_desc, pname_list, 1));
> +		ldesc = (struct fc_fn_li_desc *)fpin->fpin_desc;
> +		ldesc->desc_tag = cpu_to_be32(ELS_DTAG_LNK_INTEGRITY);
> +		ldesc->desc_len = cpu_to_be32(FC_TLV_DESC_LENGTH_FROM_SZ(*ldesc));
> +		ldesc->event_type = cpu_to_be16(FPIN_LI_UNKNOWN);
> +		ldesc->event_modifier = modifier;
> +		ldesc->event_threshold = threshold;
> +		ldesc->event_count = event_count;
> +		ldesc->detecting_wwpn = cpu_to_be64(0);

Same comment as above.

> +		ldesc->attached_wwpn = wwpn;
> +		ldesc->pname_count = cpu_to_be32(1);
> +		ldesc->pname_list[0] = wwpn;
> +		break;
> +	default:
> +		/* This should be caught above. */
> +		kfree(fpin);
> +		fpin = NULL;
> +		break;
> +	}
> +
> +	return fpin;
> +}
> +
> +/**
> + * ibmvfc_basic_fpin_to_desc(): allocate and populate a struct fc_els_fpin struct
> + * containing a descriptor.
> + * @ibmvfc_fpin: Pointer to async crq
> + *
> + * Allocate a struct fc_els_fpin containing a descriptor and populate
> + * based on data from *ibmvfc_fpin.
> + *
> + * Return:
> + * NULL     - unable to allocate structure
> + * non-NULL - pointer to populated struct fc_els_fpin
> + */
> +static struct fc_els_fpin *
> +ibmvfc_basic_fpin_to_desc(struct ibmvfc_async_crq *crq, u64 wwpn)
> +{
> +	return ibmvfc_common_fpin_to_desc(crq->fpin_status, cpu_to_be64(wwpn),
> +					  cpu_to_be16(0),
> +					  cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_PERIOD),
> +					  cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_THRESHOLD),
> +					  cpu_to_be32(1));
> +}
> +
> +/**
> + * ibmvfc_process_async_work - Process IBMVFC_AE_FPIN async CRQ from work queue
> + * @work: pointer to work_struct
> + */
> +static void ibmvfc_process_async_work(struct work_struct *work)
> +{
> +	struct ibmvfc_async_work *aw;
> +	struct ibmvfc_async_crq *crq;
> +	struct ibmvfc_target *tgt;
> +	struct ibmvfc_host *vhost;
> +	struct fc_els_fpin *fpin;
> +
> +	aw = container_of(work, struct ibmvfc_async_work, async_work_s);
> +	crq = aw->crq;
> +	vhost = aw->vhost;
> +
> +	if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
> +		goto end;
> +	list_for_each_entry(tgt, &vhost->targets, queue) {

Should we be holding the host lock when iterateing targets?

-Tyrel