Re: [PATCH v6 4/8] scsi: ibmvfc: extend async event handlers to handle async sub queue events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Double clear of `valid` in the CRQ ring buffer introduces a race condition that can clobber subsequent hardware events.
- [High] Missing check for `IBMVFC_ASYNC_ID_IS_ASSOC_ID` causes `assoc_id` to be incorrectly treated as a WWNN, preventing target lookup and dropping events.
- [Medium] KUnit tests use `msleep()` to wait for asynchronous workqueue execution, leading to flaky test failures.
- [Low] Dereferencing a `void *` pointer in the unmodified `nvme_scrqs` loop inside `ibmvfc_handle_async` causes a compilation error.
--

--- Patch [4]: [PATCH v6 4/8] scsi: ibmvfc: extend async event handlers to handle async sub queue events ---
commit d29a42191b6208bc62350b9d0fda8394443adcd4
Author: Dave Marquardt <davemarq@linux.ibm.com>

scsi: ibmvfc: extend async event handlers to handle async sub queue events

Refactor async event handling to support both traditional async CRQs and
new asynchronous sub-queue CRQs.

[ ... ]

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 19a3f12615648..4d1630d2ab1b5 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -3462,28 +3501,41 @@ static struct ibmvfc_target *ibmvfc_find_target(struct list_head *target_list,
>   */
>  static void ibmvfc_process_async_work(struct work_struct *work)
>  {
> +	struct ibmvfc_async_sub_crq *subq = NULL;
>  	struct ibmvfc_async_work *aw;
> -	struct ibmvfc_async_crq *crq;
> +	struct ibmvfc_async_crq *crq = NULL;
>  	struct ibmvfc_target *tgt;
>  	struct ibmvfc_host *vhost;
> -	struct fc_els_fpin *fpin;
> +	struct fc_els_fpin *fpin = NULL;
>  	unsigned long flags;
> +	__be64 node_name;
> +	__be64 scsi_id;
> +	bool is_subq;
> +	__be64 wwpn;
>  
>  	aw = container_of_const(work, struct ibmvfc_async_work, async_work_s);
>  	vhost = aw->vhost;
> -	crq = &aw->crq;
> +	is_subq = aw->is_subq;
> +	if (is_subq) {
> +		subq = &aw->crq.subq;
> +		scsi_id = 0;
> +		wwpn = subq->wwpn;
> +		node_name = subq->id.node_name;

[Severity: High]
Does this incorrectly treat assoc_id as a WWNN? It looks like we should
check the IBMVFC_ASYNC_ID_IS_ASSOC_ID flag before unconditionally reading
node_name, as this could prevent target lookups and cause FPIN events
to be dropped.

[ ... ]

> @@ -3511,25 +3567,52 @@ static void ibmvfc_process_async_work(struct work_struct *work)
>  
>  /**
>   * ibmvfc_handle_async - Handle an async event from the adapter
> - * @crq:	crq to process
> + * @crq:	ibmvfc_async_crq or ibmvfc_async_sub_crq
>   * @vhost:	ibmvfc host struct
> + * @is_sub_crq:	indicates whether the crq points to a struct ibmvfc_async_sub_crq
>   *
>   **/
> -VISIBLE_IF_KUNIT void ibmvfc_handle_async(struct ibmvfc_async_crq *crq,
> -					  struct ibmvfc_host *vhost)
> +VISIBLE_IF_KUNIT void ibmvfc_handle_async(void *crq,
> +					  struct ibmvfc_host *vhost,
> +					  bool is_sub_crq)
>  {

[Severity: Low]
Will this cause a compilation error? The crq parameter type is changed to a
void pointer here, but there is still an unmodified nvme_scrqs loop later in
ibmvfc_handle_async() that directly dereferences it like crq->scsi_id.

> -	const struct ibmvfc_async_desc *desc = ibmvfc_get_ae_desc(be64_to_cpu(crq->event));
> +	struct ibmvfc_async_crq *async_crq = NULL;
> +	struct ibmvfc_async_sub_crq *subq = NULL;
>  	const struct ibmvfc_async_desc *desc;
>  	struct ibmvfc_async_work *aw;
>  	struct ibmvfc_target *tgt;
> -
> -	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),
> -		   be64_to_cpu(crq->wwpn), be64_to_cpu(crq->node_name),
> -		   ibmvfc_get_link_state(crq->link_state));
> -
> -	switch (be64_to_cpu(crq->event)) {
> +	__be64 node_name;
> +	__be64 scsi_id;
> +	u8 link_state;
> +	__be64 wwpn;
> +	u64 event;
> +
> +	if (is_sub_crq) {
> +		subq = crq;
> +		event = be16_to_cpu(subq->event);
> +		link_state = subq->link_state;
> +		scsi_id = 0;
> +		wwpn = subq->wwpn;
> +		node_name = subq->id.node_name;

[Severity: High]
Is it possible to receive an assoc_id here instead of a node_name?
Missing the check for IBMVFC_ASYNC_ID_IS_ASSOC_ID could result in mismatches
during target lookup.

[ ... ]

> @@ -3615,16 +3698,27 @@ VISIBLE_IF_KUNIT void ibmvfc_handle_async(struct ibmvfc_async_crq *crq,
>  		if (aw) {
>  			INIT_WORK(&aw->async_work_s, ibmvfc_process_async_work);
>  			aw->vhost = vhost;
> -			aw->crq = *crq;
> +			aw->is_subq = is_sub_crq;
> +			if (is_sub_crq)
> +				aw->crq.subq = *subq;
> +			else
> +				aw->crq.async_crq = *async_crq;
>  			queue_work(vhost->fpin_workq, &aw->async_work_s);
>  		} else
>  			dev_err_ratelimited(vhost->dev,
>  					    "can't offload async CRQ to work queue\n");
>  		break;
>  	default:
> -		dev_err(vhost->dev, "Unknown async event received: %lld\n", crq->event);
> +		dev_err(vhost->dev, "Unknown async event received: %llu\n", event);
>  		break;
>  	}
> +
> +	rmb();
> +	if (is_sub_crq)
> +		subq->valid = 0;
> +	else
> +		async_crq->valid = 0;
> +	wmb();
>  }

[Severity: High]
Can clearing the valid byte here lead to silently dropped events?

Since ibmvfc_handle_async() clears the valid byte to 0, it tells the hardware
the slot can be reused.

In the caller ibmvfc_tasklet():

	while ((async = ibmvfc_next_async_crq(vhost)) != NULL) {
		ibmvfc_handle_async(async, vhost, false);
		async->valid = 0;
		wmb();
	}

If the hardware writes a new event into this slot between the first clear in
ibmvfc_handle_async() and the redundant clear in ibmvfc_tasklet(), wouldn't
the second clear overwrite the new event's valid byte and lose it?

[ ... ]

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> index 64a517a74af8e..0b29c1e6478b5 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> @@ -58,7 +58,7 @@ static void ibmvfc_async_fpin_test(struct kunit *test)
>  		crq[fs].scsi_id = cpu_to_be64(tgt->scsi_id);
>  		crq[fs].wwpn = cpu_to_be64(tgt->wwpn);
>  		crq[fs].node_name = cpu_to_be64(tgt->ids.node_name);
> -		ibmvfc_handle_async(&crq[fs], vhost);
> +		ibmvfc_handle_async(&crq[fs], vhost, false);
>  		msleep(1U);
>  	}

[Severity: Medium]
Will this lead to flaky tests on heavily loaded systems?

Using msleep(1U) to wait for asynchronous workqueue execution might not give
the workqueue enough time to run. Could we use flush_workqueue() instead?

[ ... ]

> @@ -93,7 +93,110 @@ static void ibmvfc_async_fpin_test(struct kunit *test)
>  	crq[0].scsi_id = cpu_to_be64(tgt->scsi_id);
>  	crq[0].wwpn = cpu_to_be64(tgt->wwpn);
>  	crq[0].node_name = cpu_to_be64(tgt->ids.node_name);
> -	ibmvfc_handle_async(&crq[0], vhost);
> +	ibmvfc_handle_async(&crq[0], vhost, false);
> +	msleep(1U);

[Severity: Medium]
Should we avoid using msleep(1U) here for synchronizing with the workqueue?

If the workqueue doesn't run within 1 millisecond, the assertions that follow
could fail.

[ ... ]

> +	/* bad path */
> +	crq[0].valid = 0x80;
> +	crq[0].link_state = IBMVFC_AE_LS_LINK_UP;
> +	crq[0].fpin_status = 0; /* bad value */
> +	crq[0].event = cpu_to_be16(IBMVFC_AE_FPIN);
> +	crq[0].wwpn = cpu_to_be64(tgt->wwpn);
> +	crq[0].id.node_name = cpu_to_be64(tgt->ids.node_name);
> +	ibmvfc_handle_async(&crq[0], vhost, true);
>  	msleep(1U);

[Severity: Medium]
Same concern here about using msleep(1U) to wait for the workqueue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-ibmvfc-fpin-support-v6-0-62a2d525e70d@linux.ibm.com?part=4