Re: [PATCH v5 5/7] scsi: ibmvfc: extend async event handlers to handle async sub queue events

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Dereferencing a `void *` pointer causes a fatal compilation error.
- [High] Double-clearing of the hardware ring buffer's `valid` flag introduces a race condition that drops events.
- [High] Target matching for sub-queue events is broken due to unconditionally treating the identifier union as `node_name`.
- [Medium] Sub-queue FPIN descriptors are generated with an invalid event type (0), causing telemetry loss.
- [Medium] KUnit tests compare mismatched FPIN port congestion statistic fields.
- [Low] Use of `rmb()` to order memory reads before a subsequent memory write violates generic memory barrier semantics.
--

--- Patch [5]: [PATCH v5 5/7] scsi: ibmvfc: extend async event handlers to handle async sub queue events ---
commit 91821c3532cfc1d2567a96c877a77e93654a286a
Author: Dave Marquardt <[email protected]>

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

This commit refactors async event handling to support traditional async
CRQs and new asynchronous sub-queue CRQs. It introduces a new is_subq flag,
extracts target information according to the event source, and updates KUnit
tests for the new interface.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index c8efe17223c9e..97856a609bde4 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> +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_be16(0),

[Severity: Medium]
Is hardcoding the event type to 0 intended here?

Unlike ibmvfc_basic_fpin_to_desc() which maps the status to a valid Fibre
Channel event type (like FPIN_CONGN_DEVICE_SPEC), passing 0 results in an
invalid event type.

Could this cause downstream generic handlers to drop the telemetry metrics?

[ ... ]
>  static void ibmvfc_process_async_work(struct work_struct *work)
>  {
[ ... ]
> +	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 misinterpret the identifier for NVMe devices?

The ibmvfc_async_subq structure uses a union for the ID. If the
IBMVFC_ASYNC_ID_IS_ASSOC_ID flag is set, this unconditionally extracts it as
node_name.

This means an assoc_id would be incorrectly compared against
tgt->ids.node_name during the ibmvfc_find_target() lookup, causing valid
sub-queue events to be ignored.

[ ... ]
> -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_subq)
>  {

[Severity: Critical]
Does this cause a compilation error?

The signature of ibmvfc_handle_async() was changed so that crq is now a
void pointer. While the list_for_each_entry() loop for scsi_scrqs was
updated to use the new scsi_id, wwpn, and node_name local variables, the
identical loop for nvme_scrqs was not updated:

ibmvfc_handle_async() {
    ...
    list_for_each_entry(tgt, &vhost->nvme_scrqs.targets, queue) {
        if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
            break;
    ...
}

Because crq is now void *, direct member access is invalid in C and will
break the build.

[ ... ]
>  	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();

[Severity: Low]
Does this barrier provide the correct guarantees?

The rmb() only ensures prior loads complete before subsequent loads.
To ensure that prior loads complete before the subsequent valid = 0 store,
shouldn't this be a full mb()?

> +	if (is_subq)
> +		subq->valid = 0;
> +	else
> +		async_crq->valid = 0;
> +	wmb();
>  }

[Severity: High]
Will clearing the valid flag here and again in the caller permanently drop
new events?

Once the valid flag is cleared here, the hardware is free to reuse this slot
and write a new event (setting valid to 0x80). However, ibmvfc_tasklet()
(the caller) redundantly executes async->valid = 0 upon return from this
function:

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

If a new event was written by the hardware in this tiny race window, the
second clear in ibmvfc_tasklet() will overwrite the 0x80 with 0, permanently
dropping the new event.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> index 64a517a74af8e..a3e3e3471c5ef 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> @@ -45,7 +45,7 @@ static void ibmvfc_async_fpin_test(struct kunit *test)
>  	fc_host = shost_to_fc_host(vhost->host);
>  
>  	pre[IBMVFC_AE_FPIN_LINK_CONGESTED] = READ_ONCE(fc_host->fpin_stats.cn_device_specific);
> -	pre[IBMVFC_AE_FPIN_PORT_CONGESTED] = READ_ONCE(tgt->rport->fpin_stats.cn);
> +	pre[IBMVFC_AE_FPIN_PORT_CONGESTED] = READ_ONCE(tgt->rport->fpin_stats.cn_device_specific);
>  	pre[IBMVFC_AE_FPIN_PORT_CLEARED] = READ_ONCE(tgt->rport->fpin_stats.cn_clear);

[Severity: Medium]
Are we comparing mismatched fields in this test?

The initialization of pre[IBMVFC_AE_FPIN_PORT_CONGESTED] was correctly
updated to capture cn_device_specific, but the corresponding post-event read
(and the second bad-path test initialization) still captures cn.

This mismatch leads to assertions comparing cn_device_specific against cn.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5