Re: [PATCH v5 38/56] scsi: qla2xxx: Add LS4 pass-through IOCB handling for 29xx series

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The `vp_idx` field in `struct qla_nvme_lsrjt_pt_arg` is defined as `u8`, which silently truncates the 16-bit Virtual Port index to 8 bits when sending NVMe LS Rejects, breaking 9-bit VP index support for 29xx adapters.
- [High] Unsolicited NVMe LS request handling (`PT_LS4_UNSOL`) lacks support for the 9-bit Virtual Port index on 29xx adapters.

Pre-existing issues:
- [High] `qla_nvme_ls_reject_iocb()` modifies the shared hardware request ring without holding the hardware queue lock (`qp_lock_ptr`).
--

--- Patch [38]: [PATCH v5 38/56] scsi: qla2xxx: Add LS4 pass-through IOCB handling for 29xx series ---
commit 5707bda86601ab783dbcbdb4edacb7475a352b7d
Author: Manish Rangankar <mrangankar@marvell.com>

scsi: qla2xxx: Add LS4 pass-through IOCB handling for 29xx series

Extend the LS4 pass-through IOCB handling to support the 128-byte
pt_ls4_request_ext layout used by 29xx series adapters. The extension
grows inline DSD capacity from 2 to 5 entries. Function signatures are
widened to void * so both layouts can be passed without casts.

> diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c
> index 8fa980db6ce4e..0038b6274d444 100644
> --- a/drivers/scsi/qla2xxx/qla_nvme.c
> +++ b/drivers/scsi/qla2xxx/qla_nvme.c
> @@ -1197,37 +1197,57 @@ static void qla_nvme_fc_format_rjt(void *buf, u8 ls_cmd, u8 reason,

[ ... ]

> +	if (IS_QLA29XX(ha)) {
> +		struct pt_ls4_request_ext *ext = lsrjt_iocb;
> +
> +		ext->vp_index = cpu_to_le16(a->vp_idx);

[Severity: High]
Does this assignment truncate the 16-bit virtual port index to 8 bits?

The struct qla_nvme_lsrjt_pt_arg defines vp_idx as a u8. When an unsolicited
NVMe LS request fails, qla2xxx_process_purls_pkt() assigns the 16-bit
vha->vp_idx to a.vp_idx, dropping the high bits:

qla2xxx_process_purls_pkt()
    a.vp_idx = vha->vp_idx;

This truncates any virtual port index >= 256. When qla_nvme_lsrjt_pt_iocb()
then zero-extends this back to 16 bits for the extended IOCB here, the NVMe
LS Reject IOCB will be routed to the wrong virtual port.

> +		ext->tx_byte_count = cpu_to_le32(a->tx_byte_count);
> +		ext->rx_byte_count = 0;
> +		put_unaligned_le64(a->tx_addr, &ext->dsd[0].address);
> +		ext->dsd[0].length = cpu_to_le32(a->tx_byte_count);
> +		put_unaligned_le64(a->rx_addr, &ext->dsd[1].address);
> +		ext->dsd[1].length = 0;
> +	} else {

[ ... ]

> @@ -1245,13 +1265,13 @@ static int
>  qla_nvme_ls_reject_iocb(struct scsi_qla_host *vha, struct qla_qpair *qp,
>  			struct qla_nvme_lsrjt_pt_arg *a, bool is_xchg_terminate)
>  {
> -	struct pt_ls4_request *lsrjt_iocb;
> +	void *lsrjt_iocb;
>  
>  	lsrjt_iocb = __qla2x00_alloc_iocbs(qp, NULL);

[Severity: High]
This is a pre-existing issue, but does qla_nvme_ls_reject_iocb() modify
the shared hardware request ring without holding the hardware queue lock?

When called from the DPC workqueue thread via qla2xxx_process_purls_pkt(),
the caller does not hold the queue lock (qp_lock_ptr):

vha->iocb_work (workqueue thread)
  qla2xxx_process_purls_pkt()
    qla_nvme_ls_reject_iocb(vha, vha->hw->base_qpair, &a, true);

This function calls __qla2x00_alloc_iocbs() here, and later calls
qla2x00_start_iocbs(), both of which require the lock to safely update
the request ring state. Can this concurrent modification corrupt the ring
structure?

>  	if (!lsrjt_iocb) {
>  		ql_log(ql_log_warn, vha, 0x210e,
>  		       "qla2x00_alloc_iocbs failed.\n");
>  		return QLA_FUNCTION_FAILED;
>  	}

[ ... ]

> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index f539190dd5044..186c6c7a39448 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -8402,6 +8402,7 @@ qla2x00_module_init(void)
>  	BUILD_BUG_ON(sizeof(struct nvram_24xx) != 512);
>  	BUILD_BUG_ON(sizeof(struct nvram_81xx) != 512);
>  	BUILD_BUG_ON(sizeof(struct pt_ls4_request) != 64);
> +	BUILD_BUG_ON(sizeof(struct pt_ls4_request_ext) != 128);
>  	BUILD_BUG_ON(sizeof(struct pt_ls4_rx_unsol) != 64);

[Severity: High]
Does the handling for unsolicited NVMe LS requests also need a 29xx
extension to support 9-bit virtual port indices?

The struct pt_ls4_rx_unsol defines vp_index as an 8-bit uint8_t. When the
29xx firmware passes an unsolicited NVMe LS request to the driver,
qla2xxx_process_purls_iocb() extracts the 8-bit vp_index to look up the
virtual port:

qla2xxx_process_purls_iocb()
    vha = qla2xxx_get_vha_from_vp_idx(ha, p->vp_index);

If the incoming request is for a virtual port >= 256, truncating the 9-bit
index provided by the 29xx firmware will cause the driver to route the
request to the wrong virtual port.

>  	BUILD_BUG_ON(sizeof(struct purex_entry_24xx) != 64);
>  	BUILD_BUG_ON(sizeof(struct purex_entry_24xx_ext) != 128);
>  	BUILD_BUG_ON(sizeof(struct qla2100_fw_dump) != 123634);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717152904.3155730-1-njavali@marvell.com?part=38