Re: [PATCH v2 17/33] ibmvfc: add NVMe/FC Port Login support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The newly added NVMe Port Login logic is unreachable because NVMe targets are never allocated.
- [High] A successful NVMe Port Login (PLOGI) will incorrectly proceed to issue a SCSI Process Login (PRLI).
--

--- Patch [17]: [PATCH v2 17/33] ibmvfc: add NVMe/FC Port Login support ---
commit dfa3e967fac7e65d31ae01b75c2fc44ab7b459fd
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

ibmvfc: add NVMe/FC Port Login support

Expand the target login path to issue the NVMe/FC-specific port login
MAD for NVMe targets.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 66fcff7ac834b..adc146214e668 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -4212,7 +4212,7 @@ static void ibmvfc_tgt_plogi_done(struct ibmvfc_event *evt)
>  	ibmvfc_set_tgt_action(tgt, IBMVFC_TGT_ACTION_NONE);
>  	switch (status) {
>  	case IBMVFC_MAD_SUCCESS:
> -		tgt_dbg(tgt, "Port Login succeeded\n");
> +		tgt_dbg(tgt, "%s Port Login succeeded\n", proto_type[tgt->protocol]);

[Severity: High]
If an NVMe target successfully completes a Port Login (PLOGI) here, will it
incorrectly proceed to issue a SCSI Process Login (PRLI)?

After a successful PLOGI, the state machine transitions to send a PRLI:

ibmvfc_tgt_plogi_done() {
    ...
    ibmvfc_init_tgt(tgt, ibmvfc_tgt_send_prli);
    break;
    ...
}

However, ibmvfc_tgt_send_prli() seems to unconditionally issue a SCSI FCP
PRLI without checking if tgt->protocol is NVMe:

ibmvfc_tgt_send_prli() {
    ...
    prli->common.opcode = cpu_to_be32(IBMVFC_PROCESS_LOGIN);
    prli->common.length = cpu_to_be16(sizeof(*prli));
    prli->scsi_id = cpu_to_be64(tgt->scsi_id);
    
    prli->parms.type = IBMVFC_SCSI_FCP_TYPE;
    ...
}

Does this cause a protocol error and target initialization failure for NVMe
targets by sending them a SCSI FCP PRLI?

[ ... ]

> @@ -4288,7 +4288,10 @@ static void ibmvfc_tgt_send_plogi(struct ibmvfc_target *tgt)
>  	} else {
>  		plogi->common.version = cpu_to_be32(1);
>  	}
> -	plogi->common.opcode = cpu_to_be32(IBMVFC_PORT_LOGIN);
> +	if (tgt->protocol == IBMVFC_PROTO_SCSI)
> +		plogi->common.opcode = cpu_to_be32(IBMVFC_PORT_LOGIN);
> +	else
> +		plogi->common.opcode = cpu_to_be32(IBMVFC_NVMF_PORT_LOGIN);

[Severity: High]
Will this NVMe branch ever execute? Looking at ibmvfc_alloc_targets(), it
appears to only allocate SCSI targets, completely ignoring the NVMe discovery
buffer:

ibmvfc_alloc_targets() {
    ...
    for (i = 0, rc = 0; !rc && i < vhost->scsi_scrqs.num_targets; i++)
        rc = ibmvfc_alloc_target(vhost, &vhost->scsi_scrqs.disc_buf[i]);
    ...
}

Since NVMe targets are never instantiated, does this leave the entire NVMe
target discovery and login flow as unreachable dead code?

>  	plogi->common.length = cpu_to_be16(sizeof(*plogi));
>  	plogi->scsi_id = cpu_to_be64(tgt->scsi_id);

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