Re: [PATCH v6 04/13] scsi: fnic: Advertise NVMe initiator service parameters

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The commit sets `iport->service_params` to advertise NVMe initiator service parameters, but this field is never read. The actual PRLI frame is hardcoded to advertise FCP capabilities instead.
--

--- Patch [4]: [PATCH v6 04/13] scsi: fnic: Advertise NVMe initiator service parameters ---
commit ed8174a20b4f25dad8815dfbe2e3def2feb4906c
Author: Karan Tilak Kumar <kartilak@cisco.com>

scsi: fnic: Advertise NVMe initiator service parameters

This commit sets FC service parameters according to the selected initiator
role. It keeps FCP retry and confirmation bits for FCP initiators, and
advertises NVMe initiator and SLER bits for NVMe initiators.

> diff --git a/drivers/scsi/fnic/fnic_main.c b/drivers/scsi/fnic/fnic_main.c
> index 3a365ea455b1a..5850d51b0e8f6 100644
> --- a/drivers/scsi/fnic/fnic_main.c
> +++ b/drivers/scsi/fnic/fnic_main.c
> @@ -1016,11 +1016,16 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	iport->max_flogi_retries = fnic->config.flogi_retries;
>  	iport->max_plogi_retries = fnic->config.plogi_retries;
>  	iport->plogi_timeout = fnic->config.plogi_timeout;
> -	iport->service_params =
> -		(FNIC_FCP_SP_INITIATOR | FNIC_FCP_SP_RD_XRDY_DIS |
> -		 FNIC_FCP_SP_CONF_CMPL);
> -	if (fnic->config.flags & VFCF_FCP_SEQ_LVL_ERR)
> -		iport->service_params |= FNIC_FCP_SP_RETRY;
> +	if (IS_FNIC_FCP_INITIATOR(fnic)) {
> +		iport->service_params = (FNIC_FCP_SP_INITIATOR |
> +				FNIC_FCP_SP_RD_XRDY_DIS | FNIC_FCP_SP_CONF_CMPL);
> +		if (fnic->config.flags & VFCF_FCP_SEQ_LVL_ERR)
> +			iport->service_params |= FNIC_FCP_SP_RETRY;
> +	} else if (IS_FNIC_NVME_INITIATOR(fnic)) {
> +		iport->service_params = (FNIC_NVME_SP_INITIATOR);
> +		if (fnic->config.flags & VFCF_FCP_SEQ_LVL_ERR)
> +			iport->service_params |= FNIC_NVME_SP_SLER;
> +	}

[Severity: High]
Is iport->service_params actually used when sending the PRLI request?

Looking at fdls_send_tgt_prli() in drivers/scsi/fnic/fdls_disc.c, it appears
the PRLI payload hardcodes the FCP parameters directly:

    .sp = {.spp_type = 0x08, .spp_flags = 0x0020,
           .spp_params = cpu_to_be32(0xA2)}

If the service_params field is ignored, will the NVMe initiator feature fall
back to advertising FCP capabilities (type 0x08) instead of NVMe, preventing
successful discovery and login for NVMe targets?

>  
>  	iport->boot_time = jiffies;
>  	iport->e_d_tov = fnic->config.ed_tov;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724174811.5118-1-kartilak@cisco.com?part=4