Tyrel Datwyler <[email protected]> writes:
> On 4/8/26 10:07 AM, Dave Marquardt via B4 Relay wrote:
>> From: Dave Marquardt <[email protected]>
>>
>> - allocate async sub-queue
>> - allocate interrupt and set up handler
>> - negotiate use of async sub-queue with NPIV (VIOS)
>> - refactor ibmvfc_basic_fpin_to_desc() and ibmvfc_full_fpin_to_desc()
>> into common routine
>> - add KUnit test to verify async sub-queue is allocated
>
> Again more descriptive commit log message required here. Also, this looks
> like a
> lot of things being implmented. Can this be broken into multiple patches? It
> sure looks like a bunch of functional changes that build on each other.
Yes, I'll break this up into more patches.
>> ---
>> drivers/scsi/ibmvscsi/ibmvfc.c | 325
>> ++++++++++++++++++++++++++++++++---
>> drivers/scsi/ibmvscsi/ibmvfc.h | 29 +++-
>> drivers/scsi/ibmvscsi/ibmvfc_kunit.c | 52 +++---
>> 3 files changed, 363 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
>> index 803fc3caa14d..26e39b367022 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
>> @@ -1471,6 +1471,13 @@ static void ibmvfc_gather_partition_info(struct
>> ibmvfc_host *vhost)
>> of_node_put(rootdn);
>> }
>>
>> +static __be64 ibmvfc_npiv_chan_caps[] = {
>> + cpu_to_be64(IBMVFC_CAN_USE_CHANNELS | IBMVFC_USE_ASYNC_SUBQ |
>> + IBMVFC_YES_SCSI | IBMVFC_CAN_HANDLE_FPIN),
>> + cpu_to_be64(IBMVFC_CAN_USE_CHANNELS),
>> +};
>> +#define IBMVFC_NPIV_CHAN_CAPS_SIZE
>> (sizeof(ibmvfc_npiv_chan_caps)/sizeof(__be64))
>> +
>
> I really don't understand what you are doing here? You seem to be definig
> various sets of capabilities, but how does the driver decide which set to use?
> As far as I can tell the index is increased and the capabilities decrease each
> time a transport event is received. This looks like maybe its just a testing
> hack.
My thought was to deal with an older VIOS that doesn't support the async
sub-queue and full FPIN. But I suppose the response should just not set the
appropriate bits. I'll go re-read the NPIV spec and figure out if this
is actually needed.
>> /**
>> * ibmvfc_set_login_info - Setup info for NPIV login
>> * @vhost: ibmvfc host struct
>> @@ -1486,6 +1493,8 @@ static void ibmvfc_set_login_info(struct ibmvfc_host
>> *vhost)
>> const char *location;
>> u16 max_cmds;
>>
>> + ENTER;
>> +
>> max_cmds = scsi_qdepth + IBMVFC_NUM_INTERNAL_REQ;
>> if (mq_enabled)
>> max_cmds += (scsi_qdepth + IBMVFC_NUM_INTERNAL_SUBQ_REQ) *
>> @@ -1509,8 +1518,12 @@ static void ibmvfc_set_login_info(struct ibmvfc_host
>> *vhost)
>> 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);
>> + if (vhost->mq_enabled || vhost->using_channels) {
>> + if (vhost->login_cap_index >= IBMVFC_NPIV_CHAN_CAPS_SIZE)
>> + login_info->capabilities |=
>> cpu_to_be64(IBMVFC_CAN_USE_CHANNELS);
>> + else
>> + login_info->capabilities |=
>> ibmvfc_npiv_chan_caps[vhost->login_cap_index];
>> + }
>>
>> login_info->async.va = cpu_to_be64(vhost->async_crq.msg_token);
>> login_info->async.len = cpu_to_be32(async_crq->size *
>> @@ -1524,6 +1537,8 @@ static void ibmvfc_set_login_info(struct ibmvfc_host
>> *vhost)
>> location = of_get_property(of_node, "ibm,loc-code", NULL);
>> location = location ? location : dev_name(vhost->dev);
>> strscpy(login_info->drc_name, location, sizeof(login_info->drc_name));
>> +
>> + LEAVE;
>> }
>>
>> /**
>> @@ -3323,7 +3338,7 @@ ibmvfc_common_fpin_to_desc(u8 fpin_status, __be64
>> wwpn, __be16 modifier,
>> * non-NULL - pointer to populated struct fc_els_fpin
>> */
>> static struct fc_els_fpin *
>> -/*XXX*/ibmvfc_basic_fpin_to_desc(struct ibmvfc_async_crq *crq)
>
> I mentioned this /*XXX*/ in an earlier patch. This needs to be fixed in that
> patch.
Yes.
>> +ibmvfc_basic_fpin_to_desc(struct ibmvfc_async_crq *crq)
>> {
>> return ibmvfc_common_fpin_to_desc(crq->fpin_status, crq->wwpn,
>> cpu_to_be16(0),
>> @@ -3332,6 +3347,29 @@ static struct fc_els_fpin *
>> 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_subq *ibmvfc_fpin)
>> +{
>> + return ibmvfc_common_fpin_to_desc(ibmvfc_fpin->fpin_status,
>> + ibmvfc_fpin->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_handle_async - Handle an async event from the adapter
>> * @crq: crq to process
>> @@ -3449,6 +3487,120 @@ VISIBLE_IF_KUNIT void ibmvfc_handle_async(struct
>> ibmvfc_async_crq *crq,
>> }
>> EXPORT_SYMBOL_IF_KUNIT(ibmvfc_handle_async);
>>
>> +VISIBLE_IF_KUNIT void ibmvfc_handle_asyncq(struct ibmvfc_crq *crq_instance,
>> + struct ibmvfc_host *vhost,
>> + struct list_head *evt_doneq)
>> +{
>> + struct ibmvfc_async_subq *crq = (struct ibmvfc_async_subq
>> *)crq_instance;
>> + const struct ibmvfc_async_desc *desc =
>> ibmvfc_get_ae_desc(be16_to_cpu(crq->event));
>> + struct ibmvfc_target *tgt;
>> + struct fc_els_fpin *fpin;
>> +
>> + ibmvfc_log(vhost, desc->log_level,
>> + "%s event received. wwpn: %llx, node_name: %llx%s event
>> 0x%x\n",
>> + desc->desc, be64_to_cpu(crq->wwpn),
>> be64_to_cpu(crq->id.node_name),
>> + ibmvfc_get_link_state(crq->link_state),
>> be16_to_cpu(crq->event));
>
> Was there no way to not copy/paste what looks like basically
> ibmvfc_handle_async
> into ibmvfc_handle_asyncq? This is a bunch of unnecessary code bloat. The
> major
> difference seems that crq->event is be64 on the standard CRQ and be16 on a
> sub-crq and accessing certain fields differently.
That's a good idea. I'll see what I can do. Seems like a little
refactoring should make it work.
> Again I think maybe we need to consider moving all the async work into a
> workqueue.
My initial thought was to just queue the FPIN processing of
ibmvfc_handle_asyncq to a work queue to resolve the problem of calling
fc_host_fpin_rcv from interrupt context, but putting all of this
processing into a work queue would work too. I'll look into it.
-Dave
