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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Double completion of BSG job in qla2900_bsg_load_mpi() on error path.
- [High] The implementations of QL_VND_LOAD_MPI and QL_VND_DUMP_MPI are completely swapped, breaking API semantics.
--

--- Patch [8]: [PATCH v5 08/56] scsi: qla2xxx: Add BSG MPI firmware load/dump for 29xx ---
commit 90e9d607deb3d4b5831bfa77970f0b380447e7b0
Author: Manish Rangankar <mrangankar@marvell.com>
scsi: qla2xxx: Add BSG MPI firmware load/dump for 29xx

This commit adds BSG vendor commands for loading and dumping MPI firmware
on 29xx adapters. It 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 92a1af81c057f..5dd7e5e969ab0 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
[ ... ]
> +static int qla2900_bsg_dump_mpi(struct bsg_job *bsg_job)
> +{
[ ... ]
> +	mutex_lock(&ha->optrom_mutex);
> +	rval = qla29xx_mpi_optrom_setup(bsg_job, vha, dmcmd->mpi_address, 1);

[Severity: High]
Does this function implement the wrong operation? Setting is_update=1 here
causes qla29xx_mpi_optrom_setup() to set ha->optrom_state to QLA_SWRITING.

> +	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);

And this copies data from the host's request_payload, suggesting this is
actually a load (host-to-device) operation rather than a dump.

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

Conversely, this function is meant for loading but passes is_update=0, putting
the optrom into the QLA_SREADING state.

[ ... ]
> +	sg_copy_from_buffer(bsg_job->reply_payload.sg_list,
> +			    bsg_job->reply_payload.sg_cnt,
> +			    ha->optrom_buffer,
> +			    ha->optrom_region_size);
> +
> +	bsg_reply->reply_payload_rcv_len = ha->optrom_region_size;
> +	vfree(ha->optrom_buffer);
> +	ha->optrom_buffer = NULL;
> +	ha->optrom_state = QLA_SWAITING;
> +	mutex_unlock(&ha->optrom_mutex);
> +	bsg_job->reply_len = sizeof(struct fc_bsg_reply);
> +	bsg_job_done(bsg_job, bsg_reply->result,
> +		     bsg_reply->reply_payload_rcv_len);
> +
> +	return rval;
> +}

[Severity: High]
Can this cause a double completion of the bsg_job?

If qla29xx_mpi_optrom_data() fails, rval is set to an error code
(e.g., -EINVAL). The function calls bsg_job_done() directly to complete
the request, but then returns the non-zero rval to the caller.

The caller (qla2x00_process_vendor_specific) passes this non-zero return value
up to the BSG layer. If a handler returns a non-zero error, the transport
layer's error path will typically call bsg_job_done() a second time on the
same job, which can lead to list corruption or a use-after-free.

Should this return 0 instead of rval, or avoid calling bsg_job_done() when
returning an error? (Note that qla2900_bsg_dump_mpi() safely returns 0).

> diff --git a/drivers/scsi/qla2xxx/qla_sup.c b/drivers/scsi/qla2xxx/qla_sup.c
> index c30ec9cab7bba..3b0a0cfe361dd 100644
> --- a/drivers/scsi/qla2xxx/qla_sup.c
> +++ b/drivers/scsi/qla2xxx/qla_sup.c
[ ... ]
> +		if (op == QLA29XX_MPI_OP_DUMP) {
> +			options |= (BIT_0);
> +			options |= opts;
[ ... ]
> +			memcpy(dcode, fwcode, burst * 4);
> +
> +			rval = qla29xx_load_dump_mpi(vha, options, mpi_addr, burst,
> +						     optrom_dma);

[Severity: High]
Does QLA29XX_MPI_OP_DUMP actually perform a load? It copies the host buffer
(fwcode) to the DMA buffer (dcode) before issuing the mailbox command, which
sends data to the device.

> +		} else if (op == QLA29XX_MPI_OP_LOAD) {
> +			options |= opts;
[ ... ]
> +			rval = qla29xx_load_dump_mpi(vha, options, mpi_addr, burst,
> +						     optrom_dma);
[ ... ]
> +			memcpy(fwcode, dcode, burst * 4);
> +		}

And conversely, OP_LOAD issues the mailbox command first and then copies
data from the DMA buffer to the host buffer, which performs a dump from the
device. Should the DUMP and LOAD implementations be swapped?

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