Re: [PATCH v9 4/9] scsi: ibmvfc: extend async event handlers for async sub-CRQ events

[Tyrel Datwyler <tyreld@linux.ibm.com>]

On 9/10/26 7:39 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] ibmvfc_find_target() incorrectly skips targets unless they support BOTH SCSI and NVMe, causing FPIN events to be dropped.
> - [Medium] Missing scsi_id extraction for async sub-CRQ events.
> - [Medium] Incorrect WWPN populated in FPIN descriptor for async sub-CRQ events when subq->wwpn is 0.
> - [Low] Missing address-of operator '&' when passing host_lock to spin_lock_irqsave() and spin_unlock_irqrestore(), causing a build failure.
> --
> 
> commit 997e4bca2edfad65478e2c572b7c236ba500e1bb
> Author: Dave Marquardt <davemarq@linux.ibm.com>
> 
> scsi: ibmvfc: extend async event handlers for async sub-CRQ events
> 
> This patch refactors async event handling to support both traditional async
> CRQs and new asynchronous sub-queue CRQs using a tagged union. It also
> updates FPIN processing logic to extract identifiers from the appropriate
> event type.
> 
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index 6b2eb82ceb98d..daec81f48857d 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)
>> +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));
>> +}
> 
> [Severity: Medium]
> If the async sub-CRQ event arrives with a WWPN of 0 but the target is
> successfully matched via node_name, will this embed a WWPN of 0 into the
> FPIN descriptor?
> 
> It looks like fc_host_fpin_rcv() would then fail to find the remote port by
> WWPN, silently dropping the FPIN and missing statistics updates. Would it be
> safer to pass in the matched tgt->wwpn here, similar to how it's done for
> standard CRQs?

The wwpn and node name should never be zero.

> 
>>  /**
>>   * ibmvfc_find_target - Search for a target in a target list
>>   * @target_list: list head of targets to search
>> @@ -3463,28 +3502,39 @@ 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;
>> +	__be64 wwpn;
>>  
>>  	aw = container_of_const(work, struct ibmvfc_async_work, async_work_s);
>>  	vhost = aw->vhost;
>> -	crq = &aw->crq;
>> +	if (aw->event.type == IBMVFC_ASYNC_CRQ_SUB) {
>> +		subq = &aw->event.subq;
>> +		scsi_id = 0;
> 
> [Severity: Medium]
> The commit message states we "extract WWPN, node_name, and scsi_id from the
> appropriate union member based on event type."
> 
> However, for async sub-CRQ events here and in ibmvfc_handle_async(), scsi_id
> is hardcoded to 0 instead of extracting nport_id from the payload. Could this
> cause target lookup failures if WWPN and node_name happen to be 0?

Again wwpn and node_name should never be zero here.

> 
>> +		wwpn = subq->wwpn;
>> +		node_name = (subq->flags & IBMVFC_ASYNC_ID_IS_ASSOC_ID) ? 0 : subq->id.node_name;
>> +	} else {
>> +		crq = &aw->event.async_crq;
>> +		scsi_id = crq->scsi_id;
>> +		wwpn = crq->wwpn;
>> +		node_name = crq->node_name;
>> +	}
>>  
>> -	if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
>> +	if (!scsi_id && !wwpn && !node_name)
>>  		goto free;
>>  
>>  	spin_lock_irqsave(vhost->host->host_lock, flags);
>> -	tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, crq->scsi_id,
>> -				 crq->wwpn, crq->node_name);
>> +	tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, scsi_id, wwpn, node_name);
> 
> [Severity: High]
> Does ibmvfc_find_target() incorrectly skip targets unless they support BOTH
> SCSI and NVMe?
> 
> Looking at the implementation of ibmvfc_find_target():
> 
>     if (!tgt->rport || !tgt->nvme_remote_port)
>         continue;
> 
> Because a target represents a single endpoint for a specific protocol (SCSI or
> NVMe), it will only ever have either tgt->rport or tgt->nvme_remote_port,
> never both.
> 
> Because of the logical OR, this condition always evaluates to true. Could this
> effectively break FPIN event handling by causing every target to be skipped?

This was covered in patch 1 and will be fixed in v10.

> 
> [ ... ]
>> @@ -3512,25 +3566,51 @@ static void ibmvfc_process_async_work(struct work_struct *work)
>>  
>>  /**
>>   * ibmvfc_handle_async - Handle an async event from the adapter
> [ ... ]
>>  	if (ae->type == IBMVFC_ASYNC_CRQ_SUB) {
>> -	switch (be64_to_cpu(crq->event)) {
>> +		subq = &ae->subq;
>> +		event = be16_to_cpu(subq->event);
>> +		link_state = subq->link_state;
>> +		scsi_id = 0;
> 
> [Severity: Medium]
> Similarly, this hardcodes scsi_id to 0 for async sub-CRQ events instead of
> extracting it from the event payload.
>

scsi_id is not part of a subq event payload. We set it to zero to ensure we
don't pass junk  to ibmvfc_find_target.

-Tyrel