Re: [PATCH v2 04/60] scsi: qla2xxx: Add get_flash_version support for 29xx adapters

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Inconsistent fallback error handling logic and commit message mismatch in `qla24xx_get_flash_version()`.

Pre-existing issues:
- [High] Hardware flash registers are read/written via `qla24xx_read_flash_dword` without proper locking, creating a race condition with concurrent sysfs operations.
--

commit 94d10a85b890acf6bb363bf65b3e955e838a9522
Author: Manish Rangankar <mrangankar@marvell.com>

scsi: qla2xxx: Add get_flash_version support for 29xx adapters

This commit removes the standalone qla29xx_get_flash_version() and folds 
29xx support directly into qla24xx_get_flash_version(). It uses early 
metadata reads for 29xx firmware and modifies PCI expansion ROM reads.

>   - PCI expansion ROM reads (header + data structure): a new
>     file-static helper, qla24xx_read_pci_rom_chunk(), abstracts the
>     per-generation flash access so both read sites are straight-line
>     calls instead of inline if/else twin blocks.  29xx uses
>     qla29xx_read_optrom_data(FLT_REG_BOOT_CODE, byte-offset); 24xx
>     uses qla24xx_read_flash_data(dword-address).

The commit message mentions a new file-static helper,
qla24xx_read_pci_rom_chunk(), but this function does not appear in the code.
Is the commit message out of sync with the patch?

> diff --git a/drivers/scsi/qla2xxx/qla_sup.c b/drivers/scsi/qla2xxx/qla_sup.c
> index eb10904f14caa..2229c2b084cff 100644
> --- a/drivers/scsi/qla2xxx/qla_sup.c
> +++ b/drivers/scsi/qla2xxx/qla_sup.c
[ ... ]
> @@ -4137,7 +4108,7 @@ qla24xx_get_flash_version(scsi_qla_host_t *vha, void *mbuf)
>  		if (ret) {
>  			ql_log(ql_log_info, vha, 0x017d,
>  			    "Unable to read PCI EXP Rom Header(%x).\n", ret);
> -			return QLA_FUNCTION_FAILED;
> +			break;
>  		}

[Severity: High]
This is a pre-existing issue, but can qla24xx_read_flash_data() race with
concurrent sysfs operations?

Hardware flash registers are read/written via qla24xx_read_flash_dword()
without proper locking:

qla24xx_read_flash_dword() {
	wrt_reg_dword(&reg->flash_addr, addr & ~FARX_DATA_FLAG);
	while (cnt--) {
		if (rd_reg_dword(&reg->flash_addr) & FARX_DATA_FLAG) {
        ...
}

When invoked via the sysfs reset handler:

qla2x00_sysfs_write_reset() {
    ...
    ha->isp_ops->get_flash_version(vha, tmp_data);
    ...
}

It doesn't appear to hold ha->optrom_mutex or ha->hardware_lock. Could
concurrent calls interleave their register accesses and corrupt data?

[Severity: Medium]
When qla24xx_read_flash_data() fails, the code now uses break to fall through
and read the active firmware image version. However, signature validation
failures later in this loop still return QLA_FUNCTION_FAILED:

		bcode = mbuf + (pcihdr % 4);
		if (memcmp(bcode, "\x55\xaa", 2)) {
			/* No signature */
			ql_log(ql_log_fatal, vha, 0x0059,
			    "No matching ROM signature.\n");
			return QLA_FUNCTION_FAILED;
		}

If the flash read succeeds but contains invalid boot code, the function aborts
entirely and skips reading the firmware version, whereas a complete flash read
failure successfully falls through. Is this inconsistent fallback behavior
intentional?

>  
>  		bcode = mbuf + (pcihdr % 4);
> @@ -4155,7 +4126,7 @@ qla24xx_get_flash_version(scsi_qla_host_t *vha, void *mbuf)
>  		if (ret) {
>  			ql_log(ql_log_info, vha, 0x018e,
>  			    "Unable to read PCI Data Structure (%x).\n", ret);
> -			return QLA_FUNCTION_FAILED;
> +			break;
>  		}
>  
>  		bcode = mbuf + (pcihdr % 4);

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