Re: [PATCH v4 02/56] scsi: qla2xxx: Add flash read/write interface for 29xx

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The newly added qla29xx flash read/write functions are not hooked to the isp_ops struct, breaking sysfs/BSG flash access.
- [Medium] Integer overflow in total_chunks and total_segments variables, causing an infinite loop in flash reads/writes
- [Low] The check_and_set_mbc_bits macro lacks a do { ... } while(0) wrapper, violating coding guidelines and causing potential compilation errors in unbraced conditionals.
--

--- Patch [2]: [PATCH v4 02/56] scsi: qla2xxx: Add flash read/write interface for 29xx ---
commit cc824b368b84bc7c06a17348bb30ee20ef746c82
Author: Manish Rangankar <mrangankar@marvell.com>

scsi: qla2xxx: Add flash read/write interface for 29xx

This patch adds the mailbox wrappers and helpers needed for flash read and 
write operations on the 29xx series adapters. It provides the infrastructure 
to parse the flash layout table and execute block reads and writes.

> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
[ ... ]
> @@ -5348,6 +5355,14 @@ static inline bool qla_vha_mark_busy(scsi_qla_host_t *vha)
>  /*
>   * Flash support definitions
>   */
> +#define check_and_set_mbc_bits(bopt, dopt, bit_to_check, bit_to_set) {	\
> +	if (bopt & bit_to_check)			\
> +		dopt |= bit_to_set;			\
> +}

[Severity: Low]
This isn't a bug, but this macro is defined as a block rather than with a
do-while(0) loop. Could this cause compilation errors if used inside an
unbraced if statement followed by an else?

> +
> +#define SET_FW_BIT(__opts, bit) ((__opts) |= (bit))
> +#define CLEAR_FW_BIT(__opts, bit) ((__opts) &= ~(bit))
[ ... ]
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -3237,6 +3237,7 @@ qla2x00_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
>  		ha->flash_data_off = ~0;
>  		ha->nvram_conf_off = ~0;
>  		ha->nvram_data_off = ~0;
> +		ha->flt_segment_length = QLA_SEGMENT_LENGTH;
>  	}

[Severity: High]
The commit message mentions adding the necessary hooks in isp_ops so that the
existing flash infrastructure can drive the new hardware. However, it looks
like the patch fails to provide the wrapper functions to map the 6-argument
qla29xx_read_optrom_data to the 4-argument isp_ops->read_optrom.

Does this mean sysfs/BSG flash access will fall back to legacy routines and
break due to ha->flash_data_off being ~0?

[ ... ]
> diff --git a/drivers/scsi/qla2xxx/qla_sup.c b/drivers/scsi/qla2xxx/qla_sup.c
> --- a/drivers/scsi/qla2xxx/qla_sup.c
> +++ b/drivers/scsi/qla2xxx/qla_sup.c
[ ... ]
> +qla29xx_write_optrom_data(struct scsi_qla_host *vha, uint16_t reg_code,
> +			 uint16_t opts, void *buf, uint32_t offset,
> +			 uint32_t length)
> +{
[ ... ]
> +	uint16_t total_segments, segment_index = 0;
> +	uint16_t chunk_index = 0, chunk_count = 0;
[ ... ]
> +	while (region_dlen > 0) {
> +		uint32_t segment_size, total_chunks;
> +		uint16_t options = 0;
> +
> +		segment_size = (region_dlen > seg_dlen) ? seg_dlen : region_dlen;
> +		total_chunks = (segment_size + OPTROM_BURST_DWORDS - 1) /
> +				OPTROM_BURST_DWORDS;
[ ... ]
> +		chunk_index++;
> +		chunk_count++;
> +		if (chunk_index >= total_chunks) {

[Severity: Medium]
Will this loop ever terminate if total_chunks is greater than 65535?

Because chunk_index is declared as a uint16_t while total_chunks is a 
uint32_t, chunk_index will wrap around to 0. 

Could total_segments also suffer from integer truncation since it is 
declared as a uint16_t?

> +			chunk_index = 0;
> +			segment_index++;
> +			region_dlen -= segment_size;
> +		}
> +	}
[ ... ]
> +qla29xx_read_optrom_data(struct scsi_qla_host *vha, uint16_t reg_code,
> +			 uint16_t opts, void *buf, uint32_t offset,
> +			 uint32_t length)
> +{
[ ... ]
> +	uint16_t total_segments, segment_index = 0;
> +	uint16_t chunk_index = 0, chunk_count = 0;
[ ... ]
> +	while (region_dlen > 0) {
> +		uint32_t segment_size, total_chunks;
> +		uint16_t options = 0;
> +
> +		segment_size = (region_dlen > seg_dlen) ? seg_dlen : region_dlen;
> +		total_chunks = (segment_size + OPTROM_BURST_DWORDS - 1) /
> +				OPTROM_BURST_DWORDS;
[ ... ]
> +		chunk_index++;
> +		chunk_count++;
> +		if (chunk_index >= total_chunks) {

[Severity: Medium]
Similarly, does this read path also suffer from the same uint16_t wrap 
issue for chunk_index and total_segments?

> +			chunk_index = 0;
> +			segment_index++;
> +			region_dlen -= segment_size;
> +		}
> +	}

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