Re: [PATCH v5 23/56] scsi: qla2xxx: Use ring-slot helpers in __qla2x00_alloc_iocbs

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the refactored code retains a dangerous flaw: the driver uses the standard library `memset()` function to zero an IOCB slot that, for QLAFX00 adapters, resides in PCI MMIO space. In `__qla2x00_alloc_iocbs()`, `qla_req_ring_slot()` retrieves the ring slot pointer and assigns it to `pkt`. For QLAFX00 adapters, the request ring is mapped directly into device memory (`req->ring = (void __force *)ha->iobase + ha->req_que_off;`), meaning `pkt` is an `__iomem` pointer. Calling `memset()` on MMIO memory can generate unaligned or byte-wise bus transactions. On strict-alignment architectures like ARM64 or SPARC, this triggers a synchronous external abort (kernel oops) and crashes the system. The subsequent `if (IS_QLAFX00(ha))` block correctly casts the fields back to `__iomem` to use `wrt_reg_byte()` and `wrt_reg_dword()`, demonstrating that the memory is indeed MMIO, but the unsafe `memset()` remains.
--

--- Patch [23]: [PATCH v5 23/56] scsi: qla2xxx: Use ring-slot helpers in __qla2x00_alloc_iocbs ---
commit 30045e77ead5341377665972b3b6c891e41c1958
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Use ring-slot helpers in __qla2x00_alloc_iocbs

This commit refactors __qla2x00_alloc_iocbs to use qla_req_ring_slot()
and qla_req_entry_size() helpers for ring pointer selection and entry
size. It consolidates the branching logic for QLA29XX and standard
adapters into a unified memset.

> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
> index aab7905c0d6ca..1ca50fc200d65 100644
> --- a/drivers/scsi/qla2xxx/qla_iocb.c
> +++ b/drivers/scsi/qla2xxx/qla_iocb.c

[ ... ]

> @@ -2530,13 +2529,8 @@ __qla2x00_alloc_iocbs(struct qla_qpair *qpair, srb_t *sp)
>  	 * layout-compatible once we return the pkt as request_t *.
>  	 */
>  	req->cnt -= req_cnt;
> -	if (IS_QLA29XX(ha)) {
> -		pkt = (request_t *)req->ring_ext_ptr;
> -		memset(pkt, 0, REQUEST_ENTRY_SIZE_EXT);
> -	} else {
> -		pkt = req->ring_ptr;
> -		memset(pkt, 0, REQUEST_ENTRY_SIZE);
> -	}
> +	pkt = qla_req_ring_slot(ha, req);
> +	memset(pkt, 0, qla_req_entry_size(ha));

[Severity: High]
This isn't a bug introduced by this patch, but could using the standard
memset() here cause unaligned or byte-wise bus transactions on QLAFX00
adapters?

For QLAFX00, the request ring is mapped directly into device memory, meaning
pkt points to __iomem. The subsequent if (IS_QLAFX00(ha)) block correctly
casts the fields back to __iomem to use wrt_reg_byte() and wrt_reg_dword(),
demonstrating that the memory is indeed MMIO.

Could calling memset() directly on this MMIO pointer trigger a synchronous
external abort or alignment fault on strict-alignment architectures like
ARM64 or SPARC, since it is not using an I/O specific function like
memset_io()?

>  	if (IS_QLAFX00(ha)) {
>  		wrt_reg_byte((u8 __force __iomem *)&pkt->entry_count, req_cnt);
>  		wrt_reg_dword((__le32 __force __iomem *)&pkt->handle, handle);

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