Tyrel Datwyler <[email protected]> writes:
> On 6/8/26 11:30 AM, Dave Marquardt via B4 Relay wrote:
>> From: Dave Marquardt <[email protected]>
>>
>> Add support for basic FPIN messages to the ibmvfc driver. This includes
>>
>> - adding FPIN handling support to the async event handler
>> - offloading processing of FPIN messages to a work queue
>> - converting the VIOS FPIN message to a struct fc_els_fpin as used by
>> the Linux kernel
>> - passing the converted struct fc_els_fpin to fc_host_fpin_rcv for
>> processing
>>
>> The FPIN message conversion routines include a common routine that
>> will also be used in patches 6 and 7, which add full and extended FPIN
>> support.
>
> You are missing a Signed-off-by tag here.
>
>> ---
>> drivers/scsi/Kconfig | 10 ++
>> drivers/scsi/ibmvscsi/Makefile | 1 +
>> drivers/scsi/ibmvscsi/ibmvfc.c | 226
>> ++++++++++++++++++++++++++++++++++-
>> drivers/scsi/ibmvscsi/ibmvfc.h | 15 +++
>> drivers/scsi/ibmvscsi/ibmvfc_kunit.c | 131 ++++++++++++++++++++
>> 5 files changed, 379 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
>> index c3042393af23..d5fc7eb2ebb1 100644
>> --- a/drivers/scsi/Kconfig
>> +++ b/drivers/scsi/Kconfig
>> @@ -758,6 +758,16 @@ config SCSI_IBMVFC
>> To compile this driver as a module, choose M here: the
>> module will be called ibmvfc.
>>
>> +config SCSI_IBMVFC_KUNIT_TEST
>> + tristate "KUnit tests for the IBM POWER Virtual FC Client" if
>> !KUNIT_ALL_TESTS
>> + depends on SCSI_IBMVFC && KUNIT
>> + default KUNIT_ALL_TESTS
>> + help
>> + Compile IBM POWER Virtual FC client KUnit tests. These tests
>> + specifically test FPIN functionality. To compile this driver
>> + as a module, choose M here: the module will be called
>> + ibmvfc_kunit.
>> +
>> config SCSI_IBMVFC_TRACE
>> bool "enable driver internal trace"
>> depends on SCSI_IBMVFC
>> diff --git a/drivers/scsi/ibmvscsi/Makefile b/drivers/scsi/ibmvscsi/Makefile
>> index 5eb1cb1a0028..75dc7aee15a0 100644
>> --- a/drivers/scsi/ibmvscsi/Makefile
>> +++ b/drivers/scsi/ibmvscsi/Makefile
>> @@ -1,3 +1,4 @@
>> # SPDX-License-Identifier: GPL-2.0-only
>> obj-$(CONFIG_SCSI_IBMVSCSI) += ibmvscsi.o
>> obj-$(CONFIG_SCSI_IBMVFC) += ibmvfc.o
>> +obj-$(CONFIG_SCSI_IBMVFC_KUNIT_TEST) += ibmvfc_kunit.o
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
>> index 3dd2adda195e..9e5f0c0f0369 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
>> @@ -30,6 +30,9 @@
>> #include <scsi/scsi_tcq.h>
>> #include <scsi/scsi_transport_fc.h>
>> #include <scsi/scsi_bsg_fc.h>
>> +#include <kunit/visibility.h>
>> +#include <scsi/fc/fc_els.h>
>> +#include <linux/overflow.h>
>> #include "ibmvfc.h"
>>
>> static unsigned int init_timeout = IBMVFC_INIT_TIMEOUT;
>> @@ -3137,6 +3140,7 @@ static const struct ibmvfc_async_desc ae_desc [] = {
>> { "Halt", IBMVFC_AE_HALT, IBMVFC_DEFAULT_LOG_LEVEL },
>> { "Resume", IBMVFC_AE_RESUME, IBMVFC_DEFAULT_LOG_LEVEL },
>> { "Adapter Failed", IBMVFC_AE_ADAPTER_FAILED, IBMVFC_DEFAULT_LOG_LEVEL
>> },
>> + { "FPIN", IBMVFC_AE_FPIN, IBMVFC_DEFAULT_LOG_LEVEL },
>> };
>>
>> static const struct ibmvfc_async_desc unknown_ae = {
>> @@ -3185,17 +3189,211 @@ static const char *ibmvfc_get_link_state(enum
>> ibmvfc_ae_link_state state)
>> return "";
>> }
>>
>> +#define IBMVFC_FPIN_CONGN_DESC_SZ (sizeof(struct fc_els_fpin) +
>> sizeof(struct fc_fn_congn_desc))
>> +#define IBMVFC_FPIN_LI_DESC_SZ (sizeof(struct fc_els_fpin) + \
>> + struct_size_t(struct fc_fn_li_desc, pname_list,
>> 1))
>> +#define IBMVFC_FPIN_PEER_CONGN_DESC_SZ (sizeof(struct fc_els_fpin) + \
>> + struct_size_t(struct
>> fc_fn_peer_congn_desc, pname_list, 1))
>> +
>> +/**
>> + * ibmvfc_fpin_size_helper(): compute fpin structure size based on fpin
>> status
>> + * @fpin_status: status value
>> + *
>> + * Return:
>> + * 0: invalid fpin_status
>> + * other: valid size
>> + */
>> +static size_t ibmvfc_fpin_size_helper(u8 fpin_status)
>> +{
>> + size_t size = 0;
>> +
>> + switch (fpin_status) {
>> + case IBMVFC_AE_FPIN_LINK_CONGESTED:
>> + case IBMVFC_AE_FPIN_CONGESTION_CLEARED:
>> + size = IBMVFC_FPIN_CONGN_DESC_SZ;
>> + break;
>> + case IBMVFC_AE_FPIN_PORT_CONGESTED:
>> + case IBMVFC_AE_FPIN_PORT_CLEARED:
>> + size = IBMVFC_FPIN_PEER_CONGN_DESC_SZ;
>> + break;
>> + case IBMVFC_AE_FPIN_PORT_DEGRADED:
>> + size = IBMVFC_FPIN_LI_DESC_SZ;
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + return size;
>> +}
>> +
>> +/**
>> + * ibmvfc_common_fpin_to_desc(): allocate and populate a struct fc_els_fpin
>> struct
>> + * containing a descriptor.
>> + *
>> + * Allocate a struct fc_els_fpin containing a descriptor and populate
>> + * based on data from *ibmvfc_fpin.
>> + *
>> + * Return:
>> + * NULL - unable to allocate structure
>> + * non-NULL - pointer to populated struct fc_els_fpin
>> + */
>> +static struct fc_els_fpin *
>> +ibmvfc_common_fpin_to_desc(u8 fpin_status, __be64 wwpn, __be16 modifier,
>> + __be32 period, __be32 threshold, __be32 event_count)
>> +{
>> + struct fc_fn_peer_congn_desc *pdesc;
>> + struct fc_fn_congn_desc *cdesc;
>> + struct fc_fn_li_desc *ldesc;
>> + struct fc_els_fpin *fpin;
>> + size_t size;
>> +
>> + size = ibmvfc_fpin_size_helper(fpin_status);
>> + if (size == 0)
>
> To be consistent with other places in the driver !size instead of size == 0
> check.
Okay.
>> + return NULL;
>> +
>> + fpin = kzalloc(size, GFP_KERNEL);
>> + if (fpin == NULL)
>
> Same nit here !fpin instead of fpin == NULL.
Okay.
>> + return NULL;
>> +
>> + fpin->fpin_cmd = ELS_FPIN;
>> +
>> + switch (fpin_status) {
>> + case IBMVFC_AE_FPIN_CONGESTION_CLEARED:
>> + case IBMVFC_AE_FPIN_LINK_CONGESTED:
>> + fpin->desc_len = cpu_to_be32(sizeof(struct fc_fn_congn_desc));
>> + cdesc = (struct fc_fn_congn_desc *)fpin->fpin_desc;
>> + cdesc->desc_tag = cpu_to_be32(ELS_DTAG_CONGESTION);
>> + cdesc->desc_len =
>> cpu_to_be32(FC_TLV_DESC_LENGTH_FROM_SZ(*cdesc));
>> + if (fpin_status == IBMVFC_AE_FPIN_CONGESTION_CLEARED)
>> + cdesc->event_type = cpu_to_be16(FPIN_CONGN_CLEAR);
>> + else
>> + cdesc->event_type = cpu_to_be16(FPIN_CONGN_DEVICE_SPEC);
>> + cdesc->event_modifier = modifier;
>> + cdesc->event_period = period;
>> + cdesc->severity = FPIN_CONGN_SEVERITY_WARNING;
>> + break;
>> + case IBMVFC_AE_FPIN_PORT_CONGESTED:
>> + case IBMVFC_AE_FPIN_PORT_CLEARED:
>> + fpin->desc_len =
>> + cpu_to_be32(struct_size_t(struct fc_fn_peer_congn_desc,
>> pname_list, 1));
>> + pdesc = (struct fc_fn_peer_congn_desc *)fpin->fpin_desc;
>> + pdesc->desc_tag = cpu_to_be32(ELS_DTAG_PEER_CONGEST);
>> + pdesc->desc_len =
>> cpu_to_be32(FC_TLV_DESC_LENGTH_FROM_SZ(*pdesc));
>> + if (fpin_status == IBMVFC_AE_FPIN_PORT_CLEARED)
>> + pdesc->event_type = cpu_to_be16(FPIN_CONGN_CLEAR);
>> + else
>> + pdesc->event_type = cpu_to_be16(FPIN_CONGN_DEVICE_SPEC);
>> + pdesc->event_modifier = modifier;
>> + pdesc->event_period = period;
>> + pdesc->detecting_wwpn = cpu_to_be64(0);
>
> While not wrong 0 is 0 regardless of endianiess. Is this value always going to
> be zero here or is this place holder code that changes later in this patchset?
> If its the case this isn't a place holder the original fpin struct was
> allocated
> with kzalloc so this value should be zero already.
I'll drop this.
>> + pdesc->attached_wwpn = wwpn;
>> + pdesc->pname_count = cpu_to_be32(1);
>> + pdesc->pname_list[0] = wwpn;
>> + break;
>> + case IBMVFC_AE_FPIN_PORT_DEGRADED:
>> + fpin->desc_len = cpu_to_be32(struct_size_t(struct
>> fc_fn_li_desc, pname_list, 1));
>> + ldesc = (struct fc_fn_li_desc *)fpin->fpin_desc;
>> + ldesc->desc_tag = cpu_to_be32(ELS_DTAG_LNK_INTEGRITY);
>> + ldesc->desc_len =
>> cpu_to_be32(FC_TLV_DESC_LENGTH_FROM_SZ(*ldesc));
>> + ldesc->event_type = cpu_to_be16(FPIN_LI_UNKNOWN);
>> + ldesc->event_modifier = modifier;
>> + ldesc->event_threshold = threshold;
>> + ldesc->event_count = event_count;
>> + ldesc->detecting_wwpn = cpu_to_be64(0);
>
> Same comment as above.
>
>> + ldesc->attached_wwpn = wwpn;
>> + ldesc->pname_count = cpu_to_be32(1);
>> + ldesc->pname_list[0] = wwpn;
>> + break;
>> + default:
>> + /* This should be caught above. */
>> + kfree(fpin);
>> + fpin = NULL;
>> + break;
>> + }
>> +
>> + return fpin;
>> +}
>> +
>> +/**
>> + * ibmvfc_basic_fpin_to_desc(): allocate and populate a struct fc_els_fpin
>> struct
>> + * containing a descriptor.
>> + * @ibmvfc_fpin: Pointer to async crq
>> + *
>> + * Allocate a struct fc_els_fpin containing a descriptor and populate
>> + * based on data from *ibmvfc_fpin.
>> + *
>> + * Return:
>> + * NULL - unable to allocate structure
>> + * non-NULL - pointer to populated struct fc_els_fpin
>> + */
>> +static struct fc_els_fpin *
>> +ibmvfc_basic_fpin_to_desc(struct ibmvfc_async_crq *crq, u64 wwpn)
>> +{
>> + return ibmvfc_common_fpin_to_desc(crq->fpin_status, cpu_to_be64(wwpn),
>> + cpu_to_be16(0),
>> +
>> cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_PERIOD),
>> +
>> cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_THRESHOLD),
>> + cpu_to_be32(1));
>> +}
>> +
>> +/**
>> + * ibmvfc_process_async_work - Process IBMVFC_AE_FPIN async CRQ from work
>> queue
>> + * @work: pointer to work_struct
>> + */
>> +static void ibmvfc_process_async_work(struct work_struct *work)
>> +{
>> + struct ibmvfc_async_work *aw;
>> + struct ibmvfc_async_crq *crq;
>> + struct ibmvfc_target *tgt;
>> + struct ibmvfc_host *vhost;
>> + struct fc_els_fpin *fpin;
>> +
>> + aw = container_of(work, struct ibmvfc_async_work, async_work_s);
>> + crq = aw->crq;
>> + vhost = aw->vhost;
>> +
>> + if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
>> + goto end;
>> + list_for_each_entry(tgt, &vhost->targets, queue) {
>
> Should we be holding the host lock when iterateing targets?
fc_host_fpin_rcv assumes no locks are held. I built a kernel with
LOCKDEP, PROVE_LOCKING, DEBUG_SPINLOCK et al defined, and got some
deadlocks. Some of the routines fc_host_fpin_rcv call acquire the host
lock also.
I'l change this code to use list_for_each_entry_safe. Is that enough for
safety in this case?
-Dave
