Re: [PATCH v6 04/56] scsi: qla2xxx: Add 29xx support in queue initialisation path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Base request and response queues (queue index 0) and fallback frees ignore the new 128-byte size requirement for 29xx hardware, leading to memory corruption and leaks.
- [High] Ring pointer arithmetic for `out_ptr` and `in_ptr` calculates incorrect offsets for QLA29XX adapters, causing memory corruption and breaking bisectability.
- [Medium] The firmware dump logic hardcodes the 64-byte legacy queue sizes, which will result in an undersized dump buffer and truncated queue data for 29xx adapters.
--

--- Patch [4]: [PATCH v6 04/56] scsi: qla2xxx: Add 29xx support in queue initialisation path ---
commit 892bfd1ade358d1fd826e15942668d47268a93f3
Author: Manish Rangankar <mrangankar@marvell.com>

scsi: qla2xxx: Add 29xx support in queue initialisation path

This commit extends the queue initialization and multi-queue management mailbox
commands for 29xx adapters. It introduces 128-byte entry structures and
updates DMA allocations to account for the larger entries on 29xx.

> Extend the queue initialisation and multi-queue management mailbox
> commands to include IS_QLA29XX() checks, following the same mailbox
> interface as 27xx/28xx.
> 
> Unlike earlier adapters that use 64-byte request/response ring entries
> (request_t / response_t), 29xx uses 128-byte entries.  Add struct
> request_ext and struct response_ext, which extend the legacy 64-byte
> layout with a 64-byte reserved area.  The first 64 bytes are
> layout-compatible with the legacy structures, so common header
> accesses remain valid.
> 
> The enlarged entry stride doubles the DMA ring memory allocated for
> both request and response queues on 29xx, and all ring pointer
> arithmetic must account for the wider entries (handled by later
> patches in this series).

[Severity: Critical]
Does this commit update the base request and response queues (queue index 0)
and fallback frees for 29xx hardware?

Looking at qla2x00_mem_alloc() in drivers/scsi/qla2xxx/qla_os.c, it allocates
the base request queue with a hardcoded 64-byte size:

        (*req)->ring = dma_alloc_coherent(&ha->pdev->dev,
                ((*req)->length + 1) * sizeof(request_t),
                &(*req)->dma, GFP_KERNEL);

And similarly in qla2x00_free_req_que(), it uses sizeof(request_t) to free
the queue. Will this cause the hardware to operate with 128-byte entries on a
queue allocated for 64-byte entries, leading to memory corruption and DMA
allocator corruption on 29xx adapters?

[Severity: Medium]
Will the firmware dump logic truncate the 128-byte entries on 29xx adapters?

Looking at qla2x00_alloc_fw_dump() in drivers/scsi/qla2xxx/qla_init.c, it
calculates the dump size assuming 64-byte queue entries:

        req_q_size = req->length * sizeof(request_t);
        rsp_q_size = rsp->length * sizeof(response_t);

And qla24xx_dump_ram() in drivers/scsi/qla2xxx/qla_dbg.c copies only 64 bytes
per entry:

        memcpy(ptr, req->ring, req->length * sizeof(request_t));

Could this result in an undersized dump buffer and truncated queue data for
29xx hardware?

> diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c
> index c563133f751e8..e75b7ae22bc52 100644
> --- a/drivers/scsi/qla2xxx/qla_mid.c
> +++ b/drivers/scsi/qla2xxx/qla_mid.c

[ ... ]

> @@ -706,6 +715,7 @@ qla25xx_create_req_que(struct qla_hw_data *ha, uint16_t options,
>  	uint16_t que_id = 0;
>  	device_reg_t *reg;
>  	uint32_t cnt;
> +	uint16_t reqsz;
>  
>  	req = kzalloc_obj(struct req_que);
>  	if (req == NULL) {
> @@ -714,9 +724,12 @@ qla25xx_create_req_que(struct qla_hw_data *ha, uint16_t options,
>  		goto failed;
>  	}
>  
> +	reqsz = IS_QLA29XX(ha) ? sizeof(struct request_ext) :
> +				 sizeof(request_t);
> +
>  	req->length = REQUEST_ENTRY_CNT_24XX;
>  	req->ring = dma_alloc_coherent(&ha->pdev->dev,
> -			(req->length + 1) * sizeof(request_t),
> +			(req->length + 1) * reqsz,
>  			&req->dma, GFP_KERNEL);

[Severity: High]
Since pointer arithmetic is handled in later patches in this series, does this
break bisectability here?

Looking at qla25xx_create_req_que() in drivers/scsi/qla2xxx/qla_mid.c, the
out_ptr sits after the ring buffer:

        req->out_ptr = (uint16_t *)(req->ring + req->length);

Because req->ring is a request_t pointer (64-byte stride), adding req->length
advances it by length * 64 bytes. For 29xx, the queue stride is 128 bytes, so
the shadow pointer will point into the middle of the allocated ring buffer
instead of immediately after it. Will this cause memory corruption when the
driver or hardware accesses the shadow pointers?

The same issue seems to apply to rsp->in_ptr in qla25xx_create_rsp_que().

Is there a way to make this commit safe for bisection?

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