Re: [PATCH v4 05/10] mpi3mr: Fix performance regression caused by extended IRQ poll sleep

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

New issues:
- [High] The threaded IRQ polling loop will loop indefinitely if `pend_ios`=
 becomes negative due to skipped corrupt descriptors.

Pre-existing issues:
- [High] The admin reply queue path reposts unvalidated DMA addresses to th=
e hardware free queue when virtual address lookup fails, causing host memor=
y corruption.
- [High] Missing release memory barrier when unlocking the `in_use` queue l=
ock, allowing race conditions on weakly ordered architectures.
--

--- Patch [5]: [PATCH v4 05/10] mpi3mr: Fix performance regression caused b=
y extended IRQ poll sleep ---
commit 19ca49ca3a484c6837fd5a670f8512d409c2678d
Author: Ranjan Kumar <[email protected]>

mpi3mr: Fix performance regression caused by extended IRQ poll sleep
   =20
This patch restores the IRQ poll sleep base to 2 us and introduces bounds
checking for request queue IDs to prevent infinite polling. It also adds
memory barriers in the admin and operational reply queue processing loops
and fixes a panic condition on malformed DMA reply addresses.

Link: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@br=
oadcom.com?part=3D5
Link: https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar@b=
roadcom.com?part=3D5
Link: https://sashiko.dev/#/patchset/20260724102505.115136-1-ranjan.kumar@b=
roadcom.com?part=3D5

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr=
_fw.c
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c

[ ... ]

> @@ -565,15 +578,33 @@ int mpi3mr_process_op_reply_q(struct mpi3mr_ioc *mr=
ioc,
>  	reply_desc =3D mpi3mr_get_reply_desc(op_reply_q, reply_ci);
>  	if ((le16_to_cpu(reply_desc->reply_flags) &
>  	    MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) !=3D exp_phase) {
> +		/* Recheck under in_use before releasing, to avoid a reclaim race */
> +		dma_rmb();
> +		if ((le16_to_cpu(reply_desc->reply_flags) &
> +		    MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) =3D=3D exp_phase)
> +			goto process_desc;
>  		atomic_dec(&op_reply_q->in_use);

[Severity: High]
This is a pre-existing issue, but is a release memory barrier missing when
unlocking the in_use queue lock?

Both here and at the end of mpi3mr_process_op_reply_q(), the lock state is
released via atomic_dec():

    op_reply_q->ci =3D reply_ci;
    op_reply_q->ephase =3D exp_phase;
    atomic_sub(threshold_comps, &op_reply_q->pend_ios);
    atomic_dec(&op_reply_q->in_use);

Since non-value-returning RMW atomic operations like atomic_dec() do not
imply memory barriers, could weakly ordered architectures reorder the writes
to op_reply_q->ci and op_reply_q->ephase after the in_use decrement?

Could this allow another CPU to acquire in_use but read stale values for ci
and ephase, causing it to double-process reply descriptors?

>  		return 0;
>  	}

[ ... ]

> @@ -744,11 +787,12 @@ static irqreturn_t mpi3mr_isr_poll(int irq, void *p=
rivdata)
>  			num_op_reply +=3D
>  			    mpi3mr_process_op_reply_q(mrioc,
>  				intr_info->op_reply_q);
> +		if (!atomic_read(&intr_info->op_reply_q->pend_ios))
> +			break;

[Severity: High]
Can pend_ios become negative and cause this loop to run indefinitely?

If mpi3mr_process_op_reply_q() encounters a corrupted descriptor:

    if (unlikely(req_q_idx >=3D mrioc->num_op_req_q)) {
        ioc_err(mrioc, "Invalid request queue id %d, skipping reply\n",
            req_q_idx + 1);
        goto next_reply;
    }

It skips the reply but still increments threshold_comps, which is later
subtracted from pend_ios. If the descriptor did not correspond to a tracked
host submission, couldn't this decrement pend_ios below zero?

Since this exact equality check (!atomic_read) evaluates to false for
negative values, will the polling loop fail to break and wait indefinitely?

> -		usleep_range(MPI3MR_IRQ_POLL_SLEEP, MPI3MR_IRQ_POLL_SLEEP + 1);
> +		usleep_range(MPI3MR_IRQ_POLL_SLEEP, 10 * MPI3MR_IRQ_POLL_SLEEP);
> =20
> -	} while (atomic_read(&intr_info->op_reply_q->pend_ios) &&
> -	    (num_op_reply < mrioc->max_host_ios));
> +	} while (num_op_reply < mrioc->max_host_ios);
> =20

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr=
_os.c
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -3430,8 +3430,12 @@ void mpi3mr_process_op_reply_desc(struct mpi3mr_io=
c *mrioc,
>  		scsi_reply =3D mpi3mr_get_reply_virt_addr(mrioc,
>  		    *reply_dma);
>  		if (!scsi_reply) {
> -			panic("%s: scsi_reply is NULL, this shouldn't happen\n",
> -			    mrioc->name);
> +			ioc_err(mrioc, "scsi_reply is NULL, invalid reply_frame_address\n");
> +			/*
> +			 * Do not let the caller repost an address that
> +			 * failed virt-addr lookup back to the hardware.
> +			 */
> +			*reply_dma =3D 0;

[Severity: High]
This isn't a bug introduced by this patch, but does the admin reply queue
path suffer from the same unvalidated DMA address issue fixed here for the
operational queue?

In mpi3mr_process_admin_reply_desc():

    def_reply =3D mpi3mr_get_reply_virt_addr(mrioc, *reply_dma);
    if (!def_reply)
        goto out;

If def_reply is NULL, the function jumps to the out label but fails to
clear *reply_dma. Then in mpi3mr_process_admin_reply_q():

    mpi3mr_process_admin_reply_desc(mrioc, reply_desc, &reply_dma);
    if (reply_dma)
        mpi3mr_repost_reply_buf(mrioc, reply_dma);

Will this cause the caller to unknowingly repost the unvalidated, non-zero
reply_dma back to the hardware free queue, leading to host memory
corruption?

>  			goto out;
>  		}

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260805110634.3466=
[email protected]?part=3D5