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

[sashiko-bot@kernel.org]

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?

>  	if (fpin) {
>  		fc_host_fpin_rcv(tgt->vhost->host,
>  				 sizeof(*fpin) + be32_to_cpu(fpin->desc_len),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925044900.414340-1-tyreld@linux.ibm.com?part=7