Re: [PATCH 1/5] ibmvfc: add basic FPIN support

[Dave Marquardt]

Tyrel Datwyler <tyreld@xxxxxxxxxxxxx> writes:

> On 4/8/26 10:07 AM, Dave Marquardt via B4 Relay wrote:
>> From: Dave Marquardt <davemarq@xxxxxxxxxxxxx>
>> 
>> - Add FPIN event descriptor
>> - Add congestion cleared status
>> - Add code to handle basic FPIN async event
>> - Add KUnit tests
>
> You need a more detailed description of your changes here for the commit log body.
>
> You will also need a signed off tag from yourself for this to even be merged.

Odd. I used b4 to prepare the patch set, and it should have added a
Signed-off-by: tag. I'll double check it next round.

> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>
>> ---
>>  drivers/scsi/Kconfig                 |  10 ++
>>  drivers/scsi/ibmvscsi/Makefile       |   1 +
>>  drivers/scsi/ibmvscsi/ibmvfc.c       | 189 ++++++++++++++++++++++++++++++++++-
>>  drivers/scsi/ibmvscsi/ibmvfc.h       |   9 ++
>>  drivers/scsi/ibmvscsi/ibmvfc_kunit.c |  95 ++++++++++++++++++
>>  5 files changed, 302 insertions(+), 2 deletions(-)
>
> <snip>
>
>> +static struct fc_els_fpin *
>> +ibmvfc_common_fpin_to_desc(u8 fpin_status, __be64 wwpn, __be16 modifier,
>> +			   __be32 period, __be32 threshold, __be32 event_count)
>> +{
>> +	struct fc_fn_peer_congn_desc *pdesc;
>> +	struct fc_fn_congn_desc *cdesc;
>> +	struct fc_fn_li_desc *ldesc;
>> +	struct fc_els_fpin *fpin;
>> +	size_t size;
>> +
>> +	size = ibmvfc_fpin_size_helper(fpin_status);
>> +	if (size == 0)
>> +		return NULL;
>> +
>> +	fpin = kzalloc(size, GFP_KERNEL);
>
> This appears to be called by ibmvfc_handle_async() with runs in atomic context
> and cannot therefore sleep. This allocation needs to be GFP_ATOMIC. Although
> there is another issue below that might make this moot.

Noted. As to the problem below, once this is moved to running in a work
queue worker thread, this can stay as is.

>> +	if (fpin == NULL)
>> +		return NULL;
>> +
>> +	fpin->fpin_cmd = ELS_FPIN;
>> +
>> +	switch (fpin_status) {
>> +	case IBMVFC_AE_FPIN_CONGESTION_CLEARED:
>> +	case IBMVFC_AE_FPIN_LINK_CONGESTED:
>> +		fpin->desc_len = cpu_to_be32(sizeof(struct fc_fn_congn_desc));
>> +		cdesc = (struct fc_fn_congn_desc *)fpin->fpin_desc;
>> +		cdesc->desc_tag = cpu_to_be32(ELS_DTAG_CONGESTION);
>> +		cdesc->desc_len = cpu_to_be32(FC_TLV_DESC_LENGTH_FROM_SZ(*cdesc));
>> +		if (fpin_status == IBMVFC_AE_FPIN_CONGESTION_CLEARED)
>> +			cdesc->event_type = cpu_to_be16(FPIN_CONGN_CLEAR);
>> +		else
>> +			cdesc->event_type = cpu_to_be16(FPIN_CONGN_DEVICE_SPEC);
>> +		cdesc->event_modifier = modifier;
>> +		cdesc->event_period = period;
>> +		cdesc->severity = FPIN_CONGN_SEVERITY_WARNING;
>> +		break;
>> +	case IBMVFC_AE_FPIN_PORT_CONGESTED:
>> +	case IBMVFC_AE_FPIN_PORT_CLEARED:
>> +		fpin->desc_len = cpu_to_be32(sizeof(struct fc_fn_peer_congn_desc));
>> +		pdesc = (struct fc_fn_peer_congn_desc *)fpin->fpin_desc;
>> +		pdesc->desc_tag = cpu_to_be32(ELS_DTAG_PEER_CONGEST);
>> +		pdesc->desc_len = cpu_to_be32(FC_TLV_DESC_LENGTH_FROM_SZ(*pdesc));
>> +		if (fpin_status == IBMVFC_AE_FPIN_PORT_CLEARED)
>> +			pdesc->event_type = cpu_to_be16(FPIN_CONGN_CLEAR);
>> +		else
>> +			pdesc->event_type = cpu_to_be16(FPIN_CONGN_DEVICE_SPEC);
>> +		pdesc->event_modifier = modifier;
>> +		pdesc->event_period = period;
>> +		pdesc->detecting_wwpn = cpu_to_be64(0);
>> +		pdesc->attached_wwpn = wwpn;
>> +		pdesc->pname_count = cpu_to_be32(1);
>> +		pdesc->pname_list[0] = wwpn;
>> +		break;
>> +	case IBMVFC_AE_FPIN_PORT_DEGRADED:
>> +		fpin->desc_len = cpu_to_be32(sizeof(struct fc_fn_li_desc));
>> +		ldesc = (struct fc_fn_li_desc *)fpin->fpin_desc;
>> +		ldesc->desc_tag = cpu_to_be32(ELS_DTAG_LNK_INTEGRITY);
>> +		ldesc->desc_len = cpu_to_be32(FC_TLV_DESC_LENGTH_FROM_SZ(*ldesc));
>> +		ldesc->event_type = cpu_to_be16(FPIN_LI_UNKNOWN);
>> +		ldesc->event_modifier = modifier;
>> +		ldesc->event_threshold = threshold;
>> +		ldesc->event_count = event_count;
>> +		ldesc->detecting_wwpn = cpu_to_be64(0);
>> +		ldesc->attached_wwpn = wwpn;
>> +		ldesc->pname_count = cpu_to_be32(1);
>> +		ldesc->pname_list[0] = wwpn;
>> +		break;
>> +	default:
>> +		/* This should be caught above. */
>> +		kfree(fpin);
>> +		fpin = NULL;
>> +		break;
>> +	}
>> +
>> +	return fpin;
>> +}
>> +
>> +/**
>> + * ibmvfc_basic_fpin_to_desc(): allocate and populate a struct fc_els_fpin struct
>> + * containing a descriptor.
>> + * @ibmvfc_fpin: Pointer to async crq
>> + *
>> + * Allocate a struct fc_els_fpin containing a descriptor and populate
>> + * based on data from *ibmvfc_fpin.
>> + *
>> + * Return:
>> + * NULL     - unable to allocate structure
>> + * non-NULL - pointer to populated struct fc_els_fpin
>> + */
>> +static struct fc_els_fpin *
>> +/*XXX*/ibmvfc_basic_fpin_to_desc(struct ibmvfc_async_crq *crq)
>
> What is with this /*XXX*/? I can't find it once I apply the patchset so I assume
> its removed in a later patch, but it should be removed here.

An artifact I missed in squashing commits. Thanks for pointing it out.

>> +{
>> +	return ibmvfc_common_fpin_to_desc(crq->fpin_status, crq->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
>>   * @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_target *tgt;
>> +	struct fc_els_fpin *fpin;
>>  
>>  	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),
>> @@ -3269,11 +3422,37 @@ 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 (!crq->scsi_id && !crq->wwpn && !crq->node_name)
>> +			break;
>> +		list_for_each_entry(tgt, &vhost->targets, queue) {
>> +			if (crq->scsi_id && cpu_to_be64(tgt->scsi_id) != crq->scsi_id)
>> +				continue;
>> +			if (crq->wwpn && cpu_to_be64(tgt->ids.port_name) != crq->wwpn)
>> +				continue;
>> +			if (crq->node_name && cpu_to_be64(tgt->ids.node_name) != crq->node_name)
>> +				continue;
>> +			if (!tgt->rport)
>> +				continue;
>> +			fpin = ibmvfc_basic_fpin_to_desc(crq);
>> +			if (fpin) {
>> +				fc_host_fpin_rcv(tgt->vhost->host,
>> +						 sizeof(*fpin) +
>> +						       be32_to_cpu(fpin->desc_len),
>> +						 (char *)fpin, 0);
>
> This call to fc_host_fpin_rcv() appears to be problematic as it assumes no locks
> are held, but ibmvfc_handle_async() is called with the scsi host lock held. We
> already do a lot more work than we probaly should in our interrupt handler. I
> think we maybe need to pass the FPIN work off to a workqueue instead to be
> handled in process context instead.

Agreed. I'm working on adding a work queue for offloading this FPIN
processing.

-Dave