* [PATCH v9 1/9] scsi: ibmvfc: add basic FPIN support
2026-09-11 2:19 [PATCH v9 0/9] scsi: ibmvfc: make ibmvfc support FPIN messages Tyrel Datwyler
@ 2026-09-11 2:19 ` Tyrel Datwyler
2026-09-11 2:38 ` sashiko-bot
2026-09-11 2:19 ` [PATCH v9 2/9] scsi: ibmvfc: add NOOP command support Tyrel Datwyler
` (7 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Tyrel Datwyler @ 2026-09-11 2:19 UTC (permalink / raw)
To: james.bottomley, martin.petersen
Cc: linux-scsi, linuxppc-dev, linux-kernel, brking, davemarq,
Tyrel Datwyler
From: Dave Marquardt <davemarq@linux.ibm.com>
Implement support for a basic level of Fabric Performance Impact
Notifications (FPIN) in the ibmvfc driver to enable monitoring of
fabric congestion and link integrity events.
Add async event handler for IBMVFC_AE_FPIN events that offloads FPIN
processing to a dedicated workqueue. Convert VIOS FPIN messages to
standard fc_els_fpin structures and pass them to fc_host_fpin_rcv() for
processing by the FC transport layer.
Introduce common FPIN conversion routines that will be reused for full
and extended FPIN support in subsequent patches. Add KUnit test
infrastructure to validate FPIN event handling and statistics updates.
Add ibmvfc_handle_async() support for IBMVFC_AE_FPIN events, a dedicated
workqueue for FPIN processing, FPIN message conversion to fc_els_fpin
format, handling of link congestion, port congestion, port cleared, port
degraded, and congestion cleared events, and a KUnit test module for
FPIN functionality.
Signed-off-by: Dave Marquardt <davemarq@linux.ibm.com>
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/Kconfig | 10 +
drivers/scsi/ibmvscsi/Makefile | 1 +
drivers/scsi/ibmvscsi/ibmvfc-core.c | 295 ++++++++++++++++++-
drivers/scsi/ibmvscsi/ibmvfc.h | 17 ++
drivers/scsi/ibmvscsi/ibmvfc_kunit.c | 408 +++++++++++++++++++++++++++
5 files changed, 728 insertions(+), 3 deletions(-)
create mode 100644 drivers/scsi/ibmvscsi/ibmvfc_kunit.c
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 1eec66195cf4..320c37c4ab36 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -760,6 +760,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 9408c7f4cdee..a227bc633f3a 100644
--- a/drivers/scsi/ibmvscsi/Makefile
+++ b/drivers/scsi/ibmvscsi/Makefile
@@ -3,3 +3,4 @@ ibmvfc-objs := ibmvfc-core.o ibmvfc-nvme.o
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-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
index aa7ae81df41b..5259a80958ce 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
@@ -31,6 +31,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;
@@ -1433,6 +1436,7 @@ void ibmvfc_release_tgt(struct kref *kref)
struct ibmvfc_target *tgt = container_of(kref, struct ibmvfc_target, kref);
mempool_free(tgt, tgt->vhost->tgt_pool);
}
+EXPORT_SYMBOL_IF_KUNIT(ibmvfc_release_tgt);
/**
* ibmvfc_get_starget_node_name - Get SCSI target's node name
@@ -3213,6 +3217,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 = {
@@ -3261,16 +3266,259 @@ 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 type, __be16 modifier,
+ __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)
+ return NULL;
+
+ fpin = kzalloc(size, GFP_KERNEL);
+ if (!fpin)
+ 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));
+ cdesc->event_type = type;
+ cdesc->event_modifier = modifier;
+ cdesc->event_period = cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_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(struct_size_t(struct fc_fn_peer_congn_desc,
+ pname_list, 1) - FC_TLV_DESC_HDR_SZ);
+ pdesc->event_type = type;
+ pdesc->event_modifier = modifier;
+ pdesc->event_period = cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_PERIOD);
+ 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(struct_size_t(struct fc_fn_li_desc,
+ pname_list, 1) - FC_TLV_DESC_HDR_SZ);
+ ldesc->event_type = type;
+ ldesc->event_modifier = modifier;
+ ldesc->event_threshold = threshold;
+ ldesc->event_count = event_count;
+ 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)
+{
+ __be16 type;
+
+ switch (crq->fpin_status) {
+ case IBMVFC_AE_FPIN_LINK_CONGESTED:
+ case IBMVFC_AE_FPIN_PORT_CONGESTED:
+ type = cpu_to_be16(FPIN_CONGN_DEVICE_SPEC);
+ break;
+ case IBMVFC_AE_FPIN_PORT_CLEARED:
+ case IBMVFC_AE_FPIN_CONGESTION_CLEARED:
+ type = cpu_to_be16(FPIN_CONGN_CLEAR);
+ break;
+ case IBMVFC_AE_FPIN_PORT_DEGRADED:
+ type = cpu_to_be16(FPIN_LI_UNKNOWN);
+ break;
+ default:
+ return NULL;
+ }
+
+ return ibmvfc_common_fpin_to_desc(crq->fpin_status, cpu_to_be64(wwpn),
+ type, cpu_to_be16(0),
+ cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_THRESHOLD),
+ cpu_to_be32(1));
+}
+
+/**
+ * ibmvfc_find_target - Search for a target in a target list
+ * @target_list: list head of targets to search
+ * @scsi_id: SCSI ID to match (0 to skip this check)
+ * @wwpn: WWPN to match (0 to skip this check)
+ * @node_name: Node name to match (0 to skip this check)
+ *
+ * Returns:
+ * Pointer to matching target, or NULL if not found
+ **/
+static struct ibmvfc_target *ibmvfc_find_target(struct list_head *target_list,
+ __be64 scsi_id, __be64 wwpn,
+ __be64 node_name)
+{
+ struct ibmvfc_target *tgt;
+
+ list_for_each_entry(tgt, target_list, queue) {
+ if (scsi_id && cpu_to_be64(tgt->scsi_id) != scsi_id)
+ continue;
+ if (wwpn && cpu_to_be64(tgt->ids.port_name) != wwpn)
+ continue;
+ if (node_name && cpu_to_be64(tgt->ids.node_name) != node_name)
+ continue;
+ if (!tgt->rport || !tgt->nvme_remote_port)
+ continue;
+ return tgt;
+ }
+
+ return NULL;
+}
+
+/**
+ * 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;
+ unsigned long flags;
+
+ aw = container_of_const(work, struct ibmvfc_async_work, async_work_s);
+ vhost = aw->vhost;
+ crq = &aw->crq;
+
+ if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
+ goto free;
+
+ spin_lock_irqsave(vhost->host->host_lock, flags);
+ tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, crq->scsi_id,
+ crq->wwpn, crq->node_name);
+ if (!tgt) {
+ /* Target not found in scsi_scrqs, search nvme_scrqs */
+ tgt = ibmvfc_find_target(&vhost->nvme_scrqs.targets,
+ crq->scsi_id, crq->wwpn,
+ crq->node_name);
+ }
+
+ if (tgt) {
+ kref_get(&tgt->kref);
+ spin_unlock_irqrestore(vhost->host->host_lock, flags);
+ } else {
+ spin_unlock_irqrestore(vhost->host->host_lock, flags);
+ dev_err_ratelimited(vhost->dev, "Invalid target for FPIN\n");
+ goto free;
+ }
+
+ 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 received, unable to process\n");
+
+ kref_put(&tgt->kref, ibmvfc_release_tgt);
+ free:
+ kfree(aw);
+}
+
/**
* ibmvfc_handle_async - Handle an async event from the adapter
* @crq: crq to process
* @vhost: ibmvfc host struct
*
**/
-static void ibmvfc_handle_async(struct ibmvfc_async_crq *crq,
- struct ibmvfc_host *vhost)
+VISIBLE_IF_KUNIT void ibmvfc_handle_async(struct ibmvfc_async_crq *crq,
+ struct ibmvfc_host *vhost)
{
const struct ibmvfc_async_desc *desc = ibmvfc_get_ae_desc(be64_to_cpu(crq->event));
+ struct ibmvfc_async_work *aw;
struct ibmvfc_target *tgt;
ibmvfc_log(vhost, desc->log_level, "%s event received. scsi_id: %llx, wwpn: %llx,"
@@ -3361,11 +3609,25 @@ static void ibmvfc_handle_async(struct ibmvfc_async_crq *crq,
case IBMVFC_AE_HALT:
ibmvfc_link_down(vhost, IBMVFC_HALTED);
break;
+ case IBMVFC_AE_FPIN:
+ if (vhost->state == IBMVFC_HOST_OFFLINE)
+ break;
+ aw = kzalloc(sizeof(struct ibmvfc_async_work), GFP_ATOMIC);
+ if (aw) {
+ INIT_WORK(&aw->async_work_s, ibmvfc_process_async_work);
+ aw->vhost = vhost;
+ aw->crq = *crq;
+ queue_work(vhost->fpin_workq, &aw->async_work_s);
+ } else
+ dev_err_ratelimited(vhost->dev,
+ "can't offload async CRQ to work queue\n");
+ break;
default:
dev_err(vhost->dev, "Unknown async event received: %lld\n", crq->event);
break;
}
}
+EXPORT_SYMBOL_IF_KUNIT(ibmvfc_handle_async);
/**
* ibmvfc_handle_crq - Handles and frees received events in the CRQ
@@ -6875,9 +7137,15 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id)
INIT_WORK(&vhost->rport_add_work_q, ibmvfc_rport_add_thread);
mutex_init(&vhost->passthru_mutex);
- if ((rc = ibmvfc_alloc_mem(vhost)))
+ vhost->fpin_workq = alloc_workqueue("%s-fpin-workq-%u", WQ_UNBOUND, 0,
+ IBMVFC_NAME, shost->host_no);
+ if (vhost->fpin_workq == NULL)
goto free_scsi_host;
+ rc = ibmvfc_alloc_mem(vhost);
+ if (rc)
+ goto free_workq;
+
vhost->work_thread = kthread_run(ibmvfc_work, vhost, "%s_%d", IBMVFC_NAME,
shost->host_no);
@@ -6923,6 +7191,9 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id)
kthread_stop(vhost->work_thread);
free_host_mem:
ibmvfc_free_mem(vhost);
+free_workq:
+ destroy_workqueue(vhost->fpin_workq);
+ vhost->fpin_workq = NULL;
free_scsi_host:
scsi_host_put(shost);
out:
@@ -6953,6 +7224,8 @@ static void ibmvfc_remove(struct vio_dev *vdev)
ibmvfc_wait_while_resetting(vhost);
kthread_stop(vhost->work_thread);
flush_work(&vhost->rport_add_work_q);
+ destroy_workqueue(vhost->fpin_workq);
+ vhost->fpin_workq = NULL;
fc_remove_host(vhost->host);
scsi_remove_host(vhost->host);
@@ -7115,5 +7388,21 @@ static void __exit ibmvfc_module_exit(void)
fc_release_transport(ibmvfc_transport_template);
}
+#if IS_ENABLED(CONFIG_KUNIT)
+VISIBLE_IF_KUNIT struct ibmvfc_host *ibmvfc_get_first_vhost(void)
+{
+ struct ibmvfc_host *vhost = NULL;
+
+ spin_lock(&ibmvfc_driver_lock);
+ if (!list_empty(&ibmvfc_head))
+ vhost = list_first_entry(&ibmvfc_head, struct ibmvfc_host, queue);
+ if (vhost)
+ scsi_host_get(vhost->host);
+ spin_unlock(&ibmvfc_driver_lock);
+ return vhost;
+}
+EXPORT_SYMBOL_IF_KUNIT(ibmvfc_get_first_vhost);
+#endif
+
module_init(ibmvfc_module_init);
module_exit(ibmvfc_module_exit);
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
index ca80ceffe53a..d7a30e7a4a28 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.h
+++ b/drivers/scsi/ibmvscsi/ibmvfc.h
@@ -750,8 +750,12 @@ enum ibmvfc_ae_fpin_status {
IBMVFC_AE_FPIN_PORT_CONGESTED = 0x2,
IBMVFC_AE_FPIN_PORT_CLEARED = 0x3,
IBMVFC_AE_FPIN_PORT_DEGRADED = 0x4,
+ IBMVFC_AE_FPIN_CONGESTION_CLEARED = 0x5,
};
+#define IBMVFC_FPIN_DEFAULT_EVENT_PERIOD (5*60*MSEC_PER_SEC) /* 5 minutes */
+#define IBMVFC_FPIN_DEFAULT_EVENT_THRESHOLD (5*60*MSEC_PER_SEC/2) /* 2.5 minutes */
+
struct ibmvfc_async_crq {
volatile u8 valid;
u8 link_state;
@@ -781,6 +785,12 @@ struct ibmvfc_async_sub_crq {
} id;
} __packed __aligned(8);
+struct ibmvfc_async_work {
+ struct ibmvfc_host *vhost;
+ struct ibmvfc_async_crq crq;
+ struct work_struct async_work_s;
+};
+
union ibmvfc_iu {
struct ibmvfc_mad_common mad_common;
struct ibmvfc_npiv_login_mad npiv_login;
@@ -1022,6 +1032,7 @@ struct ibmvfc_host {
wait_queue_head_t work_wait_q;
struct nvme_fc_local_port *nvme_local_port;
struct completion nvme_delete_done;
+ struct workqueue_struct *fpin_workq;
};
struct ibmvfc_event *__ibmvfc_get_event(struct ibmvfc_queue *queue, int reserved);
@@ -1088,4 +1099,10 @@ static inline struct ibmvfc_host *ibmvfc_channels_to_vhost(struct ibmvfc_channel
#define ibmvfc_remove_trace_file(kobj, attr) do { } while (0)
#endif
+#if IS_ENABLED(CONFIG_KUNIT)
+#include <kunit/visibility.h>
+VISIBLE_IF_KUNIT void ibmvfc_handle_async(struct ibmvfc_async_crq *crq, struct ibmvfc_host *vhost);
+VISIBLE_IF_KUNIT struct ibmvfc_host *ibmvfc_get_first_vhost(void);
+#endif
+
#endif
diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
new file mode 100644
index 000000000000..80e7f8e0fd70
--- /dev/null
+++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
@@ -0,0 +1,408 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include <kunit/test.h>
+#include <kunit/visibility.h>
+#include <scsi/scsi_device.h>
+#include <scsi/scsi_transport_fc.h>
+#include <scsi/fc/fc_els.h>
+#include <linux/list.h>
+#include <linux/delay.h>
+#include "ibmvfc.h"
+
+MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING");
+
+/**
+ * ibmvfc_async_fpin_event_test - unit test for IBMVFC_AE_FPIN parts of
+ * ibmvfc_handle_async
+ * @test: pointer to kunit structure
+ *
+ * Tests
+ * - error returns from ibmvfc_handle_async
+ * - statistics updates
+ *
+ * Return: void
+ */
+static void ibmvfc_async_fpin_test(struct kunit *test)
+{
+ u64 post[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1];
+ u64 pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1];
+ struct ibmvfc_async_crq_event ae[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1] = {
+ [0 ... IBMVFC_AE_FPIN_CONGESTION_CLEARED] = { .type = IBMVFC_ASYNC_CRQ_MAIN },
+ };
+ enum ibmvfc_ae_fpin_status fs;
+ struct fc_host_attrs *fc_host;
+ struct ibmvfc_target *tgt;
+ struct ibmvfc_host *vhost;
+ struct fc_rport *rport;
+ unsigned long flags;
+
+ vhost = ibmvfc_get_first_vhost();
+ if (!vhost)
+ kunit_skip(test, "No ibmvfc devices available");
+
+ spin_lock_irqsave(vhost->host->host_lock, flags);
+ if (vhost->scsi_scrqs.num_targets < 1) {
+ spin_unlock_irqrestore(vhost->host->host_lock, flags);
+ scsi_host_put(vhost->host);
+ kunit_skip(test, "No targets");
+ }
+ tgt = list_first_entry(&vhost->scsi_scrqs.targets, struct ibmvfc_target, queue);
+ if (!tgt->rport) {
+ spin_unlock_irqrestore(vhost->host->host_lock, flags);
+ scsi_host_put(vhost->host);
+ kunit_skip(test, "No rport");
+ }
+ rport = tgt->rport;
+ get_device(&rport->dev);
+ kref_get(&tgt->kref);
+ spin_unlock_irqrestore(vhost->host->host_lock, flags);
+
+ fc_host = shost_to_fc_host(vhost->host);
+
+ pre[IBMVFC_AE_FPIN_LINK_CONGESTED] = READ_ONCE(fc_host->fpin_stats.cn_device_specific);
+ pre[IBMVFC_AE_FPIN_PORT_CONGESTED] = READ_ONCE(rport->fpin_stats.cn_device_specific);
+ pre[IBMVFC_AE_FPIN_PORT_CLEARED] = READ_ONCE(rport->fpin_stats.cn_clear);
+ pre[IBMVFC_AE_FPIN_PORT_DEGRADED] = READ_ONCE(rport->fpin_stats.li_failure_unknown);
+ pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED] = READ_ONCE(fc_host->fpin_stats.cn_clear);
+
+ for (fs = IBMVFC_AE_FPIN_LINK_CONGESTED; fs <= IBMVFC_AE_FPIN_CONGESTION_CLEARED; fs++) {
+ ae[fs].async_crq.valid = 0x80;
+ ae[fs].async_crq.link_state = IBMVFC_AE_LS_LINK_UP;
+ ae[fs].async_crq.fpin_status = fs;
+ ae[fs].async_crq.event = cpu_to_be64(IBMVFC_AE_FPIN);
+ ae[fs].async_crq.scsi_id = cpu_to_be64(tgt->scsi_id);
+ ae[fs].async_crq.wwpn = cpu_to_be64(tgt->wwpn);
+ ae[fs].async_crq.node_name = cpu_to_be64(tgt->ids.node_name);
+ ibmvfc_handle_async(&ae[fs], vhost);
+ ae[fs].async_crq.valid = 0;
+ wmb(); /* ensure valid bit clear is visible before checking stats */
+ }
+ flush_workqueue(vhost->fpin_workq);
+
+ post[IBMVFC_AE_FPIN_LINK_CONGESTED] = READ_ONCE(fc_host->fpin_stats.cn_device_specific);
+ post[IBMVFC_AE_FPIN_PORT_CONGESTED] = READ_ONCE(rport->fpin_stats.cn_device_specific);
+ post[IBMVFC_AE_FPIN_PORT_CLEARED] = READ_ONCE(rport->fpin_stats.cn_clear);
+ post[IBMVFC_AE_FPIN_PORT_DEGRADED] = READ_ONCE(rport->fpin_stats.li_failure_unknown);
+ post[IBMVFC_AE_FPIN_CONGESTION_CLEARED] = READ_ONCE(fc_host->fpin_stats.cn_clear);
+
+ KUNIT_EXPECT_GE(test, post[IBMVFC_AE_FPIN_LINK_CONGESTED],
+ pre[IBMVFC_AE_FPIN_LINK_CONGESTED]+1);
+ KUNIT_EXPECT_GE(test, post[IBMVFC_AE_FPIN_PORT_CONGESTED],
+ pre[IBMVFC_AE_FPIN_PORT_CONGESTED]+1);
+ KUNIT_EXPECT_GE(test, post[IBMVFC_AE_FPIN_PORT_CLEARED],
+ pre[IBMVFC_AE_FPIN_PORT_CLEARED]+1);
+ KUNIT_EXPECT_GE(test, post[IBMVFC_AE_FPIN_PORT_DEGRADED],
+ pre[IBMVFC_AE_FPIN_PORT_DEGRADED]+1);
+ KUNIT_EXPECT_GE(test, post[IBMVFC_AE_FPIN_CONGESTION_CLEARED],
+ pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED]+1);
+
+ /* bad path */
+ pre[IBMVFC_AE_FPIN_LINK_CONGESTED] = READ_ONCE(fc_host->fpin_stats.cn_device_specific);
+ pre[IBMVFC_AE_FPIN_PORT_CONGESTED] = READ_ONCE(rport->fpin_stats.cn_device_specific);
+ pre[IBMVFC_AE_FPIN_PORT_CLEARED] = READ_ONCE(rport->fpin_stats.cn_clear);
+ pre[IBMVFC_AE_FPIN_PORT_DEGRADED] = READ_ONCE(rport->fpin_stats.li_failure_unknown);
+ pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED] = READ_ONCE(fc_host->fpin_stats.cn_clear);
+
+ ae[0].async_crq.valid = 0x80;
+ ae[0].async_crq.link_state = IBMVFC_AE_LS_LINK_UP;
+ ae[0].async_crq.fpin_status = 0; /* bad value */
+ ae[0].async_crq.event = cpu_to_be64(IBMVFC_AE_FPIN);
+ ae[0].async_crq.scsi_id = cpu_to_be64(tgt->scsi_id);
+ ae[0].async_crq.wwpn = cpu_to_be64(tgt->wwpn);
+ ae[0].async_crq.node_name = cpu_to_be64(tgt->ids.node_name);
+ ibmvfc_handle_async(&ae[0], vhost);
+ ae[0].async_crq.valid = 0;
+ wmb(); /* ensure valid bit clear is visible before checking stats */
+ flush_workqueue(vhost->fpin_workq);
+
+ post[IBMVFC_AE_FPIN_LINK_CONGESTED] = READ_ONCE(fc_host->fpin_stats.cn_device_specific);
+ post[IBMVFC_AE_FPIN_PORT_CONGESTED] = READ_ONCE(rport->fpin_stats.cn_device_specific);
+ post[IBMVFC_AE_FPIN_PORT_CLEARED] = READ_ONCE(rport->fpin_stats.cn_clear);
+ post[IBMVFC_AE_FPIN_PORT_DEGRADED] = READ_ONCE(rport->fpin_stats.li_failure_unknown);
+ post[IBMVFC_AE_FPIN_CONGESTION_CLEARED] = READ_ONCE(fc_host->fpin_stats.cn_clear);
+
+ KUNIT_EXPECT_EQ(test, pre[IBMVFC_AE_FPIN_LINK_CONGESTED],
+ post[IBMVFC_AE_FPIN_LINK_CONGESTED]);
+ KUNIT_EXPECT_EQ(test, pre[IBMVFC_AE_FPIN_PORT_CONGESTED],
+ post[IBMVFC_AE_FPIN_PORT_CONGESTED]);
+ KUNIT_EXPECT_EQ(test, pre[IBMVFC_AE_FPIN_PORT_CLEARED],
+ post[IBMVFC_AE_FPIN_PORT_CLEARED]);
+ KUNIT_EXPECT_EQ(test, pre[IBMVFC_AE_FPIN_PORT_DEGRADED],
+ post[IBMVFC_AE_FPIN_PORT_DEGRADED]);
+ KUNIT_EXPECT_EQ(test, pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED],
+ post[IBMVFC_AE_FPIN_CONGESTION_CLEARED]);
+
+ kref_put(&tgt->kref, ibmvfc_release_tgt);
+}
+
+/**
+ * ibmvfc_full_fpin_test - unit test for IBMVFC_AE_FPIN parts of ibmvfc_handle_async
+ * @test: pointer to kunit structure
+ *
+ * Tests
+ * - error returns from ibmvfc_handle_async
+ * - statistics updates
+ *
+ * Return: void
+ */
+static void ibmvfc_full_fpin_test(struct kunit *test)
+{
+ u64 post[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1];
+ u64 pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1];
+ struct ibmvfc_async_crq_event ae[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1] = {
+ [0 ... IBMVFC_AE_FPIN_CONGESTION_CLEARED] = { .type = IBMVFC_ASYNC_CRQ_SUB },
+ };
+ enum ibmvfc_ae_fpin_status fs;
+ struct fc_host_attrs *fc_host;
+ struct ibmvfc_target *tgt;
+ struct ibmvfc_host *vhost;
+ struct fc_rport *rport;
+ unsigned long flags;
+
+ vhost = ibmvfc_get_first_vhost();
+ if (!vhost)
+ kunit_skip(test, "No ibmvfc devices available");
+
+ spin_lock_irqsave(vhost->host->host_lock, flags);
+ if (vhost->scsi_scrqs.num_targets < 1) {
+ spin_unlock_irqrestore(vhost->host->host_lock, flags);
+ scsi_host_put(vhost->host);
+ kunit_skip(test, "No targets");
+ }
+ tgt = list_first_entry(&vhost->scsi_scrqs.targets, struct ibmvfc_target, queue);
+ if (!tgt->rport) {
+ spin_unlock_irqrestore(vhost->host->host_lock, flags);
+ scsi_host_put(vhost->host);
+ kunit_skip(test, "No rport");
+ }
+ rport = tgt->rport;
+ get_device(&rport->dev);
+ kref_get(&tgt->kref);
+ spin_unlock_irqrestore(vhost->host->host_lock, flags);
+
+ fc_host = shost_to_fc_host(vhost->host);
+
+ pre[IBMVFC_AE_FPIN_LINK_CONGESTED] = READ_ONCE(fc_host->fpin_stats.cn_device_specific);
+ pre[IBMVFC_AE_FPIN_PORT_CONGESTED] = READ_ONCE(rport->fpin_stats.cn_device_specific);
+ pre[IBMVFC_AE_FPIN_PORT_CLEARED] = READ_ONCE(rport->fpin_stats.cn_clear);
+ pre[IBMVFC_AE_FPIN_PORT_DEGRADED] = READ_ONCE(rport->fpin_stats.li_failure_unknown);
+ pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED] = READ_ONCE(fc_host->fpin_stats.cn_clear);
+
+ for (fs = IBMVFC_AE_FPIN_LINK_CONGESTED; fs <= IBMVFC_AE_FPIN_CONGESTION_CLEARED; fs++) {
+ ae[fs].subq.valid = 0x80;
+ ae[fs].subq.link_state = IBMVFC_AE_LS_LINK_UP;
+ ae[fs].subq.fpin_status = fs;
+ ae[fs].subq.event = cpu_to_be16(IBMVFC_AE_FPIN);
+ ae[fs].subq.wwpn = cpu_to_be64(tgt->wwpn);
+ ae[fs].subq.id.node_name = cpu_to_be64(tgt->ids.node_name);
+ ibmvfc_handle_async(&ae[fs], vhost);
+ ae[fs].subq.valid = 0;
+ wmb(); /* ensure valid bit clear is visible before checking stats */
+ }
+ flush_workqueue(vhost->fpin_workq);
+
+ post[IBMVFC_AE_FPIN_LINK_CONGESTED] = READ_ONCE(fc_host->fpin_stats.cn_device_specific);
+ post[IBMVFC_AE_FPIN_PORT_CONGESTED] = READ_ONCE(rport->fpin_stats.cn_device_specific);
+ post[IBMVFC_AE_FPIN_PORT_CLEARED] = READ_ONCE(rport->fpin_stats.cn_clear);
+ post[IBMVFC_AE_FPIN_PORT_DEGRADED] = READ_ONCE(rport->fpin_stats.li_failure_unknown);
+ post[IBMVFC_AE_FPIN_CONGESTION_CLEARED] = READ_ONCE(fc_host->fpin_stats.cn_clear);
+
+ KUNIT_EXPECT_GE(test, post[IBMVFC_AE_FPIN_LINK_CONGESTED],
+ pre[IBMVFC_AE_FPIN_LINK_CONGESTED]+1);
+ KUNIT_EXPECT_GE(test, post[IBMVFC_AE_FPIN_PORT_CONGESTED],
+ pre[IBMVFC_AE_FPIN_PORT_CONGESTED]+1);
+ KUNIT_EXPECT_GE(test, post[IBMVFC_AE_FPIN_PORT_CLEARED],
+ pre[IBMVFC_AE_FPIN_PORT_CLEARED]+1);
+ KUNIT_EXPECT_GE(test, post[IBMVFC_AE_FPIN_PORT_DEGRADED],
+ pre[IBMVFC_AE_FPIN_PORT_DEGRADED]+1);
+ KUNIT_EXPECT_GE(test, post[IBMVFC_AE_FPIN_CONGESTION_CLEARED],
+ pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED]+1);
+
+ /* bad path */
+ pre[IBMVFC_AE_FPIN_LINK_CONGESTED] = READ_ONCE(fc_host->fpin_stats.cn_device_specific);
+ pre[IBMVFC_AE_FPIN_PORT_CONGESTED] = READ_ONCE(rport->fpin_stats.cn_device_specific);
+ pre[IBMVFC_AE_FPIN_PORT_CLEARED] = READ_ONCE(rport->fpin_stats.cn_clear);
+ pre[IBMVFC_AE_FPIN_PORT_DEGRADED] = READ_ONCE(rport->fpin_stats.li_failure_unknown);
+ pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED] = READ_ONCE(fc_host->fpin_stats.cn_clear);
+
+ ae[0].subq.valid = 0x80;
+ ae[0].subq.link_state = IBMVFC_AE_LS_LINK_UP;
+ ae[0].subq.fpin_status = 0; /* bad value */
+ ae[0].subq.event = cpu_to_be16(IBMVFC_AE_FPIN);
+ ae[0].subq.wwpn = cpu_to_be64(tgt->wwpn);
+ ae[0].subq.id.node_name = cpu_to_be64(tgt->ids.node_name);
+ ibmvfc_handle_async(&ae[0], vhost);
+ ae[0].subq.valid = 0;
+ wmb(); /* ensure valid bit clear is visible before checking stats */
+ flush_workqueue(vhost->fpin_workq);
+
+ post[IBMVFC_AE_FPIN_LINK_CONGESTED] = READ_ONCE(fc_host->fpin_stats.cn_device_specific);
+ post[IBMVFC_AE_FPIN_PORT_CONGESTED] = READ_ONCE(rport->fpin_stats.cn_device_specific);
+ post[IBMVFC_AE_FPIN_PORT_CLEARED] = READ_ONCE(rport->fpin_stats.cn_clear);
+ post[IBMVFC_AE_FPIN_PORT_DEGRADED] = READ_ONCE(rport->fpin_stats.li_failure_unknown);
+ post[IBMVFC_AE_FPIN_CONGESTION_CLEARED] = READ_ONCE(fc_host->fpin_stats.cn_clear);
+
+ KUNIT_EXPECT_EQ(test, pre[IBMVFC_AE_FPIN_LINK_CONGESTED],
+ post[IBMVFC_AE_FPIN_LINK_CONGESTED]);
+ KUNIT_EXPECT_EQ(test, pre[IBMVFC_AE_FPIN_PORT_CONGESTED],
+ post[IBMVFC_AE_FPIN_PORT_CONGESTED]);
+ KUNIT_EXPECT_EQ(test, pre[IBMVFC_AE_FPIN_PORT_CLEARED],
+ post[IBMVFC_AE_FPIN_PORT_CLEARED]);
+ KUNIT_EXPECT_EQ(test, pre[IBMVFC_AE_FPIN_PORT_DEGRADED],
+ post[IBMVFC_AE_FPIN_PORT_DEGRADED]);
+ KUNIT_EXPECT_EQ(test, pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED],
+ post[IBMVFC_AE_FPIN_CONGESTION_CLEARED]);
+
+ put_device(&rport->dev);
+ kref_put(&tgt->kref, ibmvfc_release_tgt);
+ scsi_host_put(vhost->host);
+}
+
+#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); \
+}
+
+/**
+ * ibmvfc_extended_fpin_test - unit test for extended FPIN events
+ * @test: pointer to kunit structure
+ *
+ * Note: This test exercises extended FPIN code paths but does not check
+ * that statistics are correctly updated.
+ *
+ * Return: void
+ */
+static void ibmvfc_extended_fpin_test(struct kunit *test)
+{
+ enum ibmvfc_ae_fpin_status fs;
+ struct ibmvfc_async_subq_fpin crq[IBMVFC_AE_FPIN_CONGESTION_CLEARED+1] = {};
+ struct ibmvfc_async_subq_fpin
+ crqcn[IBMVFC_AE_FPIN_PORT_CONGESTED][FPIN_CONGN_DEVICE_SPEC+1] = {};
+ struct ibmvfc_async_subq_fpin crqportdg[FPIN_LI_DEVICE_SPEC+1] = {};
+ struct ibmvfc_target *tgt;
+ struct ibmvfc_host *vhost;
+ struct fc_rport *rport;
+ LIST_HEAD(evt_doneq);
+ unsigned long flags;
+ u64 pre, post;
+
+ vhost = ibmvfc_get_first_vhost();
+ if (!vhost)
+ kunit_skip(test, "No ibmvfc devices available");
+
+ spin_lock_irqsave(vhost->host->host_lock, flags);
+ if (vhost->scsi_scrqs.num_targets < 1) {
+ spin_unlock_irqrestore(vhost->host->host_lock, flags);
+ scsi_host_put(vhost->host);
+ kunit_skip(test, "No targets");
+ }
+ tgt = list_first_entry(&vhost->scsi_scrqs.targets, struct ibmvfc_target, queue);
+ if (!tgt->rport) {
+ spin_unlock_irqrestore(vhost->host->host_lock, flags);
+ scsi_host_put(vhost->host);
+ kunit_skip(test, "No rport");
+ }
+ rport = tgt->rport;
+ get_device(&rport->dev);
+ kref_get(&tgt->kref);
+ spin_unlock_irqrestore(vhost->host->host_lock, flags);
+
+ for (fs = IBMVFC_AE_FPIN_LINK_CONGESTED; fs <= IBMVFC_AE_FPIN_CONGESTION_CLEARED; fs++) {
+ switch (fs) {
+ case IBMVFC_AE_FPIN_PORT_CLEARED:
+ case IBMVFC_AE_FPIN_CONGESTION_CLEARED: {
+ struct ibmvfc_async_crq_event ae = { .type = IBMVFC_ASYNC_CRQ_SUB };
+
+ crq[fs].valid = 0x80;
+ crq[fs].flags = IBMVFC_ASYNC_IS_FPIN_EXT;
+ crq[fs].link_state = IBMVFC_AE_LS_LINK_UP;
+ crq[fs].fpin_status = fs;
+ crq[fs].event = cpu_to_be16(IBMVFC_AE_FPIN);
+ crq[fs].wwpn = cpu_to_be64(tgt->wwpn);
+ crq[fs].fpin_data.flags = IBMVFC_FPIN_EVENT_TYPE_VALID;
+ crq[fs].fpin_data.event_type = cpu_to_be16(FPIN_CONGN_CLEAR);
+ ae.subq = *(struct ibmvfc_async_sub_crq *)&crq[fs];
+ pre = READ_ONCE(rport->fpin_stats.cn_clear);
+ ibmvfc_handle_async(&ae, vhost);
+ flush_workqueue(vhost->fpin_workq);
+ post = READ_ONCE(rport->fpin_stats.cn_clear);
+ break;
+ }
+ case IBMVFC_AE_FPIN_LINK_CONGESTED:
+ case IBMVFC_AE_FPIN_PORT_CONGESTED:
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_CONGN_CLEAR, cn_clear,
+ crqcn[fs-1][FPIN_CONGN_CLEAR]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_CONGN_LOST_CREDIT,
+ cn_lost_credit,
+ crqcn[fs-1][FPIN_CONGN_LOST_CREDIT]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_CONGN_CREDIT_STALL,
+ cn_credit_stall,
+ crqcn[fs-1][FPIN_CONGN_CREDIT_STALL]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_CONGN_OVERSUBSCRIPTION,
+ cn_oversubscription,
+ crqcn[fs-1][FPIN_CONGN_OVERSUBSCRIPTION]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_CONGN_DEVICE_SPEC,
+ cn_device_specific,
+ crqcn[fs-1][FPIN_CONGN_DEVICE_SPEC]);
+ break;
+ case IBMVFC_AE_FPIN_PORT_DEGRADED:
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_UNKNOWN,
+ li_failure_unknown,
+ crqportdg[FPIN_LI_UNKNOWN]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_LINK_FAILURE,
+ li_link_failure_count,
+ crqportdg[FPIN_LI_LINK_FAILURE]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_LOSS_OF_SYNC,
+ li_loss_of_sync_count,
+ crqportdg[FPIN_LI_LOSS_OF_SYNC]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_LOSS_OF_SIG,
+ li_loss_of_signals_count,
+ crqportdg[FPIN_LI_LOSS_OF_SIG]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_PRIM_SEQ_ERR,
+ li_prim_seq_err_count,
+ crqportdg[FPIN_LI_PRIM_SEQ_ERR]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_INVALID_TX_WD,
+ li_invalid_tx_word_count,
+ crqportdg[FPIN_LI_INVALID_TX_WD]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_INVALID_CRC,
+ li_invalid_crc_count,
+ crqportdg[FPIN_LI_INVALID_CRC]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_DEVICE_SPEC,
+ li_device_specific,
+ crqportdg[FPIN_LI_DEVICE_SPEC]);
+ break;
+ }
+ }
+
+ put_device(&rport->dev);
+ kref_put(&tgt->kref, ibmvfc_release_tgt);
+ scsi_host_put(vhost->host);
+}
+
+static struct kunit_case ibmvfc_fpin_test_cases[] = {
+ KUNIT_CASE(ibmvfc_async_fpin_test),
+ KUNIT_CASE(ibmvfc_full_fpin_test),
+ KUNIT_CASE(ibmvfc_extended_fpin_test),
+ {},
+};
+
+static struct kunit_suite ibmvfc_fpin_test_suite = {
+ .name = "ibmvfc-fpin-test",
+ .test_cases = ibmvfc_fpin_test_cases,
+};
+kunit_test_init_section_suite(ibmvfc_fpin_test_suite);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Dave Marquardt <davemarq@linux.ibm.com>");
+MODULE_DESCRIPTION("Test module for IBM Virtual Fibre Channel Driver");
--
2.55.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v9 1/9] scsi: ibmvfc: add basic FPIN support
2026-09-11 2:19 ` [PATCH v9 1/9] scsi: ibmvfc: add basic FPIN support Tyrel Datwyler
@ 2026-09-11 2:38 ` sashiko-bot
2026-09-11 3:40 ` Tyrel Datwyler
0 siblings, 1 reply; 23+ messages in thread
From: sashiko-bot @ 2026-09-11 2:38 UTC (permalink / raw)
To: Tyrel Datwyler; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] ibmvfc_find_target incorrectly uses a logical OR condition, causing it to skip all valid targets and unconditionally return NULL.
- [Medium] Reference count leak of the SCSI host and remote port devices in the `ibmvfc_async_fpin_test` KUnit test.
- [Low] KUnit tests use undefined structures, macros, and incompatible function signatures from a future patch, breaking the build.
- [Low] Missing '&' operator when acquiring/releasing the host_lock, causing compilation to fail.
--
commit e20d55240604f534a630e6df915a24fb9e9b543b
Author: Dave Marquardt <davemarq@linux.ibm.com>
scsi: ibmvfc: add basic FPIN support
Implement support for a basic level of Fabric Performance Impact
Notifications (FPIN) in the ibmvfc driver to enable monitoring of
fabric congestion and link integrity events.
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index aa7ae81df41b1..5259a80958ce1 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -3411,6 +3450,25 @@ static struct fc_els_fpin *
> +static struct ibmvfc_target *ibmvfc_find_target(struct list_head *target_list,
> + __be64 scsi_id, __be64 wwpn,
> + __be64 node_name)
> +{
> + struct ibmvfc_target *tgt;
> +
> + list_for_each_entry(tgt, target_list, queue) {
> + if (scsi_id && cpu_to_be64(tgt->scsi_id) != scsi_id)
> + continue;
> + if (wwpn && cpu_to_be64(tgt->ids.port_name) != wwpn)
> + continue;
> + if (node_name && cpu_to_be64(tgt->ids.node_name) != node_name)
> + continue;
> + if (!tgt->rport || !tgt->nvme_remote_port)
> + continue;
[Severity: High]
Should this check use a logical AND (&&) instead of a logical OR (||)?
Since the target lists are separated into SCSI and NVMe, it seems every
target will have either rport or nvme_remote_port set to NULL. Using
a logical OR here evaluates to true for every target, causing the loop
to unconditionally skip all valid targets and return NULL.
> + return tgt;
> + }
> +
> + return NULL;
> +}
[ ... ]
> +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;
> + unsigned long flags;
> +
> + aw = container_of_const(work, struct ibmvfc_async_work, async_work_s);
> + vhost = aw->vhost;
> + crq = &aw->crq;
> +
> + if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
> + goto free;
> +
> + spin_lock_irqsave(vhost->host->host_lock, flags);
[Severity: Low]
Does this line cause a build failure due to a missing '&' operator?
In struct Scsi_Host, host_lock is an inline spinlock_t rather than a
pointer. Passing it by value to spin_lock_irqsave() causes the compiler
to fail with an incompatible type error.
Should this be written as:
spin_lock_irqsave(&vhost->host->host_lock, flags);
> + tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, crq->scsi_id,
> + crq->wwpn, crq->node_name);
[ ... ]
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> new file mode 100644
> index 0000000000000..80e7f8e0fd700
> --- /dev/null
> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> @@ -0,0 +1,408 @@
[ ... ]
> +static void ibmvfc_async_fpin_test(struct kunit *test)
> +{
> + u64 post[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1];
> + u64 pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1];
> + struct ibmvfc_async_crq_event ae[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1] = {
> + [0 ... IBMVFC_AE_FPIN_CONGESTION_CLEARED] = { .type = IBMVFC_ASYNC_CRQ_MAIN },
> + };
[Severity: Low]
Do these undefined types and macros break the build when compiling with
CONFIG_SCSI_IBMVFC_KUNIT_TEST=y?
struct ibmvfc_async_crq_event and IBMVFC_ASYNC_CRQ_MAIN are not defined
in this commit, which prevents the tests from compiling. It appears they
might have leaked from a subsequent patch in the series.
> + enum ibmvfc_ae_fpin_status fs;
> + struct fc_host_attrs *fc_host;
> + struct ibmvfc_target *tgt;
> + struct ibmvfc_host *vhost;
> + struct fc_rport *rport;
> + unsigned long flags;
> +
> + vhost = ibmvfc_get_first_vhost();
> + if (!vhost)
> + kunit_skip(test, "No ibmvfc devices available");
> +
> + spin_lock_irqsave(vhost->host->host_lock, flags);
[Severity: Low]
This also seems to be missing the '&' operator before vhost->host->host_lock,
resulting in the same build failure mentioned above.
> + if (vhost->scsi_scrqs.num_targets < 1) {
[ ... ]
> + ae[fs].async_crq.node_name = cpu_to_be64(tgt->ids.node_name);
> + ibmvfc_handle_async(&ae[fs], vhost);
[Severity: Low]
Is there a signature mismatch here?
ibmvfc_handle_async() is defined to take a struct ibmvfc_async_crq pointer
as its first argument, but the test passes a struct ibmvfc_async_crq_event
pointer instead.
> + ae[fs].async_crq.valid = 0;
> + wmb(); /* ensure valid bit clear is visible before checking stats */
> + }
[ ... ]
> + KUNIT_EXPECT_EQ(test, pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED],
> + post[IBMVFC_AE_FPIN_CONGESTION_CLEARED]);
> +
> + kref_put(&tgt->kref, ibmvfc_release_tgt);
> +}
[Severity: Medium]
Are we leaking device and host references in the successful exit path?
The test acquires references via ibmvfc_get_first_vhost() and
get_device(&rport->dev) during setup, but unlike the other tests in this
file (e.g., ibmvfc_full_fpin_test), it does not call put_device(&rport->dev)
and scsi_host_put(vhost->host) before returning. Leaking these references
can prevent module unloading and device cleanup.
> +
> +/**
> + * ibmvfc_full_fpin_test - unit test for IBMVFC_AE_FPIN parts of ibmvfc_handle_async
[ ... ]
> +static void ibmvfc_extended_fpin_test(struct kunit *test)
> +{
> + enum ibmvfc_ae_fpin_status fs;
> + struct ibmvfc_async_subq_fpin crq[IBMVFC_AE_FPIN_CONGESTION_CLEARED+1] = {};
[Severity: Low]
Does this undefined structure break the build?
struct ibmvfc_async_subq_fpin is not defined in this commit, causing a
compilation error.
> + struct ibmvfc_async_subq_fpin
> + crqcn[IBMVFC_AE_FPIN_PORT_CONGESTED][FPIN_CONGN_DEVICE_SPEC+1] = {};
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260911021910.1038128-1-tyreld@linux.ibm.com?part=1
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v9 1/9] scsi: ibmvfc: add basic FPIN support
2026-09-11 2:38 ` sashiko-bot
@ 2026-09-11 3:40 ` Tyrel Datwyler
0 siblings, 0 replies; 23+ messages in thread
From: Tyrel Datwyler @ 2026-09-11 3:40 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-scsi
On 9/10/26 7:38 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] ibmvfc_find_target incorrectly uses a logical OR condition, causing it to skip all valid targets and unconditionally return NULL.
> - [Medium] Reference count leak of the SCSI host and remote port devices in the `ibmvfc_async_fpin_test` KUnit test.
> - [Low] KUnit tests use undefined structures, macros, and incompatible function signatures from a future patch, breaking the build.
> - [Low] Missing '&' operator when acquiring/releasing the host_lock, causing compilation to fail.
> --
>
> commit e20d55240604f534a630e6df915a24fb9e9b543b
> Author: Dave Marquardt <davemarq@linux.ibm.com>
>
> scsi: ibmvfc: add basic FPIN support
>
> Implement support for a basic level of Fabric Performance Impact
> Notifications (FPIN) in the ibmvfc driver to enable monitoring of
> fabric congestion and link integrity events.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index aa7ae81df41b1..5259a80958ce1 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>
> [ ... ]
>
>> @@ -3411,6 +3450,25 @@ static struct fc_els_fpin *
>> +static struct ibmvfc_target *ibmvfc_find_target(struct list_head *target_list,
>> + __be64 scsi_id, __be64 wwpn,
>> + __be64 node_name)
>> +{
>> + struct ibmvfc_target *tgt;
>> +
>> + list_for_each_entry(tgt, target_list, queue) {
>> + if (scsi_id && cpu_to_be64(tgt->scsi_id) != scsi_id)
>> + continue;
>> + if (wwpn && cpu_to_be64(tgt->ids.port_name) != wwpn)
>> + continue;
>> + if (node_name && cpu_to_be64(tgt->ids.node_name) != node_name)
>> + continue;
>> + if (!tgt->rport || !tgt->nvme_remote_port)
>> + continue;
>
> [Severity: High]
> Should this check use a logical AND (&&) instead of a logical OR (||)?
Derp, yeah this check is meant to continue if there is no valid rport, and that
would mean both are NULL. Will fix in v10.
>
> Since the target lists are separated into SCSI and NVMe, it seems every
> target will have either rport or nvme_remote_port set to NULL. Using
> a logical OR here evaluates to true for every target, causing the loop
> to unconditionally skip all valid targets and return NULL.
>
>> + return tgt;
>> + }
>> +
>> + return NULL;
>> +}
>
> [ ... ]
>
>> +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;
>> + unsigned long flags;
>> +
>> + aw = container_of_const(work, struct ibmvfc_async_work, async_work_s);
>> + vhost = aw->vhost;
>> + crq = &aw->crq;
>> +
>> + if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
>> + goto free;
>> +
>> + spin_lock_irqsave(vhost->host->host_lock, flags);
>
> [Severity: Low]
> Does this line cause a build failure due to a missing '&' operator?
>
> In struct Scsi_Host, host_lock is an inline spinlock_t rather than a
> pointer. Passing it by value to spin_lock_irqsave() causes the compiler
> to fail with an incompatible type error.
>
> Should this be written as:
> spin_lock_irqsave(&vhost->host->host_lock, flags);
Yep, I took Dave's v8 as was and assumed it was build tested. I can confirm this
fails. Will fix in v10.
>
>> + tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, crq->scsi_id,
>> + crq->wwpn, crq->node_name);
>
> [ ... ]
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>> new file mode 100644
>> index 0000000000000..80e7f8e0fd700
>> --- /dev/null
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>> @@ -0,0 +1,408 @@
>
> [ ... ]
>
>> +static void ibmvfc_async_fpin_test(struct kunit *test)
>> +{
>> + u64 post[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1];
>> + u64 pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1];
>> + struct ibmvfc_async_crq_event ae[IBMVFC_AE_FPIN_CONGESTION_CLEARED + 1] = {
>> + [0 ... IBMVFC_AE_FPIN_CONGESTION_CLEARED] = { .type = IBMVFC_ASYNC_CRQ_MAIN },
>> + };
>
> [Severity: Low]
> Do these undefined types and macros break the build when compiling with
> CONFIG_SCSI_IBMVFC_KUNIT_TEST=y?
>
> struct ibmvfc_async_crq_event and IBMVFC_ASYNC_CRQ_MAIN are not defined
> in this commit, which prevents the tests from compiling. It appears they
> might have leaked from a subsequent patch in the series.
Confirmed that this does not build. Will break this apart into the proper
commits for v10.
>
>> + enum ibmvfc_ae_fpin_status fs;
>> + struct fc_host_attrs *fc_host;
>> + struct ibmvfc_target *tgt;
>> + struct ibmvfc_host *vhost;
>> + struct fc_rport *rport;
>> + unsigned long flags;
>> +
>> + vhost = ibmvfc_get_first_vhost();
>> + if (!vhost)
>> + kunit_skip(test, "No ibmvfc devices available");
>> +
>> + spin_lock_irqsave(vhost->host->host_lock, flags);
>
> [Severity: Low]
> This also seems to be missing the '&' operator before vhost->host->host_lock,
> resulting in the same build failure mentioned above.
Correct. Will fix in v10.
>
>> + if (vhost->scsi_scrqs.num_targets < 1) {
>
> [ ... ]
>
>> + ae[fs].async_crq.node_name = cpu_to_be64(tgt->ids.node_name);
>> + ibmvfc_handle_async(&ae[fs], vhost);
>
> [Severity: Low]
> Is there a signature mismatch here?
>
> ibmvfc_handle_async() is defined to take a struct ibmvfc_async_crq pointer
> as its first argument, but the test passes a struct ibmvfc_async_crq_event
> pointer instead.
>
>> + ae[fs].async_crq.valid = 0;
>> + wmb(); /* ensure valid bit clear is visible before checking stats */
>> + }
>
> [ ... ]
>
>> + KUNIT_EXPECT_EQ(test, pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED],
>> + post[IBMVFC_AE_FPIN_CONGESTION_CLEARED]);
>> +
>> + kref_put(&tgt->kref, ibmvfc_release_tgt);
>> +}
>
> [Severity: Medium]
> Are we leaking device and host references in the successful exit path?
>
> The test acquires references via ibmvfc_get_first_vhost() and
> get_device(&rport->dev) during setup, but unlike the other tests in this
> file (e.g., ibmvfc_full_fpin_test), it does not call put_device(&rport->dev)
> and scsi_host_put(vhost->host) before returning. Leaking these references
> can prevent module unloading and device cleanup.
>
>> +
>> +/**
>> + * ibmvfc_full_fpin_test - unit test for IBMVFC_AE_FPIN parts of ibmvfc_handle_async
>
> [ ... ]
>
>> +static void ibmvfc_extended_fpin_test(struct kunit *test)
>> +{
>> + enum ibmvfc_ae_fpin_status fs;
>> + struct ibmvfc_async_subq_fpin crq[IBMVFC_AE_FPIN_CONGESTION_CLEARED+1] = {};
>
> [Severity: Low]
> Does this undefined structure break the build?
>
> struct ibmvfc_async_subq_fpin is not defined in this commit, causing a
> compilation error.
Sure does. Will refactor into the correct commits for v10
-Tyrel
>
>> + struct ibmvfc_async_subq_fpin
>> + crqcn[IBMVFC_AE_FPIN_PORT_CONGESTED][FPIN_CONGN_DEVICE_SPEC+1] = {};
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v9 2/9] scsi: ibmvfc: add NOOP command support
2026-09-11 2:19 [PATCH v9 0/9] scsi: ibmvfc: make ibmvfc support FPIN messages Tyrel Datwyler
2026-09-11 2:19 ` [PATCH v9 1/9] scsi: ibmvfc: add basic FPIN support Tyrel Datwyler
@ 2026-09-11 2:19 ` Tyrel Datwyler
2026-09-11 2:31 ` sashiko-bot
2026-09-11 2:19 ` [PATCH v9 3/9] scsi: ibmvfc: add FPIN extended flag and async sub-CRQ queue handle Tyrel Datwyler
` (6 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Tyrel Datwyler @ 2026-09-11 2:19 UTC (permalink / raw)
To: james.bottomley, martin.petersen
Cc: linux-scsi, linuxppc-dev, linux-kernel, brking, davemarq,
Tyrel Datwyler
From: Dave Marquardt <davemarq@linux.ibm.com>
Add support for handling IBMVFC_NOOP format CRQ messages from the
VIOS partner.
Advertise the client's ability to handle NOOP commands by setting the
IBMVFC_CAN_USE_NOOP_CMD capability in ibmvfc_set_login_info().
Handle and ignore IBMVFC_NOOP format messages in both ibmvfc_handle_crq()
and ibmvfc_handle_scrq(). In both handlers, log a rate-limited error if a
NOOP is received while in the IBMVFC_ACTIVE state without the partner
having advertised IBMVFC_SUPPORT_NOOP_CMD.
In ibmvfc_handle_scrq(), also fix a typo ("Got and invalid" -> "Got an
invalid"), add a missing return on invalid message types, and guard
against null event pointers before processing CRQ responses.
Signed-off-by: Dave Marquardt <davemarq@linux.ibm.com>
Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc-core.c | 27 +++++++++++++++++++++++++--
1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
index 5259a80958ce..6b2eb82ceb98 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
@@ -1578,7 +1578,9 @@ static void ibmvfc_set_login_info(struct ibmvfc_host *vhost)
login_info->flags |= cpu_to_be16(IBMVFC_CLIENT_MIGRATED);
login_info->max_cmds = cpu_to_be32(max_cmds);
- login_info->capabilities = cpu_to_be64(IBMVFC_CAN_MIGRATE | IBMVFC_CAN_SEND_VF_WWPN);
+ login_info->capabilities =
+ cpu_to_be64(IBMVFC_CAN_MIGRATE | IBMVFC_CAN_SEND_VF_WWPN |
+ IBMVFC_CAN_USE_NOOP_CMD);
if (vhost->mq_enabled || vhost->using_channels) {
login_info->capabilities |= cpu_to_be64(IBMVFC_CAN_USE_CHANNELS);
@@ -3695,6 +3697,14 @@ static void ibmvfc_handle_crq(struct ibmvfc_crq *crq, struct ibmvfc_host *vhost,
if (crq->format == IBMVFC_ASYNC_EVENT)
return;
+ if (crq->format == IBMVFC_NOOP) {
+ if (vhost->state == IBMVFC_ACTIVE &&
+ !ibmvfc_check_caps(vhost, IBMVFC_SUPPORT_NOOP_CMD))
+ dev_err_ratelimited(vhost->dev,
+ "Received unexpected NOOP command from partner\n");
+ return;
+ }
+
/* The only kind of payload CRQs we should get are responses to
* things we send. Make sure this response is to something we
* actually sent
@@ -4225,7 +4235,20 @@ static void ibmvfc_handle_scrq(struct ibmvfc_crq *crq, struct ibmvfc_host *vhost
case IBMVFC_CRQ_XPORT_EVENT:
return;
default:
- dev_err(vhost->dev, "Got and invalid message type 0x%02x\n", crq->valid);
+ dev_err(vhost->dev, "Got an invalid message type 0x%02x\n", crq->valid);
+ return;
+ }
+
+ if (crq->format == IBMVFC_NOOP) {
+ if (vhost->state == IBMVFC_ACTIVE &&
+ !ibmvfc_check_caps(vhost, IBMVFC_SUPPORT_NOOP_CMD))
+ dev_err_ratelimited(vhost->dev,
+ "Received unexpected NOOP command from partner\n");
+ return;
+ }
+
+ if (unlikely(!evt)) {
+ dev_err(vhost->dev, "Received null event\n");
return;
}
--
2.55.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v9 2/9] scsi: ibmvfc: add NOOP command support
2026-09-11 2:19 ` [PATCH v9 2/9] scsi: ibmvfc: add NOOP command support Tyrel Datwyler
@ 2026-09-11 2:31 ` sashiko-bot
2026-09-11 3:41 ` Tyrel Datwyler
0 siblings, 1 reply; 23+ messages in thread
From: sashiko-bot @ 2026-09-11 2:31 UTC (permalink / raw)
To: Tyrel Datwyler; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Untrusted hypervisor input (`crq->ioba`) is dereferenced prior to validation, leading to a potential unmapped page fault (kernel panic).
--
commit 186cace34a3b58dd32c18fecc8f7d21b71c9763c
Author: Dave Marquardt <davemarq@linux.ibm.com>
scsi: ibmvfc: add NOOP command support
This commit adds support for handling IBMVFC_NOOP format CRQ messages from
the VIOS partner and advertises this capability during login. It updates
CRQ handlers to ignore NOOP messages while logging rate-limited errors if
the partner did not advertise support. Finally, it fixes a typo and adds a
null check for event pointers in ibmvfc_handle_scrq().
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 5259a80958ce1..6b2eb82ceb98d 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -4225,7 +4235,20 @@ static void ibmvfc_handle_scrq(struct ibmvfc_crq *crq, struct ibmvfc_host *vhost
> case IBMVFC_CRQ_XPORT_EVENT:
> return;
> default:
> - dev_err(vhost->dev, "Got and invalid message type 0x%02x\n", crq->valid);
> + dev_err(vhost->dev, "Got an invalid message type 0x%02x\n", crq->valid);
> + return;
> + }
> +
> + if (crq->format == IBMVFC_NOOP) {
> + if (vhost->state == IBMVFC_ACTIVE &&
> + !ibmvfc_check_caps(vhost, IBMVFC_SUPPORT_NOOP_CMD))
> + dev_err_ratelimited(vhost->dev,
> + "Received unexpected NOOP command from partner\n");
> + return;
> + }
> +
> + if (unlikely(!evt)) {
> + dev_err(vhost->dev, "Received null event\n");
> return;
> }
>
> /* The only kind of payload CRQs we should get are responses to
> * things we send. Make sure this response is to something we
> * actually sent
> */
> if (unlikely(!ibmvfc_valid_event(&evt->queue->evt_pool, evt))) {
[Severity: High]
This isn't a bug introduced by this patch, but does ibmvfc_handle_scrq()
dereference the unvalidated evt pointer?
Since evt is derived from an untrusted hypervisor value (crq->ioba), if the
hypervisor sends an invalid non-null pointer, evaluating &evt->queue->evt_pool
will dereference evt->queue before ibmvfc_valid_event() can check if evt
falls within the legitimate event pool bounds. Can this cause an unmapped
page fault and kernel panic?
Should the implementation bypass the untrusted pointer entirely when locating
the event pool, for instance by passing the actual scrq object from the
caller?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260911021910.1038128-1-tyreld@linux.ibm.com?part=2
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v9 2/9] scsi: ibmvfc: add NOOP command support
2026-09-11 2:31 ` sashiko-bot
@ 2026-09-11 3:41 ` Tyrel Datwyler
0 siblings, 0 replies; 23+ messages in thread
From: Tyrel Datwyler @ 2026-09-11 3:41 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-scsi
On 9/10/26 7:31 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Untrusted hypervisor input (`crq->ioba`) is dereferenced prior to validation, leading to a potential unmapped page fault (kernel panic).
> --
>
> commit 186cace34a3b58dd32c18fecc8f7d21b71c9763c
> Author: Dave Marquardt <davemarq@linux.ibm.com>
>
> scsi: ibmvfc: add NOOP command support
>
> This commit adds support for handling IBMVFC_NOOP format CRQ messages from
> the VIOS partner and advertises this capability during login. It updates
> CRQ handlers to ignore NOOP messages while logging rate-limited errors if
> the partner did not advertise support. Finally, it fixes a typo and adds a
> null check for event pointers in ibmvfc_handle_scrq().
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index 5259a80958ce1..6b2eb82ceb98d 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> [ ... ]
>> @@ -4225,7 +4235,20 @@ static void ibmvfc_handle_scrq(struct ibmvfc_crq *crq, struct ibmvfc_host *vhost
>> case IBMVFC_CRQ_XPORT_EVENT:
>> return;
>> default:
>> - dev_err(vhost->dev, "Got and invalid message type 0x%02x\n", crq->valid);
>> + dev_err(vhost->dev, "Got an invalid message type 0x%02x\n", crq->valid);
>> + return;
>> + }
>> +
>> + if (crq->format == IBMVFC_NOOP) {
>> + if (vhost->state == IBMVFC_ACTIVE &&
>> + !ibmvfc_check_caps(vhost, IBMVFC_SUPPORT_NOOP_CMD))
>> + dev_err_ratelimited(vhost->dev,
>> + "Received unexpected NOOP command from partner\n");
>> + return;
>> + }
>> +
>> + if (unlikely(!evt)) {
>> + dev_err(vhost->dev, "Received null event\n");
>> return;
>> }
>>
>> /* The only kind of payload CRQs we should get are responses to
>> * things we send. Make sure this response is to something we
>> * actually sent
>> */
>> if (unlikely(!ibmvfc_valid_event(&evt->queue->evt_pool, evt))) {
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does ibmvfc_handle_scrq()
> dereference the unvalidated evt pointer?
>
> Since evt is derived from an untrusted hypervisor value (crq->ioba), if the
> hypervisor sends an invalid non-null pointer, evaluating &evt->queue->evt_pool
> will dereference evt->queue before ibmvfc_valid_event() can check if evt
> falls within the legitimate event pool bounds. Can this cause an unmapped
> page fault and kernel panic?
>
> Should the implementation bypass the untrusted pointer entirely when locating
> the event pool, for instance by passing the actual scrq object from the
> caller?
>
We trust the hypervisor/VIOS to provide a valid IOBA to the relevant evt structure.
-Tyrel
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v9 3/9] scsi: ibmvfc: add FPIN extended flag and async sub-CRQ queue handle
2026-09-11 2:19 [PATCH v9 0/9] scsi: ibmvfc: make ibmvfc support FPIN messages Tyrel Datwyler
2026-09-11 2:19 ` [PATCH v9 1/9] scsi: ibmvfc: add basic FPIN support Tyrel Datwyler
2026-09-11 2:19 ` [PATCH v9 2/9] scsi: ibmvfc: add NOOP command support Tyrel Datwyler
@ 2026-09-11 2:19 ` Tyrel Datwyler
2026-09-11 2:19 ` [PATCH v9 4/9] scsi: ibmvfc: extend async event handlers for async sub-CRQ events Tyrel Datwyler
` (5 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Tyrel Datwyler @ 2026-09-11 2:19 UTC (permalink / raw)
To: james.bottomley, martin.petersen
Cc: linux-scsi, linuxppc-dev, linux-kernel, brking, davemarq,
Tyrel Datwyler
From: Dave Marquardt <davemarq@linux.ibm.com>
Add IBMVFC_ASYNC_IS_FPIN_EXT flag bit (0x02) to ibmvfc_async_sub_crq.flags
to distinguish extended FPIN events from standard ones.
Add async_sub_crq field of type ibmvfc_queue to ibmvfc_host to hold the
queue handle for the asynchronous sub-CRQ, alongside the existing crq and
async_crq queues.
Signed-off-by: Dave Marquardt <davemarq@linux.ibm.com>
Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
index d7a30e7a4a28..a8a063d99564 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.h
+++ b/drivers/scsi/ibmvscsi/ibmvfc.h
@@ -773,6 +773,7 @@ struct ibmvfc_async_sub_crq {
volatile u8 valid;
u8 flags;
#define IBMVFC_ASYNC_ID_IS_ASSOC_ID 0x01
+#define IBMVFC_ASYNC_IS_FPIN_EXT 0x02
u8 link_state;
u8 fpin_status;
__be16 event;
@@ -990,6 +991,7 @@ struct ibmvfc_host {
mempool_t *tgt_pool;
struct ibmvfc_queue crq;
struct ibmvfc_queue async_crq;
+ struct ibmvfc_queue async_sub_crq;
struct ibmvfc_channels scsi_scrqs;
struct ibmvfc_channels nvme_scrqs;
struct ibmvfc_npiv_login login_info;
--
2.55.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v9 4/9] scsi: ibmvfc: extend async event handlers for async sub-CRQ events
2026-09-11 2:19 [PATCH v9 0/9] scsi: ibmvfc: make ibmvfc support FPIN messages Tyrel Datwyler
` (2 preceding siblings ...)
2026-09-11 2:19 ` [PATCH v9 3/9] scsi: ibmvfc: add FPIN extended flag and async sub-CRQ queue handle Tyrel Datwyler
@ 2026-09-11 2:19 ` Tyrel Datwyler
2026-09-11 2:39 ` sashiko-bot
2026-09-11 2:19 ` [PATCH v9 5/9] scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ Tyrel Datwyler
` (4 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Tyrel Datwyler @ 2026-09-11 2:19 UTC (permalink / raw)
To: james.bottomley, martin.petersen
Cc: linux-scsi, linuxppc-dev, linux-kernel, brking, davemarq,
Tyrel Datwyler
From: Dave Marquardt <davemarq@linux.ibm.com>
Refactor async event handling to support both traditional async CRQs and
new asynchronous sub-queue CRQs.
Introduce struct ibmvfc_async_crq_event, a tagged union that wraps
either an ibmvfc_async_crq (main CRQ) or an ibmvfc_async_sub_crq
(async sub-CRQ), with an enum ibmvfc_async_crq_type discriminator.
Replace the ibmvfc_async_work bare union and is_subq bool with a
single event field of this type.
Modify ibmvfc_handle_async() to accept a struct ibmvfc_async_crq_event *
instead of a void * plus a bool flag. Update ibmvfc_process_async_work()
to dispatch based on event.type.
Add ibmvfc_full_fpin_to_desc() to convert full FPIN messages from async
sub-queue format to fc_els_fpin structures. Update FPIN processing logic
to extract WWPN, node_name, and scsi_id from the appropriate union member
based on event type.
Update KUnit tests to use struct ibmvfc_async_crq_event arrays at call
sites.
Signed-off-by: Dave Marquardt <davemarq@linux.ibm.com>
Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc-core.c | 162 +++++++++++++++++++++------
drivers/scsi/ibmvscsi/ibmvfc.h | 18 ++-
drivers/scsi/ibmvscsi/ibmvfc_kunit.c | 135 ----------------------
3 files changed, 141 insertions(+), 174 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
index 6b2eb82ceb98..daec81f48857 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
@@ -3426,6 +3426,45 @@ ibmvfc_basic_fpin_to_desc(struct ibmvfc_async_crq *crq, u64 wwpn)
cpu_to_be32(1));
}
+/**
+ * ibmvfc_full_fpin_to_desc(): allocate and populate a struct fc_els_fpin struct
+ * containing a descriptor.
+ * @ibmvfc_fpin: Pointer to async subq FPIN data
+ *
+ * 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_full_fpin_to_desc(struct ibmvfc_async_sub_crq *ibmvfc_fpin)
+{
+ __be16 type;
+
+ switch (ibmvfc_fpin->fpin_status) {
+ case IBMVFC_AE_FPIN_LINK_CONGESTED:
+ case IBMVFC_AE_FPIN_PORT_CONGESTED:
+ type = cpu_to_be16(FPIN_CONGN_DEVICE_SPEC);
+ break;
+ case IBMVFC_AE_FPIN_PORT_CLEARED:
+ case IBMVFC_AE_FPIN_CONGESTION_CLEARED:
+ type = cpu_to_be16(FPIN_CONGN_CLEAR);
+ break;
+ case IBMVFC_AE_FPIN_PORT_DEGRADED:
+ type = cpu_to_be16(FPIN_LI_UNKNOWN);
+ break;
+ default:
+ return NULL;
+ }
+
+ return ibmvfc_common_fpin_to_desc(ibmvfc_fpin->fpin_status, ibmvfc_fpin->wwpn,
+ type, cpu_to_be16(0),
+ cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_THRESHOLD),
+ cpu_to_be32(1));
+}
+
/**
* ibmvfc_find_target - Search for a target in a target list
* @target_list: list head of targets to search
@@ -3463,28 +3502,39 @@ static struct ibmvfc_target *ibmvfc_find_target(struct list_head *target_list,
*/
static void ibmvfc_process_async_work(struct work_struct *work)
{
+ struct ibmvfc_async_sub_crq *subq = NULL;
struct ibmvfc_async_work *aw;
- struct ibmvfc_async_crq *crq;
+ struct ibmvfc_async_crq *crq = NULL;
struct ibmvfc_target *tgt;
struct ibmvfc_host *vhost;
- struct fc_els_fpin *fpin;
+ struct fc_els_fpin *fpin = NULL;
unsigned long flags;
+ __be64 node_name;
+ __be64 scsi_id;
+ __be64 wwpn;
aw = container_of_const(work, struct ibmvfc_async_work, async_work_s);
vhost = aw->vhost;
- crq = &aw->crq;
+ if (aw->event.type == IBMVFC_ASYNC_CRQ_SUB) {
+ subq = &aw->event.subq;
+ scsi_id = 0;
+ wwpn = subq->wwpn;
+ node_name = (subq->flags & IBMVFC_ASYNC_ID_IS_ASSOC_ID) ? 0 : subq->id.node_name;
+ } else {
+ crq = &aw->event.async_crq;
+ scsi_id = crq->scsi_id;
+ wwpn = crq->wwpn;
+ node_name = crq->node_name;
+ }
- if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
+ if (!scsi_id && !wwpn && !node_name)
goto free;
spin_lock_irqsave(vhost->host->host_lock, flags);
- tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, crq->scsi_id,
- crq->wwpn, crq->node_name);
+ tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, scsi_id, wwpn, node_name);
if (!tgt) {
/* Target not found in scsi_scrqs, search nvme_scrqs */
- tgt = ibmvfc_find_target(&vhost->nvme_scrqs.targets,
- crq->scsi_id, crq->wwpn,
- crq->node_name);
+ tgt = ibmvfc_find_target(&vhost->nvme_scrqs.targets, scsi_id, wwpn, node_name);
}
if (tgt) {
@@ -3496,7 +3546,11 @@ static void ibmvfc_process_async_work(struct work_struct *work)
goto free;
}
- fpin = ibmvfc_basic_fpin_to_desc(crq, tgt->wwpn);
+ if (crq)
+ fpin = ibmvfc_basic_fpin_to_desc(crq, tgt->wwpn);
+ else
+ fpin = ibmvfc_full_fpin_to_desc(subq);
+
if (fpin) {
fc_host_fpin_rcv(tgt->vhost->host,
sizeof(*fpin) + be32_to_cpu(fpin->desc_len),
@@ -3512,25 +3566,51 @@ static void ibmvfc_process_async_work(struct work_struct *work)
/**
* ibmvfc_handle_async - Handle an async event from the adapter
- * @crq: crq to process
+ * @ae: tagged union wrapping either an ibmvfc_async_crq (main CRQ) or an
+ * ibmvfc_async_sub_crq (async sub-CRQ); the type field identifies which
* @vhost: ibmvfc host struct
*
**/
-VISIBLE_IF_KUNIT void ibmvfc_handle_async(struct ibmvfc_async_crq *crq,
+VISIBLE_IF_KUNIT void ibmvfc_handle_async(struct ibmvfc_async_crq_event *ae,
struct ibmvfc_host *vhost)
{
- const struct ibmvfc_async_desc *desc = ibmvfc_get_ae_desc(be64_to_cpu(crq->event));
+ struct ibmvfc_async_crq *async_crq = NULL;
+ struct ibmvfc_async_sub_crq *subq = NULL;
+ const struct ibmvfc_async_desc *desc;
struct ibmvfc_async_work *aw;
struct ibmvfc_target *tgt;
-
- ibmvfc_log(vhost, desc->log_level, "%s event received. scsi_id: %llx, wwpn: %llx,"
- " node_name: %llx%s\n", desc->desc, be64_to_cpu(crq->scsi_id),
- be64_to_cpu(crq->wwpn), be64_to_cpu(crq->node_name),
- ibmvfc_get_link_state(crq->link_state));
-
- switch (be64_to_cpu(crq->event)) {
+ __be64 node_name;
+ __be64 scsi_id;
+ u8 link_state;
+ __be64 wwpn;
+ u64 event;
+
+ if (ae->type == IBMVFC_ASYNC_CRQ_SUB) {
+ subq = &ae->subq;
+ event = be16_to_cpu(subq->event);
+ link_state = subq->link_state;
+ scsi_id = 0;
+ wwpn = subq->wwpn;
+ node_name = subq->flags & IBMVFC_ASYNC_ID_IS_ASSOC_ID ? 0 : subq->id.node_name;
+ } else {
+ async_crq = &ae->async_crq;
+ event = be64_to_cpu(async_crq->event);
+ link_state = async_crq->link_state;
+ scsi_id = async_crq->scsi_id;
+ wwpn = async_crq->wwpn;
+ node_name = async_crq->node_name;
+ }
+
+ desc = ibmvfc_get_ae_desc(event);
+ ibmvfc_log(vhost, desc->log_level,
+ "%s event received. scsi_id: %llx, wwpn: %llx, node_name: %llx, event %llx%s\n",
+ desc->desc, be64_to_cpu(scsi_id),
+ be64_to_cpu(wwpn), be64_to_cpu(node_name), event,
+ ibmvfc_get_link_state(link_state));
+
+ switch (event) {
case IBMVFC_AE_RESUME:
- switch (crq->link_state) {
+ switch (link_state) {
case IBMVFC_AE_LS_LINK_DOWN:
ibmvfc_link_down(vhost, IBMVFC_LINK_DOWN);
break;
@@ -3569,33 +3649,33 @@ VISIBLE_IF_KUNIT void ibmvfc_handle_async(struct ibmvfc_async_crq *crq,
case IBMVFC_AE_ELS_PRLO:
case IBMVFC_AE_ELS_PLOGI:
list_for_each_entry(tgt, &vhost->scsi_scrqs.targets, queue) {
- if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
+ if (!scsi_id && !wwpn && !node_name)
break;
- if (crq->scsi_id && cpu_to_be64(tgt->scsi_id) != crq->scsi_id)
+ if (scsi_id && cpu_to_be64(tgt->scsi_id) != scsi_id)
continue;
- if (crq->wwpn && cpu_to_be64(tgt->ids.port_name) != crq->wwpn)
+ if (wwpn && cpu_to_be64(tgt->ids.port_name) != wwpn)
continue;
- if (crq->node_name && cpu_to_be64(tgt->ids.node_name) != crq->node_name)
+ if (node_name && cpu_to_be64(tgt->ids.node_name) != node_name)
continue;
- if (tgt->need_login && be64_to_cpu(crq->event) == IBMVFC_AE_ELS_LOGO)
+ if (tgt->need_login && event == IBMVFC_AE_ELS_LOGO)
tgt->logo_rcvd = 1;
- if (!tgt->need_login || be64_to_cpu(crq->event) == IBMVFC_AE_ELS_PLOGI) {
+ if (!tgt->need_login || event == IBMVFC_AE_ELS_PLOGI) {
ibmvfc_del_tgt(tgt);
ibmvfc_reinit_host(vhost);
}
}
list_for_each_entry(tgt, &vhost->nvme_scrqs.targets, queue) {
- if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
+ if (!scsi_id && !wwpn && !node_name)
break;
- if (crq->scsi_id && cpu_to_be64(tgt->scsi_id) != crq->scsi_id)
+ if (scsi_id && cpu_to_be64(tgt->scsi_id) != scsi_id)
continue;
- if (crq->wwpn && cpu_to_be64(tgt->ids.port_name) != crq->wwpn)
+ if (wwpn && cpu_to_be64(tgt->ids.port_name) != wwpn)
continue;
- if (crq->node_name && cpu_to_be64(tgt->ids.node_name) != crq->node_name)
+ if (node_name && cpu_to_be64(tgt->ids.node_name) != node_name)
continue;
- if (tgt->need_login && be64_to_cpu(crq->event) == IBMVFC_AE_ELS_LOGO)
+ if (tgt->need_login && event == IBMVFC_AE_ELS_LOGO)
tgt->logo_rcvd = 1;
- if (!tgt->need_login || be64_to_cpu(crq->event) == IBMVFC_AE_ELS_PLOGI) {
+ if (!tgt->need_login || event == IBMVFC_AE_ELS_PLOGI) {
ibmvfc_del_tgt(tgt);
ibmvfc_reinit_host(vhost);
}
@@ -3618,14 +3698,14 @@ VISIBLE_IF_KUNIT void ibmvfc_handle_async(struct ibmvfc_async_crq *crq,
if (aw) {
INIT_WORK(&aw->async_work_s, ibmvfc_process_async_work);
aw->vhost = vhost;
- aw->crq = *crq;
+ aw->event = *ae;
queue_work(vhost->fpin_workq, &aw->async_work_s);
} else
dev_err_ratelimited(vhost->dev,
"can't offload async CRQ to work queue\n");
break;
default:
- dev_err(vhost->dev, "Unknown async event received: %lld\n", crq->event);
+ dev_err(vhost->dev, "Unknown async event received: %llu\n", event);
break;
}
}
@@ -4166,7 +4246,11 @@ static void ibmvfc_tasklet(void *data)
while (!done) {
/* Pull all the valid messages off the async CRQ */
while ((async = ibmvfc_next_async_crq(vhost)) != NULL) {
- ibmvfc_handle_async(async, vhost);
+ struct ibmvfc_async_crq_event ae = {
+ .type = IBMVFC_ASYNC_CRQ_MAIN,
+ .async_crq = *async,
+ };
+ ibmvfc_handle_async(&ae, vhost);
async->valid = 0;
wmb();
}
@@ -4180,8 +4264,12 @@ static void ibmvfc_tasklet(void *data)
vio_enable_interrupts(vdev);
if ((async = ibmvfc_next_async_crq(vhost)) != NULL) {
+ struct ibmvfc_async_crq_event ae = {
+ .type = IBMVFC_ASYNC_CRQ_MAIN,
+ .async_crq = *async,
+ };
vio_disable_interrupts(vdev);
- ibmvfc_handle_async(async, vhost);
+ ibmvfc_handle_async(&ae, vhost);
async->valid = 0;
wmb();
} else if ((crq = ibmvfc_next_crq(vhost)) != NULL) {
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
index a8a063d99564..6fce7a4922bb 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.h
+++ b/drivers/scsi/ibmvscsi/ibmvfc.h
@@ -786,9 +786,22 @@ struct ibmvfc_async_sub_crq {
} id;
} __packed __aligned(8);
+enum ibmvfc_async_crq_type {
+ IBMVFC_ASYNC_CRQ_MAIN = 0,
+ IBMVFC_ASYNC_CRQ_SUB,
+};
+
+struct ibmvfc_async_crq_event {
+ enum ibmvfc_async_crq_type type;
+ union {
+ struct ibmvfc_async_crq async_crq;
+ struct ibmvfc_async_sub_crq subq;
+ };
+};
+
struct ibmvfc_async_work {
struct ibmvfc_host *vhost;
- struct ibmvfc_async_crq crq;
+ struct ibmvfc_async_crq_event event;
struct work_struct async_work_s;
};
@@ -1103,7 +1116,8 @@ static inline struct ibmvfc_host *ibmvfc_channels_to_vhost(struct ibmvfc_channel
#if IS_ENABLED(CONFIG_KUNIT)
#include <kunit/visibility.h>
-VISIBLE_IF_KUNIT void ibmvfc_handle_async(struct ibmvfc_async_crq *crq, struct ibmvfc_host *vhost);
+VISIBLE_IF_KUNIT void ibmvfc_handle_async(struct ibmvfc_async_crq_event *event,
+ struct ibmvfc_host *vhost);
VISIBLE_IF_KUNIT struct ibmvfc_host *ibmvfc_get_first_vhost(void);
#endif
diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
index 80e7f8e0fd70..7aaed4f64f26 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
@@ -3,7 +3,6 @@
#include <kunit/visibility.h>
#include <scsi/scsi_device.h>
#include <scsi/scsi_transport_fc.h>
-#include <scsi/fc/fc_els.h>
#include <linux/list.h>
#include <linux/delay.h>
#include "ibmvfc.h"
@@ -257,143 +256,9 @@ static void ibmvfc_full_fpin_test(struct kunit *test)
scsi_host_put(vhost->host);
}
-#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); \
-}
-
-/**
- * ibmvfc_extended_fpin_test - unit test for extended FPIN events
- * @test: pointer to kunit structure
- *
- * Note: This test exercises extended FPIN code paths but does not check
- * that statistics are correctly updated.
- *
- * Return: void
- */
-static void ibmvfc_extended_fpin_test(struct kunit *test)
-{
- enum ibmvfc_ae_fpin_status fs;
- struct ibmvfc_async_subq_fpin crq[IBMVFC_AE_FPIN_CONGESTION_CLEARED+1] = {};
- struct ibmvfc_async_subq_fpin
- crqcn[IBMVFC_AE_FPIN_PORT_CONGESTED][FPIN_CONGN_DEVICE_SPEC+1] = {};
- struct ibmvfc_async_subq_fpin crqportdg[FPIN_LI_DEVICE_SPEC+1] = {};
- struct ibmvfc_target *tgt;
- struct ibmvfc_host *vhost;
- struct fc_rport *rport;
- LIST_HEAD(evt_doneq);
- unsigned long flags;
- u64 pre, post;
-
- vhost = ibmvfc_get_first_vhost();
- if (!vhost)
- kunit_skip(test, "No ibmvfc devices available");
-
- spin_lock_irqsave(vhost->host->host_lock, flags);
- if (vhost->scsi_scrqs.num_targets < 1) {
- spin_unlock_irqrestore(vhost->host->host_lock, flags);
- scsi_host_put(vhost->host);
- kunit_skip(test, "No targets");
- }
- tgt = list_first_entry(&vhost->scsi_scrqs.targets, struct ibmvfc_target, queue);
- if (!tgt->rport) {
- spin_unlock_irqrestore(vhost->host->host_lock, flags);
- scsi_host_put(vhost->host);
- kunit_skip(test, "No rport");
- }
- rport = tgt->rport;
- get_device(&rport->dev);
- kref_get(&tgt->kref);
- spin_unlock_irqrestore(vhost->host->host_lock, flags);
-
- for (fs = IBMVFC_AE_FPIN_LINK_CONGESTED; fs <= IBMVFC_AE_FPIN_CONGESTION_CLEARED; fs++) {
- switch (fs) {
- case IBMVFC_AE_FPIN_PORT_CLEARED:
- case IBMVFC_AE_FPIN_CONGESTION_CLEARED: {
- struct ibmvfc_async_crq_event ae = { .type = IBMVFC_ASYNC_CRQ_SUB };
-
- crq[fs].valid = 0x80;
- crq[fs].flags = IBMVFC_ASYNC_IS_FPIN_EXT;
- crq[fs].link_state = IBMVFC_AE_LS_LINK_UP;
- crq[fs].fpin_status = fs;
- crq[fs].event = cpu_to_be16(IBMVFC_AE_FPIN);
- crq[fs].wwpn = cpu_to_be64(tgt->wwpn);
- crq[fs].fpin_data.flags = IBMVFC_FPIN_EVENT_TYPE_VALID;
- crq[fs].fpin_data.event_type = cpu_to_be16(FPIN_CONGN_CLEAR);
- ae.subq = *(struct ibmvfc_async_sub_crq *)&crq[fs];
- pre = READ_ONCE(rport->fpin_stats.cn_clear);
- ibmvfc_handle_async(&ae, vhost);
- flush_workqueue(vhost->fpin_workq);
- post = READ_ONCE(rport->fpin_stats.cn_clear);
- break;
- }
- case IBMVFC_AE_FPIN_LINK_CONGESTED:
- case IBMVFC_AE_FPIN_PORT_CONGESTED:
- IBMVFC_TEST_FPIN_EXT(fs, FPIN_CONGN_CLEAR, cn_clear,
- crqcn[fs-1][FPIN_CONGN_CLEAR]);
- IBMVFC_TEST_FPIN_EXT(fs, FPIN_CONGN_LOST_CREDIT,
- cn_lost_credit,
- crqcn[fs-1][FPIN_CONGN_LOST_CREDIT]);
- IBMVFC_TEST_FPIN_EXT(fs, FPIN_CONGN_CREDIT_STALL,
- cn_credit_stall,
- crqcn[fs-1][FPIN_CONGN_CREDIT_STALL]);
- IBMVFC_TEST_FPIN_EXT(fs, FPIN_CONGN_OVERSUBSCRIPTION,
- cn_oversubscription,
- crqcn[fs-1][FPIN_CONGN_OVERSUBSCRIPTION]);
- IBMVFC_TEST_FPIN_EXT(fs, FPIN_CONGN_DEVICE_SPEC,
- cn_device_specific,
- crqcn[fs-1][FPIN_CONGN_DEVICE_SPEC]);
- break;
- case IBMVFC_AE_FPIN_PORT_DEGRADED:
- IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_UNKNOWN,
- li_failure_unknown,
- crqportdg[FPIN_LI_UNKNOWN]);
- IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_LINK_FAILURE,
- li_link_failure_count,
- crqportdg[FPIN_LI_LINK_FAILURE]);
- IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_LOSS_OF_SYNC,
- li_loss_of_sync_count,
- crqportdg[FPIN_LI_LOSS_OF_SYNC]);
- IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_LOSS_OF_SIG,
- li_loss_of_signals_count,
- crqportdg[FPIN_LI_LOSS_OF_SIG]);
- IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_PRIM_SEQ_ERR,
- li_prim_seq_err_count,
- crqportdg[FPIN_LI_PRIM_SEQ_ERR]);
- IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_INVALID_TX_WD,
- li_invalid_tx_word_count,
- crqportdg[FPIN_LI_INVALID_TX_WD]);
- IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_INVALID_CRC,
- li_invalid_crc_count,
- crqportdg[FPIN_LI_INVALID_CRC]);
- IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_DEVICE_SPEC,
- li_device_specific,
- crqportdg[FPIN_LI_DEVICE_SPEC]);
- break;
- }
- }
-
- put_device(&rport->dev);
- kref_put(&tgt->kref, ibmvfc_release_tgt);
- scsi_host_put(vhost->host);
-}
-
static struct kunit_case ibmvfc_fpin_test_cases[] = {
KUNIT_CASE(ibmvfc_async_fpin_test),
KUNIT_CASE(ibmvfc_full_fpin_test),
- KUNIT_CASE(ibmvfc_extended_fpin_test),
{},
};
--
2.55.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v9 4/9] scsi: ibmvfc: extend async event handlers for async sub-CRQ events
2026-09-11 2:19 ` [PATCH v9 4/9] scsi: ibmvfc: extend async event handlers for async sub-CRQ events Tyrel Datwyler
@ 2026-09-11 2:39 ` sashiko-bot
2026-09-11 3:52 ` Tyrel Datwyler
0 siblings, 1 reply; 23+ messages in thread
From: sashiko-bot @ 2026-09-11 2:39 UTC (permalink / raw)
To: Tyrel Datwyler; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] ibmvfc_find_target() incorrectly skips targets unless they support BOTH SCSI and NVMe, causing FPIN events to be dropped.
- [Medium] Missing scsi_id extraction for async sub-CRQ events.
- [Medium] Incorrect WWPN populated in FPIN descriptor for async sub-CRQ events when subq->wwpn is 0.
- [Low] Missing address-of operator '&' when passing host_lock to spin_lock_irqsave() and spin_unlock_irqrestore(), causing a build failure.
--
commit 997e4bca2edfad65478e2c572b7c236ba500e1bb
Author: Dave Marquardt <davemarq@linux.ibm.com>
scsi: ibmvfc: extend async event handlers for async sub-CRQ events
This patch refactors async event handling to support both traditional async
CRQs and new asynchronous sub-queue CRQs using a tagged union. It also
updates FPIN processing logic to extract identifiers from the appropriate
event type.
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 6b2eb82ceb98d..daec81f48857d 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -3426,6 +3426,45 @@ ibmvfc_basic_fpin_to_desc(struct ibmvfc_async_crq *crq, u64 wwpn)
> +static struct fc_els_fpin *
> +ibmvfc_full_fpin_to_desc(struct ibmvfc_async_sub_crq *ibmvfc_fpin)
> +{
> + __be16 type;
[ ... ]
> + return ibmvfc_common_fpin_to_desc(ibmvfc_fpin->fpin_status, ibmvfc_fpin->wwpn,
> + type, cpu_to_be16(0),
> + cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_THRESHOLD),
> + cpu_to_be32(1));
> +}
[Severity: Medium]
If the async sub-CRQ event arrives with a WWPN of 0 but the target is
successfully matched via node_name, will this embed a WWPN of 0 into the
FPIN descriptor?
It looks like fc_host_fpin_rcv() would then fail to find the remote port by
WWPN, silently dropping the FPIN and missing statistics updates. Would it be
safer to pass in the matched tgt->wwpn here, similar to how it's done for
standard CRQs?
> /**
> * ibmvfc_find_target - Search for a target in a target list
> * @target_list: list head of targets to search
> @@ -3463,28 +3502,39 @@ static struct ibmvfc_target *ibmvfc_find_target(struct list_head *target_list,
> */
> static void ibmvfc_process_async_work(struct work_struct *work)
> {
> + struct ibmvfc_async_sub_crq *subq = NULL;
> struct ibmvfc_async_work *aw;
> - struct ibmvfc_async_crq *crq;
> + struct ibmvfc_async_crq *crq = NULL;
> struct ibmvfc_target *tgt;
> struct ibmvfc_host *vhost;
> - struct fc_els_fpin *fpin;
> + struct fc_els_fpin *fpin = NULL;
> unsigned long flags;
> + __be64 node_name;
> + __be64 scsi_id;
> + __be64 wwpn;
>
> aw = container_of_const(work, struct ibmvfc_async_work, async_work_s);
> vhost = aw->vhost;
> - crq = &aw->crq;
> + if (aw->event.type == IBMVFC_ASYNC_CRQ_SUB) {
> + subq = &aw->event.subq;
> + scsi_id = 0;
[Severity: Medium]
The commit message states we "extract WWPN, node_name, and scsi_id from the
appropriate union member based on event type."
However, for async sub-CRQ events here and in ibmvfc_handle_async(), scsi_id
is hardcoded to 0 instead of extracting nport_id from the payload. Could this
cause target lookup failures if WWPN and node_name happen to be 0?
> + wwpn = subq->wwpn;
> + node_name = (subq->flags & IBMVFC_ASYNC_ID_IS_ASSOC_ID) ? 0 : subq->id.node_name;
> + } else {
> + crq = &aw->event.async_crq;
> + scsi_id = crq->scsi_id;
> + wwpn = crq->wwpn;
> + node_name = crq->node_name;
> + }
>
> - if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
> + if (!scsi_id && !wwpn && !node_name)
> goto free;
>
> spin_lock_irqsave(vhost->host->host_lock, flags);
> - tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, crq->scsi_id,
> - crq->wwpn, crq->node_name);
> + tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, scsi_id, wwpn, node_name);
[Severity: High]
Does ibmvfc_find_target() incorrectly skip targets unless they support BOTH
SCSI and NVMe?
Looking at the implementation of ibmvfc_find_target():
if (!tgt->rport || !tgt->nvme_remote_port)
continue;
Because a target represents a single endpoint for a specific protocol (SCSI or
NVMe), it will only ever have either tgt->rport or tgt->nvme_remote_port,
never both.
Because of the logical OR, this condition always evaluates to true. Could this
effectively break FPIN event handling by causing every target to be skipped?
[ ... ]
> @@ -3512,25 +3566,51 @@ static void ibmvfc_process_async_work(struct work_struct *work)
>
> /**
> * ibmvfc_handle_async - Handle an async event from the adapter
[ ... ]
> if (ae->type == IBMVFC_ASYNC_CRQ_SUB) {
> - switch (be64_to_cpu(crq->event)) {
> + subq = &ae->subq;
> + event = be16_to_cpu(subq->event);
> + link_state = subq->link_state;
> + scsi_id = 0;
[Severity: Medium]
Similarly, this hardcodes scsi_id to 0 for async sub-CRQ events instead of
extracting it from the event payload.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260911021910.1038128-1-tyreld@linux.ibm.com?part=4
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v9 4/9] scsi: ibmvfc: extend async event handlers for async sub-CRQ events
2026-09-11 2:39 ` sashiko-bot
@ 2026-09-11 3:52 ` Tyrel Datwyler
0 siblings, 0 replies; 23+ messages in thread
From: Tyrel Datwyler @ 2026-09-11 3:52 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-scsi
On 9/10/26 7:39 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] ibmvfc_find_target() incorrectly skips targets unless they support BOTH SCSI and NVMe, causing FPIN events to be dropped.
> - [Medium] Missing scsi_id extraction for async sub-CRQ events.
> - [Medium] Incorrect WWPN populated in FPIN descriptor for async sub-CRQ events when subq->wwpn is 0.
> - [Low] Missing address-of operator '&' when passing host_lock to spin_lock_irqsave() and spin_unlock_irqrestore(), causing a build failure.
> --
>
> commit 997e4bca2edfad65478e2c572b7c236ba500e1bb
> Author: Dave Marquardt <davemarq@linux.ibm.com>
>
> scsi: ibmvfc: extend async event handlers for async sub-CRQ events
>
> This patch refactors async event handling to support both traditional async
> CRQs and new asynchronous sub-queue CRQs using a tagged union. It also
> updates FPIN processing logic to extract identifiers from the appropriate
> event type.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index 6b2eb82ceb98d..daec81f48857d 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> [ ... ]
>> @@ -3426,6 +3426,45 @@ ibmvfc_basic_fpin_to_desc(struct ibmvfc_async_crq *crq, u64 wwpn)
>> +static struct fc_els_fpin *
>> +ibmvfc_full_fpin_to_desc(struct ibmvfc_async_sub_crq *ibmvfc_fpin)
>> +{
>> + __be16 type;
> [ ... ]
>> + return ibmvfc_common_fpin_to_desc(ibmvfc_fpin->fpin_status, ibmvfc_fpin->wwpn,
>> + type, cpu_to_be16(0),
>> + cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_THRESHOLD),
>> + cpu_to_be32(1));
>> +}
>
> [Severity: Medium]
> If the async sub-CRQ event arrives with a WWPN of 0 but the target is
> successfully matched via node_name, will this embed a WWPN of 0 into the
> FPIN descriptor?
>
> It looks like fc_host_fpin_rcv() would then fail to find the remote port by
> WWPN, silently dropping the FPIN and missing statistics updates. Would it be
> safer to pass in the matched tgt->wwpn here, similar to how it's done for
> standard CRQs?
The wwpn and node name should never be zero.
>
>> /**
>> * ibmvfc_find_target - Search for a target in a target list
>> * @target_list: list head of targets to search
>> @@ -3463,28 +3502,39 @@ static struct ibmvfc_target *ibmvfc_find_target(struct list_head *target_list,
>> */
>> static void ibmvfc_process_async_work(struct work_struct *work)
>> {
>> + struct ibmvfc_async_sub_crq *subq = NULL;
>> struct ibmvfc_async_work *aw;
>> - struct ibmvfc_async_crq *crq;
>> + struct ibmvfc_async_crq *crq = NULL;
>> struct ibmvfc_target *tgt;
>> struct ibmvfc_host *vhost;
>> - struct fc_els_fpin *fpin;
>> + struct fc_els_fpin *fpin = NULL;
>> unsigned long flags;
>> + __be64 node_name;
>> + __be64 scsi_id;
>> + __be64 wwpn;
>>
>> aw = container_of_const(work, struct ibmvfc_async_work, async_work_s);
>> vhost = aw->vhost;
>> - crq = &aw->crq;
>> + if (aw->event.type == IBMVFC_ASYNC_CRQ_SUB) {
>> + subq = &aw->event.subq;
>> + scsi_id = 0;
>
> [Severity: Medium]
> The commit message states we "extract WWPN, node_name, and scsi_id from the
> appropriate union member based on event type."
>
> However, for async sub-CRQ events here and in ibmvfc_handle_async(), scsi_id
> is hardcoded to 0 instead of extracting nport_id from the payload. Could this
> cause target lookup failures if WWPN and node_name happen to be 0?
Again wwpn and node_name should never be zero here.
>
>> + wwpn = subq->wwpn;
>> + node_name = (subq->flags & IBMVFC_ASYNC_ID_IS_ASSOC_ID) ? 0 : subq->id.node_name;
>> + } else {
>> + crq = &aw->event.async_crq;
>> + scsi_id = crq->scsi_id;
>> + wwpn = crq->wwpn;
>> + node_name = crq->node_name;
>> + }
>>
>> - if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
>> + if (!scsi_id && !wwpn && !node_name)
>> goto free;
>>
>> spin_lock_irqsave(vhost->host->host_lock, flags);
>> - tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, crq->scsi_id,
>> - crq->wwpn, crq->node_name);
>> + tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, scsi_id, wwpn, node_name);
>
> [Severity: High]
> Does ibmvfc_find_target() incorrectly skip targets unless they support BOTH
> SCSI and NVMe?
>
> Looking at the implementation of ibmvfc_find_target():
>
> if (!tgt->rport || !tgt->nvme_remote_port)
> continue;
>
> Because a target represents a single endpoint for a specific protocol (SCSI or
> NVMe), it will only ever have either tgt->rport or tgt->nvme_remote_port,
> never both.
>
> Because of the logical OR, this condition always evaluates to true. Could this
> effectively break FPIN event handling by causing every target to be skipped?
This was covered in patch 1 and will be fixed in v10.
>
> [ ... ]
>> @@ -3512,25 +3566,51 @@ static void ibmvfc_process_async_work(struct work_struct *work)
>>
>> /**
>> * ibmvfc_handle_async - Handle an async event from the adapter
> [ ... ]
>> if (ae->type == IBMVFC_ASYNC_CRQ_SUB) {
>> - switch (be64_to_cpu(crq->event)) {
>> + subq = &ae->subq;
>> + event = be16_to_cpu(subq->event);
>> + link_state = subq->link_state;
>> + scsi_id = 0;
>
> [Severity: Medium]
> Similarly, this hardcodes scsi_id to 0 for async sub-CRQ events instead of
> extracting it from the event payload.
>
scsi_id is not part of a subq event payload. We set it to zero to ensure we
don't pass junk to ibmvfc_find_target.
-Tyrel
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v9 5/9] scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ
2026-09-11 2:19 [PATCH v9 0/9] scsi: ibmvfc: make ibmvfc support FPIN messages Tyrel Datwyler
` (3 preceding siblings ...)
2026-09-11 2:19 ` [PATCH v9 4/9] scsi: ibmvfc: extend async event handlers for async sub-CRQ events Tyrel Datwyler
@ 2026-09-11 2:19 ` Tyrel Datwyler
2026-09-11 2:44 ` sashiko-bot
2026-09-11 2:19 ` [PATCH v9 6/9] scsi: ibmvfc: extend channel reg/dereg helpers for async sub-CRQ Tyrel Datwyler
` (3 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Tyrel Datwyler @ 2026-09-11 2:19 UTC (permalink / raw)
To: james.bottomley, martin.petersen
Cc: linux-scsi, linuxppc-dev, linux-kernel, brking, davemarq,
Tyrel Datwyler
From: Dave Marquardt <davemarq@linux.ibm.com>
Add ibmvfc_interrupt_async_subq(), an IRQ handler dedicated to
asynchronous sub-CRQ events from the adapter. The handler disables
the sub-CRQ IRQ and then calls ibmvfc_drain_async_subq() to consume
all pending entries before re-enabling interrupts. The handler is
marked as __maybe_unused until a later patch when it is used.
ibmvfc_drain_async_subq() holds the per-queue q_lock while
processing. It loops over available CRQ entries via ibmvfc_next_scrq(),
wrapping each in a typed struct ibmvfc_async_crq_event and dispatching
it to ibmvfc_handle_async(), then clears the valid bit and issues a
write barrier. After draining, it re-enables the sub-CRQ IRQ and performs
one final check for a newly arrived entry to close the IRQ-enable race;
if one is found it is processed before exiting the loop.
Signed-off-by: Dave Marquardt <davemarq@linux.ibm.com>
Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc-core.c | 54 +++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
index daec81f48857..553fca31cb3a 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
@@ -4376,6 +4376,60 @@ static struct ibmvfc_crq *ibmvfc_next_scrq(struct ibmvfc_queue *scrq)
return crq;
}
+static void ibmvfc_drain_async_subq(struct ibmvfc_queue *scrq)
+{
+ struct ibmvfc_host *vhost = scrq->vhost;
+ unsigned long flags;
+ struct ibmvfc_crq *crq;
+ int done = 0;
+
+ spin_lock_irqsave(vhost->host->host_lock, flags);
+ spin_lock(scrq->q_lock);
+ while (!done) {
+ while ((crq = ibmvfc_next_scrq(scrq)) != NULL) {
+ struct ibmvfc_async_crq_event ae = {
+ .type = IBMVFC_ASYNC_CRQ_SUB,
+ .subq = *(struct ibmvfc_async_sub_crq *)crq,
+ };
+ ibmvfc_handle_async(&ae, scrq->vhost);
+ crq->valid = 0;
+ wmb(); /* complete write */
+ }
+
+ ibmvfc_toggle_scrq_irq(scrq, 1);
+ crq = ibmvfc_next_scrq(scrq);
+ if (crq != NULL) {
+ struct ibmvfc_async_crq_event ae = {
+ .type = IBMVFC_ASYNC_CRQ_SUB,
+ .subq = *(struct ibmvfc_async_sub_crq *)crq,
+ };
+ ibmvfc_toggle_scrq_irq(scrq, 0);
+ ibmvfc_handle_async(&ae, scrq->vhost);
+ crq->valid = 0;
+ wmb(); /* complete write */
+ } else
+ done = 1;
+ }
+ spin_unlock(scrq->q_lock);
+ spin_unlock_irqrestore(vhost->host->host_lock, flags);
+}
+
+/**
+ * ibmvfc_interrupt_async_subq - Handle an async event from the adapter
+ * @irq: interrupt request
+ * @scrq_instance: async subq
+ *
+ **/
+static irqreturn_t __maybe_unused ibmvfc_interrupt_async_subq(int irq, void *scrq_instance)
+{
+ struct ibmvfc_queue *scrq = (struct ibmvfc_queue *)scrq_instance;
+
+ ibmvfc_toggle_scrq_irq(scrq, 0);
+ ibmvfc_drain_async_subq(scrq);
+
+ return IRQ_HANDLED;
+}
+
static void ibmvfc_drain_sub_crq(struct ibmvfc_queue *scrq)
{
struct ibmvfc_crq *crq;
--
2.55.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v9 5/9] scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ
2026-09-11 2:19 ` [PATCH v9 5/9] scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ Tyrel Datwyler
@ 2026-09-11 2:44 ` sashiko-bot
2026-09-11 3:53 ` Tyrel Datwyler
0 siblings, 1 reply; 23+ messages in thread
From: sashiko-bot @ 2026-09-11 2:44 UTC (permalink / raw)
To: Tyrel Datwyler; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Missing address-of operator (`&`) when referencing `Scsi_Host`'s `host_lock`, causing a fatal compilation error.
--
commit af3cb177b633d7c7bd704888c846393f549edaef
Author: Dave Marquardt <davemarq@linux.ibm.com>
Date: Fri Sep 11 02:19:05 2026 +0000
scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ
This commit adds an interrupt routine and a draining function for asynchronous
sub-CRQ events. It handles disabling and enabling IRQs to process entries
while holding the queue and host locks.
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index daec81f48857d..553fca31cb3aa 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -4376,6 +4376,60 @@ static struct ibmvfc_crq *ibmvfc_next_scrq(struct ibmvfc_queue *scrq)
> return crq;
> }
>
> +static void ibmvfc_drain_async_subq(struct ibmvfc_queue *scrq)
> +{
> + struct ibmvfc_host *vhost = scrq->vhost;
> + unsigned long flags;
> + struct ibmvfc_crq *crq;
> + int done = 0;
> +
> + spin_lock_irqsave(vhost->host->host_lock, flags);
[Severity: Low]
Does this code need an address-of operator for host_lock?
Since kernel 5.14, the Scsi_Host's host_lock field is an embedded spinlock_t
struct, so passing it directly to spin_lock_irqsave causes it to be passed
by value. This regression will result in a compilation error because the
lock function expects a pointer to a spinlock_t.
Would it make sense to also update ibmvfc_process_async_work, as well as
ibmvfc_async_fpin_test and ibmvfc_full_fpin_test in the KUnit tests, which
appear to have this same pattern introduced earlier in the series?
> + spin_lock(scrq->q_lock);
> + while (!done) {
[ ... ]
> + } else
> + done = 1;
> + }
> + spin_unlock(scrq->q_lock);
> + spin_unlock_irqrestore(vhost->host->host_lock, flags);
[Severity: Low]
Similar to the lock acquisition above, does spin_unlock_irqrestore also
need an address-of operator to pass a pointer to host_lock?
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260911021910.1038128-1-tyreld@linux.ibm.com?part=5
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v9 5/9] scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ
2026-09-11 2:44 ` sashiko-bot
@ 2026-09-11 3:53 ` Tyrel Datwyler
0 siblings, 0 replies; 23+ messages in thread
From: Tyrel Datwyler @ 2026-09-11 3:53 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-scsi
On 9/10/26 7:44 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] Missing address-of operator (`&`) when referencing `Scsi_Host`'s `host_lock`, causing a fatal compilation error.
> --
>
> commit af3cb177b633d7c7bd704888c846393f549edaef
> Author: Dave Marquardt <davemarq@linux.ibm.com>
> Date: Fri Sep 11 02:19:05 2026 +0000
>
> scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ
>
> This commit adds an interrupt routine and a draining function for asynchronous
> sub-CRQ events. It handles disabling and enabling IRQs to process entries
> while holding the queue and host locks.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index daec81f48857d..553fca31cb3aa 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>
> [ ... ]
>
>> @@ -4376,6 +4376,60 @@ static struct ibmvfc_crq *ibmvfc_next_scrq(struct ibmvfc_queue *scrq)
>> return crq;
>> }
>>
>> +static void ibmvfc_drain_async_subq(struct ibmvfc_queue *scrq)
>> +{
>> + struct ibmvfc_host *vhost = scrq->vhost;
>> + unsigned long flags;
>> + struct ibmvfc_crq *crq;
>> + int done = 0;
>> +
>> + spin_lock_irqsave(vhost->host->host_lock, flags);
>
> [Severity: Low]
> Does this code need an address-of operator for host_lock?
>
> Since kernel 5.14, the Scsi_Host's host_lock field is an embedded spinlock_t
> struct, so passing it directly to spin_lock_irqsave causes it to be passed
> by value. This regression will result in a compilation error because the
> lock function expects a pointer to a spinlock_t.
>
> Would it make sense to also update ibmvfc_process_async_work, as well as
> ibmvfc_async_fpin_test and ibmvfc_full_fpin_test in the KUnit tests, which
> appear to have this same pattern introduced earlier in the series?
>
>> + spin_lock(scrq->q_lock);
>> + while (!done) {
>
> [ ... ]
>
>> + } else
>> + done = 1;
>> + }
>> + spin_unlock(scrq->q_lock);
>> + spin_unlock_irqrestore(vhost->host->host_lock, flags);
>
> [Severity: Low]
> Similar to the lock acquisition above, does spin_unlock_irqrestore also
> need an address-of operator to pass a pointer to host_lock?
>
>> +}
>
Again, as noted in patch 1 this breaks the build. Will fix in v10.
-Tyrel
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v9 6/9] scsi: ibmvfc: extend channel reg/dereg helpers for async sub-CRQ
2026-09-11 2:19 [PATCH v9 0/9] scsi: ibmvfc: make ibmvfc support FPIN messages Tyrel Datwyler
` (4 preceding siblings ...)
2026-09-11 2:19 ` [PATCH v9 5/9] scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ Tyrel Datwyler
@ 2026-09-11 2:19 ` Tyrel Datwyler
2026-09-11 2:19 ` [PATCH v9 7/9] scsi: ibmvfc: fix IRQ leak and guard deregister on channel reg failure Tyrel Datwyler
` (2 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Tyrel Datwyler @ 2026-09-11 2:19 UTC (permalink / raw)
To: james.bottomley, martin.petersen
Cc: linux-scsi, linuxppc-dev, linux-kernel, brking, davemarq,
Tyrel Datwyler
From: Dave Marquardt <davemarq@linux.ibm.com>
ibmvfc_register_channel() and ibmvfc_deregister_channel() previously only
handled indexed sub-CRQ channels drawn from the channels->scrqs[] array.
The async sub-CRQ (vhost->async_sub_crq) had no registration path through
these helpers, requiring separate handling.
Extend both functions to accept a negative index as a sentinel value
signalling that the async sub-CRQ should be operated on instead of an
indexed scrq entry. When index < 0, the queue pointer is set to
&vhost->async_sub_crq, the IRQ is named "ibmvfc-<addr>-async", and the
handler is set to ibmvfc_interrupt_async_subq rather than the per-protocol
ibmvfc_interrupt_mq handler. hwq_id assignment is skipped for the async
queue since it has no meaningful hardware queue index.
Stopped marking ibmvfc_interrupt_async_subq as __maybe_unused.
Error messages in both paths are updated to distinguish async sub-CRQ
failures from indexed sub-CRQ failures. Kernel-doc headers are added to
both functions documenting the negative-index convention.
Signed-off-by: Dave Marquardt <davemarq@linux.ibm.com>
Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc-core.c | 93 ++++++++++++++++++++++-------
1 file changed, 70 insertions(+), 23 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
index 553fca31cb3a..fa312a58471f 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
@@ -4420,7 +4420,7 @@ static void ibmvfc_drain_async_subq(struct ibmvfc_queue *scrq)
* @scrq_instance: async subq
*
**/
-static irqreturn_t __maybe_unused ibmvfc_interrupt_async_subq(int irq, void *scrq_instance)
+static irqreturn_t ibmvfc_interrupt_async_subq(int irq, void *scrq_instance)
{
struct ibmvfc_queue *scrq = (struct ibmvfc_queue *)scrq_instance;
@@ -6811,13 +6811,29 @@ static int ibmvfc_init_crq(struct ibmvfc_host *vhost)
return retrc;
}
+/**
+ * ibmvfc_register_channel - Register a sub-CRQ channel with the hypervisor
+ * @vhost: ibmvfc host struct
+ * @channels: ibmvfc channels struct containing the channel array and protocol
+ * @index: index into the channels array for the queue to register, or
+ * a negative value to register the async sub-CRQ
+ *
+ * Register a sub-CRQ with the hypervisor via h_reg_sub_crq, map its hardware
+ * IRQ to a Linux IRQ, and bind an interrupt handler to it. The handler is
+ * selected based on the channel protocol (SCSI or NVMe) for normal queues, or
+ * set to the async sub-CRQ handler when @index is negative.
+ *
+ * Return value:
+ * 0 on success / non-zero on failure
+ **/
static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
struct ibmvfc_channels *channels,
int index)
{
struct device *dev = vhost->dev;
struct vio_dev *vdev = to_vio_dev(dev);
- struct ibmvfc_queue *scrq = &channels->scrqs[index];
+ bool is_async = index < 0;
+ struct ibmvfc_queue *scrq = !is_async ? &channels->scrqs[index] : &vhost->async_sub_crq;
int rc = -ENOMEM;
ENTER;
@@ -6837,36 +6853,49 @@ static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
if (!scrq->irq) {
rc = -EINVAL;
- dev_err(dev, "Error mapping sub-crq[%d] irq\n", index);
+ if (!is_async)
+ dev_err(dev, "Error mapping sub-crq[%d] irq\n", index);
+ else
+ dev_err(dev, "Error mapping async sub-crq irq\n");
goto irq_failed;
}
- switch (channels->protocol) {
- case IBMVFC_PROTO_SCSI:
- snprintf(scrq->name, sizeof(scrq->name), "ibmvfc-%x-scsi%d",
- vdev->unit_address, index);
- scrq->handler = ibmvfc_interrupt_mq;
- break;
- case IBMVFC_PROTO_NVME:
- snprintf(scrq->name, sizeof(scrq->name), "ibmvfc-%x-nvmf%d",
- vdev->unit_address, index);
- scrq->handler = ibmvfc_interrupt_mq;
- break;
- default:
- dev_err(dev, "Unknown channel protocol (%d)\n",
- channels->protocol);
- goto irq_failed;
+ if (!is_async) {
+ switch (channels->protocol) {
+ case IBMVFC_PROTO_SCSI:
+ snprintf(scrq->name, sizeof(scrq->name), "ibmvfc-%x-scsi%d",
+ vdev->unit_address, index);
+ scrq->handler = ibmvfc_interrupt_mq;
+ break;
+ case IBMVFC_PROTO_NVME:
+ snprintf(scrq->name, sizeof(scrq->name), "ibmvfc-%x-nvmf%d",
+ vdev->unit_address, index);
+ scrq->handler = ibmvfc_interrupt_mq;
+ break;
+ default:
+ dev_err(dev, "Unknown channel protocol (%d)\n",
+ channels->protocol);
+ goto irq_failed;
+ }
+ } else {
+ snprintf(scrq->name, sizeof(scrq->name), "ibmvfc-%x-async",
+ vdev->unit_address);
+ scrq->handler = ibmvfc_interrupt_async_subq;
}
rc = request_irq(scrq->irq, scrq->handler, 0, scrq->name, scrq);
if (rc) {
- dev_err(dev, "Couldn't register sub-crq[%d] irq\n", index);
+ if (!is_async)
+ dev_err(dev, "Couldn't register sub-crq[%d] irq\n", index);
+ else
+ dev_err(dev, "Couldn't register async sub-crq irq\n");
irq_dispose_mapping(scrq->irq);
goto irq_failed;
}
- scrq->hwq_id = index;
+ if (!is_async)
+ scrq->hwq_id = index;
LEAVE;
return 0;
@@ -6880,13 +6909,26 @@ static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
return rc;
}
+/**
+ * ibmvfc_deregister_channel - Deregister a sub-CRQ channel with the hypervisor
+ * @vhost: ibmvfc host struct
+ * @channels: ibmvfc channels struct containing the sub-CRQ array
+ * @index: index into the sub-CRQ array, or -1 to deregister the
+ * asynchronous sub-CRQ
+ *
+ * Frees the IRQ, disposes of the IRQ mapping, and calls H_FREE_SUB_CRQ to
+ * release the sub-CRQ with the hypervisor. On success the queue message
+ * buffer is zeroed and the current index is reset. If H_FREE_SUB_CRQ fails,
+ * an error is logged but the channel resources are cleaned up regardless.
+ */
static void ibmvfc_deregister_channel(struct ibmvfc_host *vhost,
struct ibmvfc_channels *channels,
int index)
{
struct device *dev = vhost->dev;
struct vio_dev *vdev = to_vio_dev(dev);
- struct ibmvfc_queue *scrq = &channels->scrqs[index];
+ bool is_async = index < 0;
+ struct ibmvfc_queue *scrq = !is_async ? &channels->scrqs[index] : &vhost->async_sub_crq;
long rc;
ENTER;
@@ -6900,8 +6942,13 @@ static void ibmvfc_deregister_channel(struct ibmvfc_host *vhost,
scrq->cookie);
} while (rc == H_BUSY || H_IS_LONG_BUSY(rc));
- if (rc)
- dev_err(dev, "Failed to free sub-crq[%d]: rc=%ld\n", index, rc);
+ if (rc) {
+ if (!is_async)
+ dev_err(dev, "Failed to free sub-crq[%d]: rc=%ld\n",
+ index, rc);
+ else
+ dev_err(dev, "Failed to free async sub-crq: rc=%ld\n", rc);
+ }
/* Clean out the queue */
memset(scrq->msgs.crq, 0, PAGE_SIZE);
--
2.55.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v9 7/9] scsi: ibmvfc: fix IRQ leak and guard deregister on channel reg failure
2026-09-11 2:19 [PATCH v9 0/9] scsi: ibmvfc: make ibmvfc support FPIN messages Tyrel Datwyler
` (5 preceding siblings ...)
2026-09-11 2:19 ` [PATCH v9 6/9] scsi: ibmvfc: extend channel reg/dereg helpers for async sub-CRQ Tyrel Datwyler
@ 2026-09-11 2:19 ` Tyrel Datwyler
2026-09-11 2:34 ` sashiko-bot
2026-09-11 2:19 ` [PATCH v9 8/9] scsi: ibmvfc: register and use asynchronous sub CRQ for events Tyrel Datwyler
2026-09-11 2:19 ` [PATCH v9 9/9] scsi: ibmvfc: handle extended FPIN events Tyrel Datwyler
8 siblings, 1 reply; 23+ messages in thread
From: Tyrel Datwyler @ 2026-09-11 2:19 UTC (permalink / raw)
To: james.bottomley, martin.petersen
Cc: linux-scsi, linuxppc-dev, linux-kernel, brking, davemarq,
Tyrel Datwyler
From: Dave Marquardt <davemarq@linux.ibm.com>
If request_irq() fails during ibmvfc_register_channel(), the error path
disposes of the IRQ mapping via irq_dispose_mapping() but leaves scrq->irq
populated with the stale virq number. A subsequent call to
ibmvfc_deregister_channel() during teardown or reset cleanup then
unconditionally invokes free_irq() and irq_dispose_mapping() on the stale
IRQ descriptor, triggering kernel warnings.
Fix this by:
1. Explicitly setting scrq->irq to 0 after irq_dispose_mapping() in the
ibmvfc_register_channel() error path.
2. Guarding free_irq() and irq_dispose_mapping() with a check for scrq->irq
in ibmvfc_deregister_channel() so unmapped or unregistered queues are
safely skipped.
Signed-off-by: Dave Marquardt <davemarq@linux.ibm.com>
Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc-core.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
index fa312a58471f..8eeb125ee13b 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
@@ -6891,6 +6891,7 @@ static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
else
dev_err(dev, "Couldn't register async sub-crq irq\n");
irq_dispose_mapping(scrq->irq);
+ scrq->irq = 0;
goto irq_failed;
}
@@ -6933,9 +6934,11 @@ static void ibmvfc_deregister_channel(struct ibmvfc_host *vhost,
ENTER;
- free_irq(scrq->irq, scrq);
- irq_dispose_mapping(scrq->irq);
- scrq->irq = 0;
+ if (scrq->irq) {
+ free_irq(scrq->irq, scrq);
+ irq_dispose_mapping(scrq->irq);
+ scrq->irq = 0;
+ }
do {
rc = plpar_hcall_norets(H_FREE_SUB_CRQ, vdev->unit_address,
--
2.55.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v9 7/9] scsi: ibmvfc: fix IRQ leak and guard deregister on channel reg failure
2026-09-11 2:19 ` [PATCH v9 7/9] scsi: ibmvfc: fix IRQ leak and guard deregister on channel reg failure Tyrel Datwyler
@ 2026-09-11 2:34 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-09-11 2:34 UTC (permalink / raw)
To: Tyrel Datwyler; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Error code from request_irq failure is swallowed, causing initialization functions to falsely return success.
- [High] Unconditional execution of H_FREE_SUB_CRQ and failure to clear `scrq->cookie` leads to hypervisor double-free and use-after-free of sub-CRQs.
--
commit 2c9e0fcaebe0c140397143556c20f5137db7357d
Author: Dave Marquardt <davemarq@linux.ibm.com>
scsi: ibmvfc: fix IRQ leak and guard deregister on channel reg failure
This commit fixes an IRQ leak where the error path leaves scrq->irq
populated, causing a subsequent deregister call to double-free the IRQ.
It explicitly sets scrq->irq to 0 on failure and guards the teardown
process against unregistered queues.
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index fa312a58471fa..8eeb125ee13b8 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -6891,6 +6891,7 @@ static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
> else
> dev_err(dev, "Couldn't register async sub-crq irq\n");
> irq_dispose_mapping(scrq->irq);
> + scrq->irq = 0;
> goto irq_failed;
> }
>
> @@ -6933,9 +6934,11 @@ static void ibmvfc_deregister_channel(struct ibmvfc_host *vhost,
>
> ENTER;
>
> - free_irq(scrq->irq, scrq);
> - irq_dispose_mapping(scrq->irq);
> - scrq->irq = 0;
> + if (scrq->irq) {
> + free_irq(scrq->irq, scrq);
> + irq_dispose_mapping(scrq->irq);
> + scrq->irq = 0;
> + }
>
> do {
> rc = plpar_hcall_norets(H_FREE_SUB_CRQ, vdev->unit_address,
[Severity: High]
This is a pre-existing issue, but by clearing the irq here, should we also be
clearing scrq->cookie?
If ibmvfc_reg_sub_crqs() fails, it triggers a rollback that calls
ibmvfc_deregister_channel(), which executes H_FREE_SUB_CRQ but leaves
scrq->cookie populated.
Since ibmvfc_dereg_sub_crqs() unconditionally loops over channels->max_queues
and calls H_FREE_SUB_CRQ on everything during teardown, couldn't this lead to
passing stale cookies to the hypervisor?
If those cookies were reused for NVMe queues, would this destroy active queues
and cause use-after-free corruption?
Similarly, in ibmvfc_register_channel(), if request_irq() fails, the error
path executes H_FREE_SUB_CRQ without clearing scrq->cookie:
irq_failed:
do {
hcall_rc = plpar_hcall_norets(H_FREE_SUB_CRQ, vdev->unit_address, scrq->cookie);
} while (hcall_rc == H_BUSY || H_IS_LONG_BUSY(hcall_rc));
reg_failed:
[Severity: High]
This is a pre-existing issue, but while reviewing the initialization code,
does ibmvfc_init_crq() swallow the error code from request_irq() failures?
In ibmvfc_init_crq(), retrc is set to 0 upon successful CRQ registration:
retrc = 0;
...
if ((rc = request_irq(vdev->irq, ibmvfc_interrupt, 0, IBMVFC_NAME, vhost))) {
dev_err(dev, "Couldn't register irq 0x%x. rc=%d\n", vdev->irq, rc);
goto req_irq_failed;
}
...
req_irq_failed:
tasklet_kill(&vhost->tasklet);
do {
rc = plpar_hcall_norets(H_FREE_CRQ, vdev->unit_address);
} while (rc == H_BUSY || H_IS_LONG_BUSY(rc));
reg_crq_failed:
ibmvfc_free_queue(vhost, crq);
return retrc;
Since retrc is already 0, the error path unconditionally returns success
instead of propagating rc.
Could this cause the probe function to consider the adapter successfully
initialized, even though it lacks a functional IRQ handler?
Would this lead to a silent hang during IO operations since no completion
interrupts will be processed?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260911021910.1038128-1-tyreld@linux.ibm.com?part=7
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v9 8/9] scsi: ibmvfc: register and use asynchronous sub CRQ for events
2026-09-11 2:19 [PATCH v9 0/9] scsi: ibmvfc: make ibmvfc support FPIN messages Tyrel Datwyler
` (6 preceding siblings ...)
2026-09-11 2:19 ` [PATCH v9 7/9] scsi: ibmvfc: fix IRQ leak and guard deregister on channel reg failure Tyrel Datwyler
@ 2026-09-11 2:19 ` Tyrel Datwyler
2026-09-11 2:40 ` sashiko-bot
2026-09-11 2:19 ` [PATCH v9 9/9] scsi: ibmvfc: handle extended FPIN events Tyrel Datwyler
8 siblings, 1 reply; 23+ messages in thread
From: Tyrel Datwyler @ 2026-09-11 2:19 UTC (permalink / raw)
To: james.bottomley, martin.petersen
Cc: linux-scsi, linuxppc-dev, linux-kernel, brking, davemarq,
Tyrel Datwyler
From: Dave Marquardt <davemarq@linux.ibm.com>
Wire the async sub-CRQ into the sub-CRQ lifecycle so it is allocated,
registered, and freed alongside the SCSI channel queues.
In ibmvfc_init_sub_crqs(), allocate the async sub-CRQ queue buffer via
ibmvfc_alloc_queue() before allocating the SCSI channels. Register it
with the VIOS by calling ibmvfc_register_channel() with index -1 (the
negative-index sentinel introduced in the previous commit). Either
failure disables multi-queue and aborts init.
In ibmvfc_release_sub_crqs(), ibmvfc_reenable_crq_queue(), and
ibmvfc_reset_crq(), deregister and re-register the async sub-CRQ
alongside the SCSI channel queues.
In ibmvfc_channel_setup_done(), capture the async sub-CRQ handle
returned by the VIOS in the channel setup response and store it in
vhost->async_sub_crq.vios_cookie.
In ibmvfc_set_login_info(), advertise IBMVFC_USE_ASYNC_SUBQ,
IBMVFC_CAN_HANDLE_FPIN, and IBMVFC_YES_SCSI capabilities whenever
multi-queue channels are enabled. IBMVFC_YES_SCSI was previously only
set for NVMe-enabled configurations; move it to the common multi-queue
path so it is always advertised when channels are in use.
Fix a variable-shadowing bug in ibmvfc_register_channel() where the
irq_failed cleanup loop reused rc for the H_FREE_SUB_CRQ hcall result,
clobbering the error code returned to the caller. Introduce hcall_rc
for the cleanup loop instead.
Guard the memset() in ibmvfc_deregister_channel() behind a check of
scrq->msgs.handle to prevent a NULL dereference when the function is
called on a queue that was never allocated, such as async_sub_crq when
multi-queue is disabled or initialization failed before
ibmvfc_alloc_queue() was reached.
Signed-off-by: Dave Marquardt <davemarq@linux.ibm.com>
Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc-core.c | 51 +++++++++++++++++++++++++----
1 file changed, 44 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
index 8eeb125ee13b..6eb299989724 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
@@ -192,6 +192,8 @@ static void ibmvfc_tgt_move_login(struct ibmvfc_target *);
static void ibmvfc_dereg_sub_crqs(struct ibmvfc_host *, struct ibmvfc_channels *);
static void ibmvfc_reg_sub_crqs(struct ibmvfc_host *, struct ibmvfc_channels *);
+static void ibmvfc_deregister_channel(struct ibmvfc_host *, struct ibmvfc_channels *, int);
+static int ibmvfc_register_channel(struct ibmvfc_host *, struct ibmvfc_channels *, int);
static const char *unknown_error = "unknown error";
@@ -964,6 +966,7 @@ static int ibmvfc_reenable_crq_queue(struct ibmvfc_host *vhost)
struct vio_dev *vdev = to_vio_dev(vhost->dev);
unsigned long flags;
+ ibmvfc_deregister_channel(vhost, &vhost->scsi_scrqs, -1);
ibmvfc_dereg_sub_crqs(vhost, &vhost->scsi_scrqs);
ibmvfc_dereg_sub_crqs(vhost, &vhost->nvme_scrqs);
@@ -986,6 +989,7 @@ static int ibmvfc_reenable_crq_queue(struct ibmvfc_host *vhost)
spin_unlock(vhost->crq.q_lock);
spin_unlock_irqrestore(&vhost->host->host_lock, flags);
+ ibmvfc_register_channel(vhost, &vhost->scsi_scrqs, -1);
ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs);
@@ -1006,6 +1010,7 @@ static int ibmvfc_reset_crq(struct ibmvfc_host *vhost)
struct vio_dev *vdev = to_vio_dev(vhost->dev);
struct ibmvfc_queue *crq = &vhost->crq;
+ ibmvfc_deregister_channel(vhost, &vhost->scsi_scrqs, -1);
ibmvfc_dereg_sub_crqs(vhost, &vhost->scsi_scrqs);
ibmvfc_dereg_sub_crqs(vhost, &vhost->nvme_scrqs);
@@ -1042,6 +1047,7 @@ static int ibmvfc_reset_crq(struct ibmvfc_host *vhost)
spin_unlock(vhost->crq.q_lock);
spin_unlock_irqrestore(&vhost->host->host_lock, flags);
+ ibmvfc_register_channel(vhost, &vhost->scsi_scrqs, -1);
ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs);
@@ -1584,9 +1590,11 @@ static void ibmvfc_set_login_info(struct ibmvfc_host *vhost)
if (vhost->mq_enabled || vhost->using_channels) {
login_info->capabilities |= cpu_to_be64(IBMVFC_CAN_USE_CHANNELS);
+ login_info->capabilities |= cpu_to_be64(IBMVFC_USE_ASYNC_SUBQ);
+ login_info->capabilities |= cpu_to_be64(IBMVFC_CAN_HANDLE_FPIN);
+ login_info->capabilities |= cpu_to_be64(IBMVFC_YES_SCSI);
if (vhost->nvme_enabled) {
login_info->capabilities |= cpu_to_be64(IBMVFC_YES_NVMEOF);
- login_info->capabilities |= cpu_to_be64(IBMVFC_YES_SCSI);
login_info->capabilities |= cpu_to_be64(IBMVFC_CAN_USE_WWPN_ALL);
}
}
@@ -5812,6 +5820,7 @@ static void ibmvfc_channel_setup_done(struct ibmvfc_event *evt)
for (i = 0; i < nvme->active_queues; i++)
nvme->scrqs[i].vios_cookie =
be64_to_cpu(setup->channel_handles[scsi->active_queues + i]);
+ vhost->async_sub_crq.vios_cookie = be64_to_cpu(setup->async_sub_crq_handle);
ibmvfc_dbg(vhost, "Using %u SCSI channels\n",
scsi->active_queues);
@@ -5871,6 +5880,7 @@ static void ibmvfc_channel_setup(struct ibmvfc_host *vhost)
for (i = 0; i < nvme_channels; i++)
setup_buf->channel_handles[scsi_channels + i] =
cpu_to_be64(nvme->scrqs[i].cookie);
+ setup_buf->async_sub_crq_handle = cpu_to_be64(vhost->async_sub_crq.cookie);
}
ibmvfc_init_event(evt, ibmvfc_channel_setup_done, IBMVFC_MAD_FORMAT);
@@ -6835,6 +6845,7 @@ static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
bool is_async = index < 0;
struct ibmvfc_queue *scrq = !is_async ? &channels->scrqs[index] : &vhost->async_sub_crq;
int rc = -ENOMEM;
+ int hcall_rc;
ENTER;
@@ -6903,8 +6914,8 @@ static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
irq_failed:
do {
- rc = plpar_hcall_norets(H_FREE_SUB_CRQ, vdev->unit_address, scrq->cookie);
- } while (rc == H_BUSY || H_IS_LONG_BUSY(rc));
+ hcall_rc = plpar_hcall_norets(H_FREE_SUB_CRQ, vdev->unit_address, scrq->cookie);
+ } while (hcall_rc == H_BUSY || H_IS_LONG_BUSY(hcall_rc));
reg_failed:
LEAVE;
return rc;
@@ -6954,8 +6965,10 @@ static void ibmvfc_deregister_channel(struct ibmvfc_host *vhost,
}
/* Clean out the queue */
- memset(scrq->msgs.crq, 0, PAGE_SIZE);
- scrq->cur = 0;
+ if (scrq->msgs.handle) {
+ memset(scrq->msgs.crq, 0, PAGE_SIZE);
+ scrq->cur = 0;
+ }
LEAVE;
}
@@ -6972,7 +6985,9 @@ static void ibmvfc_reg_sub_crqs(struct ibmvfc_host *vhost,
for (i = 0; i < channels->max_queues; i++) {
if (ibmvfc_register_channel(vhost, channels, i)) {
for (j = i; j > 0; j--)
- ibmvfc_deregister_channel(vhost, channels, j - 1);
+ ibmvfc_deregister_channel(
+ vhost, channels, j - 1);
+
vhost->do_enquiry = 0;
return;
}
@@ -7027,16 +7042,26 @@ static int ibmvfc_alloc_channels(struct ibmvfc_host *vhost,
static void ibmvfc_init_sub_crqs(struct ibmvfc_host *vhost)
{
+ int rc = 0;
+
ENTER;
if (!vhost->mq_enabled)
return;
- if (ibmvfc_alloc_channels(vhost, &vhost->scsi_scrqs)) {
+ rc = ibmvfc_alloc_queue(vhost, &vhost->async_sub_crq, IBMVFC_SUB_CRQ_FMT);
+ if (rc) {
vhost->do_enquiry = 0;
vhost->mq_enabled = 0;
return;
}
+ /* register async_sub_crq channel */
+ if (ibmvfc_register_channel(vhost, &vhost->scsi_scrqs, -1))
+ goto free_async_sub_crq;
+
+ if (ibmvfc_alloc_channels(vhost, &vhost->scsi_scrqs))
+ goto deregister_async_sub_crq;
+
ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
if (vhost->nvme_enabled) {
@@ -7047,6 +7072,15 @@ static void ibmvfc_init_sub_crqs(struct ibmvfc_host *vhost)
}
LEAVE;
+ return;
+
+ deregister_async_sub_crq:
+ ibmvfc_deregister_channel(vhost, &vhost->scsi_scrqs, -1);
+free_async_sub_crq:
+ ibmvfc_free_queue(vhost, &vhost->async_sub_crq);
+ vhost->do_enquiry = 0;
+ vhost->mq_enabled = 0;
+ return;
}
static void ibmvfc_release_channels(struct ibmvfc_host *vhost,
@@ -7070,6 +7104,9 @@ static void ibmvfc_release_channels(struct ibmvfc_host *vhost,
static void ibmvfc_release_sub_crqs(struct ibmvfc_host *vhost)
{
ENTER;
+ ibmvfc_deregister_channel(vhost, &vhost->scsi_scrqs, -1);
+ ibmvfc_free_queue(vhost, &vhost->async_sub_crq);
+
if (!vhost->scsi_scrqs.scrqs)
return;
--
2.55.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v9 8/9] scsi: ibmvfc: register and use asynchronous sub CRQ for events
2026-09-11 2:19 ` [PATCH v9 8/9] scsi: ibmvfc: register and use asynchronous sub CRQ for events Tyrel Datwyler
@ 2026-09-11 2:40 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-09-11 2:40 UTC (permalink / raw)
To: Tyrel Datwyler; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Critical] Unconditional lifecycle management of `async_sub_crq` causes Use-After-Free, double-free, and DMA corruption via uninitialized addresses.
Pre-existing issues:
- [Critical] Missing bounds check on VIOS-provided queue counts leads to out-of-bounds heap writes.
--
commit ba224804598ce14d90bf10ea7c6101dfc09e9780
Author: Dave Marquardt <davemarq@linux.ibm.com>
scsi: ibmvfc: register and use asynchronous sub CRQ for events
This patch wires the async sub-CRQ into the sub-CRQ lifecycle so it is
allocated, registered, and freed alongside the SCSI channel queues. It also
fixes a variable-shadowing bug in channel registration and advertises
capabilities when multi-queue channels are enabled.
> 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
[ ... ]
> @@ -1006,6 +1010,7 @@ static int ibmvfc_reset_crq(struct ibmvfc_host *vhost)
> struct vio_dev *vdev = to_vio_dev(vhost->dev);
> struct ibmvfc_queue *crq = &vhost->crq;
>
> + ibmvfc_deregister_channel(vhost, &vhost->scsi_scrqs, -1);
> ibmvfc_dereg_sub_crqs(vhost, &vhost->scsi_scrqs);
> ibmvfc_dereg_sub_crqs(vhost, &vhost->nvme_scrqs);
[ ... ]
> @@ -1042,6 +1047,7 @@ static int ibmvfc_reset_crq(struct ibmvfc_host *vhost)
> spin_unlock(vhost->crq.q_lock);
> spin_unlock_irqrestore(&vhost->host->host_lock, flags);
>
> + ibmvfc_register_channel(vhost, &vhost->scsi_scrqs, -1);
[Severity: Critical]
Does this code unconditionally register the async sub-CRQ during a host reset,
even if multi-queue is disabled or the queue was never allocated?
If a system boots with multi-queue disabled (or if its initialization fails),
the async_sub_crq remains unallocated. When a host reset occurs, this call
reaches h_reg_sub_crq() with an uninitialized msg_token (value 0):
ibmvfc_register_channel() {
...
rc = h_reg_sub_crq(vdev->unit_address, scrq->msg_token, PAGE_SIZE,
&scrq->cookie, &scrq->hw_irq);
...
}
Could this instruct the hypervisor to map and write to guest physical address 0,
causing DMA corruption?
> ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
> ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs);
[ ... ]
> @@ -5812,6 +5820,7 @@ static void ibmvfc_channel_setup_done(struct ibmvfc_event *evt)
> for (i = 0; i < nvme->active_queues; i++)
> nvme->scrqs[i].vios_cookie =
> be64_to_cpu(setup->channel_handles[scsi->active_queues + i]);
> + vhost->async_sub_crq.vios_cookie = be64_to_cpu(setup->async_sub_crq_handle);
[Severity: Critical]
This isn't a bug introduced by this patch, but is it possible for the
untrusted VIOS input in the channel setup response to overflow the queues
array?
In ibmvfc_channel_setup_done(), the driver sets the loop boundaries by reading
active_queues from setup->num_scsi_subq_channels and
setup->num_nvme_subq_channels. If the VIOS returns a count larger than
the array capacity (max_queues), does this loop blindly write
setup->channel_handles[i] past the end of the scrqs array, potentially
causing out-of-bounds writes to the kernel heap?
>
> ibmvfc_dbg(vhost, "Using %u SCSI channels\n",
> scsi->active_queues);
[ ... ]
> @@ -7070,6 +7104,9 @@ static void ibmvfc_release_channels(struct ibmvfc_host *vhost,
> static void ibmvfc_release_sub_crqs(struct ibmvfc_host *vhost)
> {
> ENTER;
> + ibmvfc_deregister_channel(vhost, &vhost->scsi_scrqs, -1);
> + ibmvfc_free_queue(vhost, &vhost->async_sub_crq);
> +
[Severity: Critical]
If ibmvfc_alloc_channels() fails during ibmvfc_init_sub_crqs(), the
async_sub_crq is freed but its state is not cleared. When the driver
later unloads or releases queues, does this unconditional call free the
queue a second time?
Could this lead to a double free of the event pool array and a use-after-free
on the event queue list?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260911021910.1038128-1-tyreld@linux.ibm.com?part=8
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v9 9/9] scsi: ibmvfc: handle extended FPIN events
2026-09-11 2:19 [PATCH v9 0/9] scsi: ibmvfc: make ibmvfc support FPIN messages Tyrel Datwyler
` (7 preceding siblings ...)
2026-09-11 2:19 ` [PATCH v9 8/9] scsi: ibmvfc: register and use asynchronous sub CRQ for events Tyrel Datwyler
@ 2026-09-11 2:19 ` Tyrel Datwyler
2026-09-11 2:40 ` sashiko-bot
8 siblings, 1 reply; 23+ messages in thread
From: Tyrel Datwyler @ 2026-09-11 2:19 UTC (permalink / raw)
To: james.bottomley, martin.petersen
Cc: linux-scsi, linuxppc-dev, linux-kernel, brking, davemarq,
Tyrel Datwyler
From: Dave Marquardt <davemarq@linux.ibm.com>
Implement support for extended FPIN messages received via the
asynchronous sub-queue, completing full FPIN functionality.
Extended FPIN messages provide more detailed information about fabric
events compared to basic FPIN messages, including specific event types,
modifiers, thresholds, and event counts.
Add ibmvfc_ext_fpin_to_desc() to convert extended FPIN messages from
async sub-queue format to fc_els_fpin structures with complete descriptor
information. Update ibmvfc_process_async_work() and ibmvfc_handle_async()
to zero node_name and dispatch to ibmvfc_ext_fpin_to_desc() when the
IBMVFC_ASYNC_IS_FPIN_EXT flag is set.
Set the IBMVFC_CAN_HANDLE_FPIN_EXT capability bit during login to inform
VIOS that the client can process extended FPIN messages. Also add the
IBMVFC_SUPPORT_FPIN_EXT response capability bit definition used to gate
extended FPIN processing.
Add KUnit tests to exercise extended FPIN event handling across all
FPIN status and event type combinations.
struct ibmvfc_async_subq_fpin uses volatile on its valid and wwpn fields,
consistent with the existing ibmvfc_crq, ibmvfc_async_crq, and
ibmvfc_async_sub_crq structs in ibmvfc.h, all of which mark their
hardware-owned ring-buffer fields volatile to prevent the compiler from
caching or eliminating reads of memory written directly by the VIOS.
Signed-off-by: Dave Marquardt <davemarq@linux.ibm.com>
Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc-core.c | 67 ++++++++++++-
drivers/scsi/ibmvscsi/ibmvfc.h | 30 ++++++
drivers/scsi/ibmvscsi/ibmvfc_kunit.c | 135 +++++++++++++++++++++++++++
3 files changed, 228 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
index 6eb299989724..bd10c3fd5ccf 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
@@ -1593,6 +1593,7 @@ static void ibmvfc_set_login_info(struct ibmvfc_host *vhost)
login_info->capabilities |= cpu_to_be64(IBMVFC_USE_ASYNC_SUBQ);
login_info->capabilities |= cpu_to_be64(IBMVFC_CAN_HANDLE_FPIN);
login_info->capabilities |= cpu_to_be64(IBMVFC_YES_SCSI);
+ login_info->capabilities |= cpu_to_be64(IBMVFC_CAN_HANDLE_FPIN_EXT);
if (vhost->nvme_enabled) {
login_info->capabilities |= cpu_to_be64(IBMVFC_YES_NVMEOF);
login_info->capabilities |= cpu_to_be64(IBMVFC_CAN_USE_WWPN_ALL);
@@ -3473,6 +3474,41 @@ ibmvfc_full_fpin_to_desc(struct ibmvfc_async_sub_crq *ibmvfc_fpin)
cpu_to_be32(1));
}
+/**
+ * ibmvfc_ext_fpin_to_desc(): allocate and populate a struct fc_els_fpin struct
+ * containing a descriptor.
+ * @ibmvfc_fpin: Pointer to async subq FPIN data
+ *
+ * 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_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);
+}
+
/**
* ibmvfc_find_target - Search for a target in a target list
* @target_list: list head of targets to search
@@ -3511,6 +3547,7 @@ static struct ibmvfc_target *ibmvfc_find_target(struct list_head *target_list,
static void ibmvfc_process_async_work(struct work_struct *work)
{
struct ibmvfc_async_sub_crq *subq = NULL;
+ struct ibmvfc_async_subq_fpin *sqfpin;
struct ibmvfc_async_work *aw;
struct ibmvfc_async_crq *crq = NULL;
struct ibmvfc_target *tgt;
@@ -3527,7 +3564,10 @@ static void ibmvfc_process_async_work(struct work_struct *work)
subq = &aw->event.subq;
scsi_id = 0;
wwpn = subq->wwpn;
- node_name = (subq->flags & IBMVFC_ASYNC_ID_IS_ASSOC_ID) ? 0 : subq->id.node_name;
+ if (subq->flags & (IBMVFC_ASYNC_IS_FPIN_EXT | IBMVFC_ASYNC_ID_IS_ASSOC_ID))
+ node_name = 0;
+ else
+ node_name = subq->id.node_name;
} else {
crq = &aw->event.async_crq;
scsi_id = crq->scsi_id;
@@ -3556,8 +3596,24 @@ static void ibmvfc_process_async_work(struct work_struct *work)
if (crq)
fpin = ibmvfc_basic_fpin_to_desc(crq, tgt->wwpn);
- else
- fpin = ibmvfc_full_fpin_to_desc(subq);
+ else {
+ static_assert(sizeof(struct ibmvfc_async_subq_fpin) ==
+ sizeof(struct ibmvfc_async_sub_crq));
+ static_assert(offsetof(struct ibmvfc_async_subq_fpin, fpin_data) ==
+ offsetof(struct ibmvfc_async_sub_crq, nport_id));
+ sqfpin = (struct ibmvfc_async_subq_fpin *)subq;
+ if ((subq->flags & IBMVFC_ASYNC_IS_FPIN_EXT) == 0) {
+ fpin = ibmvfc_full_fpin_to_desc(subq);
+ } else if (!(sqfpin->fpin_data.flags & IBMVFC_FPIN_EVENT_TYPE_VALID)) {
+ dev_err_ratelimited(vhost->dev,
+ "Invalid extended FPIN event received\n");
+ } else if (!ibmvfc_check_caps(vhost, IBMVFC_SUPPORT_FPIN_EXT)) {
+ dev_err_ratelimited(vhost->dev,
+ "Unexpected extended FPIN event received\n");
+ } else {
+ fpin = ibmvfc_ext_fpin_to_desc(sqfpin);
+ }
+ }
if (fpin) {
fc_host_fpin_rcv(tgt->vhost->host,
@@ -3599,7 +3655,10 @@ VISIBLE_IF_KUNIT void ibmvfc_handle_async(struct ibmvfc_async_crq_event *ae,
link_state = subq->link_state;
scsi_id = 0;
wwpn = subq->wwpn;
- node_name = subq->flags & IBMVFC_ASYNC_ID_IS_ASSOC_ID ? 0 : subq->id.node_name;
+ if (subq->flags & (IBMVFC_ASYNC_IS_FPIN_EXT | IBMVFC_ASYNC_ID_IS_ASSOC_ID))
+ node_name = 0;
+ else
+ node_name = subq->id.node_name;
} else {
async_crq = &ae->async_crq;
event = be64_to_cpu(async_crq->event);
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
index 6fce7a4922bb..20933af59d48 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.h
+++ b/drivers/scsi/ibmvscsi/ibmvfc.h
@@ -210,6 +210,7 @@ struct ibmvfc_npiv_login {
#define IBMVFC_CAN_USE_WWPN_ALL 0x080
#define IBMVFC_USE_ASYNC_SUBQ 0x100
#define IBMVFC_CAN_USE_NOOP_CMD 0x200
+#define IBMVFC_CAN_HANDLE_FPIN_EXT 0x800
__be64 node_name;
struct srp_direct_buf async;
u8 partition_name[IBMVFC_MAX_NAME];
@@ -261,6 +262,7 @@ struct ibmvfc_npiv_login_resp {
#define IBMVFC_SUPPORT_WWPN_ALL 0x0400
#define IBMVFC_ASYNC_SUBQ 0x0800
#define IBMVFC_SUPPORT_NOOP_CMD 0x1000
+#define IBMVFC_SUPPORT_FPIN_EXT 0x2000
__be32 max_cmds;
__be32 scsi_id_sz;
__be64 max_dma_len;
@@ -786,6 +788,34 @@ struct ibmvfc_async_sub_crq {
} id;
} __packed __aligned(8);
+struct ibmvfc_fpin_data {
+#define IBMVFC_FPIN_EVENT_TYPE_VALID 0x01
+#define IBMVFC_FPIN_MODIFIER_VALID 0x02
+#define IBMVFC_FPIN_THRESHOLD_VALID 0x04
+#define IBMVFC_FPIN_SEVERITY_VALID 0x08
+#define IBMVFC_FPIN_EVENT_COUNT_VALID 0x10
+ u8 flags;
+ u8 reserved[3];
+ __be16 event_type;
+ __be16 event_type_modifier;
+ __be32 event_threshold;
+ union {
+ u8 severity;
+ __be32 event_count;
+ } event_data;
+} __packed __aligned(8);
+
+struct ibmvfc_async_subq_fpin {
+ volatile u8 valid;
+ u8 flags;
+ u8 link_state;
+ u8 fpin_status;
+ __be16 event;
+ __be16 pad;
+ volatile __be64 wwpn;
+ struct ibmvfc_fpin_data fpin_data;
+} __packed __aligned(8);
+
enum ibmvfc_async_crq_type {
IBMVFC_ASYNC_CRQ_MAIN = 0,
IBMVFC_ASYNC_CRQ_SUB,
diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
index 7aaed4f64f26..80e7f8e0fd70 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
@@ -3,6 +3,7 @@
#include <kunit/visibility.h>
#include <scsi/scsi_device.h>
#include <scsi/scsi_transport_fc.h>
+#include <scsi/fc/fc_els.h>
#include <linux/list.h>
#include <linux/delay.h>
#include "ibmvfc.h"
@@ -256,9 +257,143 @@ static void ibmvfc_full_fpin_test(struct kunit *test)
scsi_host_put(vhost->host);
}
+#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); \
+}
+
+/**
+ * ibmvfc_extended_fpin_test - unit test for extended FPIN events
+ * @test: pointer to kunit structure
+ *
+ * Note: This test exercises extended FPIN code paths but does not check
+ * that statistics are correctly updated.
+ *
+ * Return: void
+ */
+static void ibmvfc_extended_fpin_test(struct kunit *test)
+{
+ enum ibmvfc_ae_fpin_status fs;
+ struct ibmvfc_async_subq_fpin crq[IBMVFC_AE_FPIN_CONGESTION_CLEARED+1] = {};
+ struct ibmvfc_async_subq_fpin
+ crqcn[IBMVFC_AE_FPIN_PORT_CONGESTED][FPIN_CONGN_DEVICE_SPEC+1] = {};
+ struct ibmvfc_async_subq_fpin crqportdg[FPIN_LI_DEVICE_SPEC+1] = {};
+ struct ibmvfc_target *tgt;
+ struct ibmvfc_host *vhost;
+ struct fc_rport *rport;
+ LIST_HEAD(evt_doneq);
+ unsigned long flags;
+ u64 pre, post;
+
+ vhost = ibmvfc_get_first_vhost();
+ if (!vhost)
+ kunit_skip(test, "No ibmvfc devices available");
+
+ spin_lock_irqsave(vhost->host->host_lock, flags);
+ if (vhost->scsi_scrqs.num_targets < 1) {
+ spin_unlock_irqrestore(vhost->host->host_lock, flags);
+ scsi_host_put(vhost->host);
+ kunit_skip(test, "No targets");
+ }
+ tgt = list_first_entry(&vhost->scsi_scrqs.targets, struct ibmvfc_target, queue);
+ if (!tgt->rport) {
+ spin_unlock_irqrestore(vhost->host->host_lock, flags);
+ scsi_host_put(vhost->host);
+ kunit_skip(test, "No rport");
+ }
+ rport = tgt->rport;
+ get_device(&rport->dev);
+ kref_get(&tgt->kref);
+ spin_unlock_irqrestore(vhost->host->host_lock, flags);
+
+ for (fs = IBMVFC_AE_FPIN_LINK_CONGESTED; fs <= IBMVFC_AE_FPIN_CONGESTION_CLEARED; fs++) {
+ switch (fs) {
+ case IBMVFC_AE_FPIN_PORT_CLEARED:
+ case IBMVFC_AE_FPIN_CONGESTION_CLEARED: {
+ struct ibmvfc_async_crq_event ae = { .type = IBMVFC_ASYNC_CRQ_SUB };
+
+ crq[fs].valid = 0x80;
+ crq[fs].flags = IBMVFC_ASYNC_IS_FPIN_EXT;
+ crq[fs].link_state = IBMVFC_AE_LS_LINK_UP;
+ crq[fs].fpin_status = fs;
+ crq[fs].event = cpu_to_be16(IBMVFC_AE_FPIN);
+ crq[fs].wwpn = cpu_to_be64(tgt->wwpn);
+ crq[fs].fpin_data.flags = IBMVFC_FPIN_EVENT_TYPE_VALID;
+ crq[fs].fpin_data.event_type = cpu_to_be16(FPIN_CONGN_CLEAR);
+ ae.subq = *(struct ibmvfc_async_sub_crq *)&crq[fs];
+ pre = READ_ONCE(rport->fpin_stats.cn_clear);
+ ibmvfc_handle_async(&ae, vhost);
+ flush_workqueue(vhost->fpin_workq);
+ post = READ_ONCE(rport->fpin_stats.cn_clear);
+ break;
+ }
+ case IBMVFC_AE_FPIN_LINK_CONGESTED:
+ case IBMVFC_AE_FPIN_PORT_CONGESTED:
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_CONGN_CLEAR, cn_clear,
+ crqcn[fs-1][FPIN_CONGN_CLEAR]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_CONGN_LOST_CREDIT,
+ cn_lost_credit,
+ crqcn[fs-1][FPIN_CONGN_LOST_CREDIT]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_CONGN_CREDIT_STALL,
+ cn_credit_stall,
+ crqcn[fs-1][FPIN_CONGN_CREDIT_STALL]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_CONGN_OVERSUBSCRIPTION,
+ cn_oversubscription,
+ crqcn[fs-1][FPIN_CONGN_OVERSUBSCRIPTION]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_CONGN_DEVICE_SPEC,
+ cn_device_specific,
+ crqcn[fs-1][FPIN_CONGN_DEVICE_SPEC]);
+ break;
+ case IBMVFC_AE_FPIN_PORT_DEGRADED:
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_UNKNOWN,
+ li_failure_unknown,
+ crqportdg[FPIN_LI_UNKNOWN]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_LINK_FAILURE,
+ li_link_failure_count,
+ crqportdg[FPIN_LI_LINK_FAILURE]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_LOSS_OF_SYNC,
+ li_loss_of_sync_count,
+ crqportdg[FPIN_LI_LOSS_OF_SYNC]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_LOSS_OF_SIG,
+ li_loss_of_signals_count,
+ crqportdg[FPIN_LI_LOSS_OF_SIG]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_PRIM_SEQ_ERR,
+ li_prim_seq_err_count,
+ crqportdg[FPIN_LI_PRIM_SEQ_ERR]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_INVALID_TX_WD,
+ li_invalid_tx_word_count,
+ crqportdg[FPIN_LI_INVALID_TX_WD]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_INVALID_CRC,
+ li_invalid_crc_count,
+ crqportdg[FPIN_LI_INVALID_CRC]);
+ IBMVFC_TEST_FPIN_EXT(fs, FPIN_LI_DEVICE_SPEC,
+ li_device_specific,
+ crqportdg[FPIN_LI_DEVICE_SPEC]);
+ break;
+ }
+ }
+
+ put_device(&rport->dev);
+ kref_put(&tgt->kref, ibmvfc_release_tgt);
+ scsi_host_put(vhost->host);
+}
+
static struct kunit_case ibmvfc_fpin_test_cases[] = {
KUNIT_CASE(ibmvfc_async_fpin_test),
KUNIT_CASE(ibmvfc_full_fpin_test),
+ KUNIT_CASE(ibmvfc_extended_fpin_test),
{},
};
--
2.55.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v9 9/9] scsi: ibmvfc: handle extended FPIN events
2026-09-11 2:19 ` [PATCH v9 9/9] scsi: ibmvfc: handle extended FPIN events Tyrel Datwyler
@ 2026-09-11 2:40 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-09-11 2:40 UTC (permalink / raw)
To: Tyrel Datwyler; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Unused list head and assigned but unused local variables in kunit test function.
--
commit e69daead7ba4a1176ce3e9bde4970bed18450907
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, completing full FPIN functionality.
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> index 7aaed4f64f26b..80e7f8e0fd700 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
[ ... ]
> @@ -256,9 +257,143 @@ static void ibmvfc_full_fpin_test(struct kunit *test)
> scsi_host_put(vhost->host);
> }
>
> +#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); \
> +}
> +
> +/**
> + * ibmvfc_extended_fpin_test - unit test for extended FPIN events
> + * @test: pointer to kunit structure
> + *
> + * Note: This test exercises extended FPIN code paths but does not check
> + * that statistics are correctly updated.
> + *
> + * Return: void
> + */
> +static void ibmvfc_extended_fpin_test(struct kunit *test)
> +{
> + enum ibmvfc_ae_fpin_status fs;
> + struct ibmvfc_async_subq_fpin crq[IBMVFC_AE_FPIN_CONGESTION_CLEARED+1] = {};
> + struct ibmvfc_async_subq_fpin
> + crqcn[IBMVFC_AE_FPIN_PORT_CONGESTED][FPIN_CONGN_DEVICE_SPEC+1] = {};
> + struct ibmvfc_async_subq_fpin crqportdg[FPIN_LI_DEVICE_SPEC+1] = {};
> + struct ibmvfc_target *tgt;
> + struct ibmvfc_host *vhost;
> + struct fc_rport *rport;
> + LIST_HEAD(evt_doneq);
> + unsigned long flags;
> + u64 pre, post;
[Severity: Low]
Are evt_doneq, pre, and post unused in ibmvfc_extended_fpin_test()?
The function declares LIST_HEAD(evt_doneq) which is never used. It also
declares and assigns pre and post variables (via the IBMVFC_TEST_FPIN_EXT
macro and manually later in the function) but never evaluates them. Could this
leave dead code and potentially trigger compiler warnings for unused variables?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260911021910.1038128-1-tyreld@linux.ibm.com?part=9
^ permalink raw reply [flat|nested] 23+ messages in thread