Re: [PATCH v11 07/11] scsi: ibmvfc: extend async event handlers for async sub-CRQ events

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

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] scsi: ibmvfc: FPIN descriptor uses potentially unpopulated WWPN from sub-CRQ
> --
>
> commit 8a1ab5ee327462636518cdba29b91df0f867bb9f
> Author: Dave Marquardt <davemarq@linux.ibm.com>
>
> scsi: ibmvfc: extend async event handlers for async sub-CRQ events
>
> This commit refactors async event handling to support both traditional async
> CRQs and new asynchronous sub-queue CRQs. It introduces a tagged union that
> wraps the async event and dispatches processing based on event type.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index f465efadc02c8..321bb2f7219ed 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;
>
> [ ... ]
>
>> +	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));
>> +}
>
> [ ... ]
>
>> @@ -3463,28 +3502,40 @@ static struct ibmvfc_target *ibmvfc_find_target(struct list_head *target_list,
>>   */
>>  static void ibmvfc_process_async_work(struct work_struct *work)
>>  {
>
> [ ... ]
>
>> @@ -3496,7 +3547,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);
>> +
>
> [Severity: Medium]
> If the hardware sends an async sub-CRQ FPIN event lacking a WWPN but
> providing a valid node_name or scsi_id, the driver successfully resolves
> the target (tgt) earlier in this function. 
>
> However, ibmvfc_full_fpin_to_desc() uses subq->wwpn instead of the resolved
> tgt->wwpn. Could this generate an FPIN descriptor with a zero attached WWPN,
> causing fc_host_fpin_rcv() to fail to match the target's rport and drop the
> congestion statistics update?
>
> The legacy CRQ path safely passes tgt->wwpn via ibmvfc_basic_fpin_to_desc().
> Should ibmvfc_full_fpin_to_desc() be updated to also take tgt->wwpn?

I agree. I've extended ibmvfc_full_fpin_to_desc() with a new u64 wwpn
argument, similar to ibmvfc_basic_fpin_to_desc().

-Dave