Re: [PATCH v2 2/2] scsi: qla4xxx: advance the reset-sequence array cursor in poll_read_list

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: qla4xxx: missing initialization of reset template array_index corrupts firmware restart
--

commit f2bbbc943d62eae5bbd2ca3ef4c8394aabc05ad4
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: qla4xxx: advance the reset-sequence array cursor in poll_read_list

This commit fixes qla4_83xx_poll_read_list() to ensure the array_index
cursor is saved back to ha->reset_tmplt on every read. This prevents
multiple reads from overwriting the same slot during the hardware reset
sequence.

> diff --git a/drivers/scsi/qla4xxx/ql4_83xx.c b/drivers/scsi/qla4xxx/ql4_83xx.c
> index 44d285af22727..2ac833c5b1400 100644
> --- a/drivers/scsi/qla4xxx/ql4_83xx.c
> +++ b/drivers/scsi/qla4xxx/ql4_83xx.c
> @@ -1117,7 +1117,8 @@ static void qla4_83xx_poll_read_list(struct scsi_qla_host *ha,
>  				ha->reset_tmplt.array[index++] = value;
>  
>  				if (index == QLA83XX_MAX_RESET_SEQ_ENTRIES)
> -					ha->reset_tmplt.array_index = 1;
> +					index = 1;
> +				ha->reset_tmplt.array_index = index;

[Severity: High]
Since struct scsi_qla_host is zero-initialized, ha->reset_tmplt.array_index
starts at 0. During the first execution of qla4_83xx_poll_read_list(), the
polled value will be placed at array[0].

However, looking at qla4_83xx_rmw_crb_reg(), an index of 0 is treated as a
hardware bypass flag to read the register directly instead of the array:

qla4_83xx_rmw_crb_reg() {
	...
	if (p_rmw_hdr->index_a)
		value = ha->reset_tmplt.array[p_rmw_hdr->index_a];
	else
		qla4_83xx_rd_reg_indirect(ha, raddr, &value);
	...
}

Does this make array[0] completely inaccessible to the template? The wrap
around check setting index = 1 suggests the template logic assumes 1-based
indexing. If the first polled value is written to array[0], won't all later
read-modify-write operations read from the wrong array slot?

Additionally, when attempting firmware recovery via qla4_83xx_restart(),
qla4_83xx_process_stop_seq() resets ha->reset_tmplt.seq_index to 0:

qla4_83xx_process_stop_seq() {
	ha->reset_tmplt.seq_index = 0;
	qla4_83xx_process_reset_template(ha, ha->reset_tmplt.stop_offset);
}

But it appears to omit resetting ha->reset_tmplt.array_index to 1. On
subsequent reset attempts, does this cause the cursor to drift continually
rather than starting fresh, completely desynchronizing the polled values
from the template execution?

>  			}
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011040257.2017421-1-yhlee@isslab.korea.ac.kr?part=2