Re: [PATCH v4 14/56] scsi: qla2xxx: Enable get_fw_version mailbox for 29xx

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing QLA29XX hardware state initialization in `qla2x00_execute_fw()` results in dead code and missing features.
- [High] Missing macro and memory allocation updates for QLA29XX EDIF support lead to unallocated memory pools.
- [Medium] The patch populates `mpi_version` and `pep_version` capability fields for 29XX adapters but fails to expose them to userspace due to missing sysfs updates.

Pre-existing issues:
- [Critical] Incorrect mempool_free on a DMA pool allocation in the error path.
--

--- Patch [14]: [PATCH v4 14/56] scsi: qla2xxx: Enable get_fw_version mailbox for 29xx ---
commit 7ec86c4145546172d16b2ee833700898350294f6
Author: Nilesh Javali <njavali@marvell.com>
Subject: scsi: qla2xxx: Enable get_fw_version mailbox for 29xx

This patch populates the serdes_version and several firmware capability fields
for 29xx adapters by including them in the get_fw_version mailbox path. It
adds IS_QLA29XX() checks to the relevant conditionals so that firmware version,
EDIF, and serdes information are retrieved correctly.

> diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
> --- a/drivers/scsi/qla2xxx/qla_attr.c
> +++ b/drivers/scsi/qla2xxx/qla_attr.c
> @@ -1526,7 +1526,7 @@ qla2x00_serdes_version_show(struct device *dev, struct device_attribute *attr,
>  	scsi_qla_host_t *vha = shost_priv(class_to_shost(dev));
>  	struct qla_hw_data *ha = vha->hw;
>  
> -	if (!IS_QLA27XX(ha) && !IS_QLA28XX(ha))
> +	if (!IS_QLA27XX(ha) && !IS_QLA28XX(ha) && !IS_QLA29XX(ha))
>  		return scnprintf(buf, PAGE_SIZE, "\n");

[Severity: Medium]
Since we are exposing the serdes_version for 29xx here, should we also
update qla2x00_mpi_version_show() and qla2x00_pep_version_show() in this
same file?

The patch populates ha->mpi_version and ha->pep_version in
qla2x00_get_fw_version() for 29xx, but the sysfs show functions still
restrict output to older models. This causes them to return an empty line,
hiding the newly available data.

[ ... ]

> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
> @@ -1212,7 +1212,7 @@ qla2x00_get_fw_version(scsi_qla_host_t *vha)
>  			vha->flags.nvme2_enabled = 1;
>  		}
>  
> -		if (IS_QLA28XX(ha) && ha->flags.edif_hw && ql2xsecenable &&
> +		if ((IS_QLA28XX(ha) || IS_QLA29XX(ha)) && ha->flags.edif_hw && ql2xsecenable &&
>  		    (ha->fw_attributes_ext[0] & FW_ATTR_EXT0_EDIF)) {
>  			ha->flags.edif_enabled = 1;

[Severity: High]
Will ha->flags.edif_hw ever actually be set for 29xx adapters?

Looking at qla2x00_execute_fw() in qla_mbx.c, the initialization seems to be
missing the IS_QLA29XX check:

qla_mbx.c:qla2x00_execute_fw()
    if (IS_QLA28XX(ha) && (mcp->mb[5] & EDIF_HW_SUPPORT)) {
        ha->flags.edif_hw = 1;

If ha->flags.edif_hw remains 0, won't this new check in get_fw_version() act
as dead code for 29xx?

Additionally, qla2x00_execute_fw() appears to be missing 29xx checks for
NVME_ENABLE_FLAG and for initializing ha->max_supported_speed.

[Severity: High]
If EDIF is enabled on a 29xx adapter, will this lead to unallocated memory
pools?

The EDIF_CAP macro in qla_edif.h only checks IS_QLA28XX. Furthermore, in
qla_os.c:qla2x00_mem_alloc(), the memory pools are allocated like this:

qla_os.c:qla2x00_mem_alloc()
    if (ql2xenabledif || (IS_QLA28XX(ha) && ql2xsecenable)) {
        ha->ctx_mempool = mempool_create_slab_pool(...)

Without IS_QLA29XX here, ha->ctx_mempool and ha->dl_dma_pool won't be
allocated when ql2xsecenable=1 on a 29xx adapter. Could this cause null
pointer dereferences when the driver attempts to use EDIF paths?

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential memory corruption
hazard in the error handling paths for type 6 SCSI commands.

In qla_iocb.c:qla_start_scsi_type6() and qla2xxx_dif_start_scsi_mq(),
sp->u.scmd.crc_ctx is allocated from a DMA pool:

    sp->u.scmd.crc_ctx = dma_pool_zalloc(ha->dl_dma_pool, GFP_ATOMIC, ...);

However, if an error occurs later (e.g., mapping fails or resources run
out), the error path frees it using a different mempool:

    mempool_free(sp->u.scmd.crc_ctx, ha->ctx_mempool);

Since ha->ctx_mempool is a slab-backed mempool (meant for struct ct6_dsd)
and not a DMA pool, doesn't freeing a DMA-backed address to this slab pool
corrupt memory?

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