Re: [PATCH v2 08/60] scsi: qla2xxx: Add BSG MPI firmware load/dump for 29xx

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The implementations of LOAD and DUMP MPI commands are semantically reversed, leading to firmware data corruption.
- [High] Incorrect address scaling in `qla29xx_mpi_optrom_data()` mixes byte offsets with word counts during burst transfers, causing corrupted memory chunks.
- [Medium] Applying Option ROM (Flash) bounds to MPI RAM addresses causes spurious rejections and truncated transfers.
--

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

scsi: qla2xxx: Add BSG MPI firmware load/dump for 29xx

Add BSG vendor commands for loading and dumping MPI firmware on
29xx adapters. This extends the existing BSG infrastructure with
the necessary mailbox wrappers and flash helpers for MPI
operations.

> diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
> index 0baf486e8fb89..92a0bc6dc7bcd 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
> @@ -1756,6 +1756,153 @@ static int qla29xx_bsg_flash_block_write(struct bsg_job *bsg_job)
>  	return 0;
>  }
>  
> +static int qla2900_bsg_dump_mpi(struct bsg_job *bsg_job)
> +{
[ ... ]
> +	mutex_lock(&ha->optrom_mutex);
> +	rval = qla2x00_optrom_setup(bsg_job, vha, dmcmd->mpi_address, 1);

[Severity: Medium]
Will this correctly validate the MPI RAM boundaries? It looks like
qla2x00_optrom_setup() validates the start parameter against ha->optrom_size,
which is the physical size of the flash chip, not the MPI RAM size. Could
this falsely reject or truncate valid MPI RAM addresses if they exceed the
flash size limits?

> +	if (rval) {
> +		mutex_unlock(&ha->optrom_mutex);
> +		return rval;
> +	}
> +
> +	sg_copy_to_buffer(bsg_job->request_payload.sg_list,
> +			  bsg_job->request_payload.sg_cnt, ha->optrom_buffer,
> +			  ha->optrom_region_size);
[ ... ]
> +	rval = qla29xx_mpi_optrom_data(vha, opts, ha->optrom_buffer,
> +				       ha->optrom_region_start,
> +				       ha->optrom_region_size,
> +				       QLA29XX_MPI_OP_DUMP);

[Severity: High]
Are the semantics of DUMP and LOAD reversed here? The function is named
qla2900_bsg_dump_mpi(), but it copies data from the user's request_payload
and passes QLA29XX_MPI_OP_DUMP, which acts as a Write operation to the
device. Usually, dumping firmware implies reading it from the device into a
buffer.

[ ... ]
> +static int qla2900_bsg_load_mpi(struct bsg_job *bsg_job)
> +{
[ ... ]
> +	mutex_lock(&ha->optrom_mutex);
> +	rval = qla2x00_optrom_setup(bsg_job, vha, lmcmd->mpi_address, 0);

[Severity: Medium]
Similar to qla2900_bsg_dump_mpi(), this passes lmcmd->mpi_address to
qla2x00_optrom_setup(). Could this result in the transfer being silently
truncated if the requested MPI RAM address space differs from the flash
size constraints?

> +	if (rval) {
[ ... ]
> +	rval = qla29xx_mpi_optrom_data(vha, opts, ha->optrom_buffer,
> +				       ha->optrom_region_start,
> +				       ha->optrom_region_size,
> +				       QLA29XX_MPI_OP_LOAD);
[ ... ]
> +	sg_copy_from_buffer(bsg_job->reply_payload.sg_list,
> +			    bsg_job->reply_payload.sg_cnt,
> +			    ha->optrom_buffer,
> +			    ha->optrom_region_size);

[Severity: High]
This function is named qla2900_bsg_load_mpi(), but it appears to be reading
data from the device into the reply_payload (a Read operation). Standard
firmware conventions typically define LOAD as sending data to the device.
Could invoking these commands overwrite the adapter's MPI RAM with
uninitialized data due to the reversed directions?

> diff --git a/drivers/scsi/qla2xxx/qla_sup.c b/drivers/scsi/qla2xxx/qla_sup.c
> index 1d5ea5e432edc..af7509083910d 100644
> --- a/drivers/scsi/qla2xxx/qla_sup.c
> +++ b/drivers/scsi/qla2xxx/qla_sup.c
> @@ -527,6 +527,123 @@ qla29xx_read_optrom_data(struct scsi_qla_host *vha, uint16_t reg_code,
[ ... ]
> +int
> +qla29xx_mpi_optrom_data(struct scsi_qla_host *vha, uint16_t opts, void *buf,
> +		       uint32_t offset, uint32_t length, enum qla29xx_mpi_optrom_op op)
> +{
[ ... ]
> +	mpi_addr = offset;
> +	dcode = (uint32_t *)optrom;
> +	memset(dcode, 0, OPTROM_BURST_SIZE);
> +
> +	fwcode = (uint32_t *)buf;
> +	mpi_size = length >> 2;
> +	burst = OPTROM_BURST_SIZE >> 2;
> +	total_chunks = (mpi_size + burst - 1) / burst;
> +
> +	while (mpi_size > 0) {
[ ... ]
> +		chunk_count++;
> +		fwcode += burst;
> +		mpi_addr += burst;
> +		mpi_size -= burst;
> +	}

[Severity: High]
Does this loop mix byte offsets with word counts? It looks like mpi_addr is
initialized to offset (a byte address from the user parameter), but it is
advanced by burst. Since burst is calculated as OPTROM_BURST_SIZE >> 2
(a count of 32-bit words), mpi_addr will only advance by a quarter of the
intended byte distance each iteration. Will this cause successive transfers
to heavily overlap and corrupt the firmware load or dump?

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