Re: [PATCH 4/5] ibmvfc: use async sub-queue for FPIN messages

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

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

> On 4/8/26 10:07 AM, Dave Marquardt via B4 Relay wrote:
>> From: Dave Marquardt <davemarq@linux.ibm.com>
>> 
>> - 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