Re: [PATCH v2 1/7] ibmvfc: add basic FPIN support
From: Tyrel Datwyler <tyreld@linux.ibm.com>
Date: 2026-06-15 23:39:05
Also in:
linux-scsi, lkml
On 6/8/26 11:30 AM, Dave Marquardt via B4 Relay wrote:
quoted hunk ↗ jump to hunk
From: Dave Marquardt <redacted> 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. --- 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_IBMVFCdiff --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.odiff --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) + return NULL; + + fpin = kzalloc(size, GFP_KERNEL); + if (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); + 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); + 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) { + if (crq->scsi_id && cpu_to_be64(tgt->scsi_id) != crq->scsi_id) + continue; + if (crq->wwpn && cpu_to_be64(tgt->ids.port_name) != crq->wwpn) + continue; + if (crq->node_name && cpu_to_be64(tgt->ids.node_name) != crq->node_name) + continue; + if (!tgt->rport) + continue; + fpin = ibmvfc_basic_fpin_to_desc(crq, tgt->wwpn); + if (fpin) { + fc_host_fpin_rcv(tgt->vhost->host, + sizeof(*fpin) + be32_to_cpu(fpin->desc_len), + (char *)fpin, 0); + kfree(fpin); + } else + dev_err_ratelimited(vhost->dev, + "FPIN event %u received, unable to process\n", + crq->fpin_status); + } + + end: + crq->valid = 0;
I've been mulling this over since last week and I'm inclined to agree with the sashiko analysis as well that clearing this asynchrously is not ideal. I think it's better to make a copy of the crq entry and pass the copy instead of a pointer to the data in our actual ring buffer. -Tyrel