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

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

Tyrel Datwyler <tyreld@linux.ibm.com> writes:

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

Okay.

>> +		return NULL;
>> +
>> +	fpin = kzalloc(size, GFP_KERNEL);
>> +	if (fpin == NULL)
>
> Same nit here !fpin instead of fpin == NULL.

Okay.

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

I'll drop this.

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

fc_host_fpin_rcv assumes no locks are held. I built a kernel with
LOCKDEP, PROVE_LOCKING, DEBUG_SPINLOCK et al defined, and got some
deadlocks. Some of the routines fc_host_fpin_rcv call acquire the host
lock also.

I'l change this code to use list_for_each_entry_safe. Is that enough for
safety in this case?

-Dave