Re: [PATCH v3 14/88] scsi: qla2xxx: Enable get_fw_version mailbox for 29xx

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mpi_version and pep_version sysfs attributes return empty strings for 29xx adapters.
- [Medium] EDIF enablement logic for 29xx adapters is unreachable dead code.
--

--- Patch [14]: [PATCH v3 14/88] scsi: qla2xxx: Enable get_fw_version mailbox for 29xx ---
commit 2382364dd411bb453d1411f7e7ec1a1bea0e802e
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Enable get_fw_version mailbox for 29xx

The serdes_version and several firmware capability fields were not
populated for 29xx because the get_fw_version mailbox path excluded
it from the 27xx/28xx checks. This commit adds IS_QLA29XX() to the
relevant conditionals so that firmware version, EDIF, and serdes
information are correctly retrieved on 29xx adapters.

> diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
> index e8755ab86b6ac..c44f5282abb39 100644
> --- 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 IS_QLA29XX() is being added here to expose the serdes version, should it
also be added to qla2x00_mpi_version_show() and qla2x00_pep_version_show() in
the same file?

Without it, userspace tools querying the mpi_version and pep_version sysfs
attributes for 29xx adapters will receive empty strings instead of the actual
firmware version data:

drivers/scsi/qla2xxx/qla_attr.c:qla2x00_mpi_version_show() {
    ...
    if (!IS_QLA81XX(ha) && !IS_QLA8031(ha) && !IS_QLA8044(ha) &&
        !IS_QLA27XX(ha) && !IS_QLA28XX(ha))
        return scnprintf(buf, PAGE_SIZE, "\n");
    ...
}

[ ... ]

> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> index 9c2633ca5036c..0feb98b83293a 100644
> --- 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;
>  			ql_log(ql_log_info, vha, 0xffff,

[Severity: Medium]
Will this EDIF enablement logic ever execute for 29xx adapters?

Looking at qla2x00_execute_fw() in the same file, ha->flags.edif_hw is only
initialized for 28xx adapters:

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

Because ha->flags.edif_hw is never set for 29xx adapters, this patched
condition will always evaluate to false, leaving EDIF permanently disabled
on 29xx hardware.

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