Re: [PATCH 2/2] scsi: mpi3mr: Stop IRQ polling when no reply is ready

[Usama Arif <usama.arif@linux.dev>]

On 30/09/2026 16:20, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] scsi: mpi3mr: dropped IRQ poll trigger causes hard-IRQ storm
> --
> 
> commit a00593b528c873a4c3ec905f3fa02c054a4e3bf0
> Author: Usama Arif <usama.arif@linux.dev>
> Subject: scsi: mpi3mr: Stop IRQ polling when no reply is ready
> 
> This commit modifies the IRQ polling logic in the mpi3mr driver to stop
> polling when a post-sleep check finds the queue empty. It introduces a
> barrier and a recheck of pend_ios to avoid losing wakeups from concurrent
> submitters.
> 
>> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
>> index 102f84667a5cf..6f113d8625bb4 100644
>> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
>> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> [ ... ]
>> @@ -819,17 +829,34 @@ static irqreturn_t mpi3mr_isr_poll(int irq, void *privdata)
>>  
>>  		if (!midx)
>>  			mpi3mr_process_admin_reply_q(mrioc);
>> -		num_op_reply +=
>> -		    mpi3mr_process_op_reply_q(mrioc, op_reply_q);
>> +		num_op_reply += mpi3mr_process_op_reply_q(mrioc, op_reply_q,
>> +							  &checked_empty);
>> +		/* Stop only on an empty check made while owning the queue */
>> +		if (slept && checked_empty) {
>> +			idle = true;
>> +			break;
>> +		}
>>  		if (!atomic_read(&op_reply_q->pend_ios))
>>  			break;
>>  
>>  		usleep_range(MPI3MR_IRQ_POLL_SLEEP, 10 * MPI3MR_IRQ_POLL_SLEEP);
>> +		slept = true;
>>  
>>  	} while (num_op_reply < mrioc->max_host_ios);
>>  
>> -	if (op_reply_q)
>> -		op_reply_q->enable_irq_poll = false;
>> +	if (op_reply_q) {
>> +		WRITE_ONCE(op_reply_q->enable_irq_poll, false);
>> +		if (idle) {
>> +			/*
>> +			 * Recheck after clearing, pairs with atomic_inc_return()
>> +			 * in mpi3mr_op_request_post().
>> +			 */
>> +			smp_mb();
>> +			if (atomic_read(&op_reply_q->pend_ios) >
>> +			    MPI3MR_IRQ_POLL_TRIGGER_IOCOUNT)
>> +				WRITE_ONCE(op_reply_q->enable_irq_poll, true);
>> +		}
>> +	}
> 
> [Severity: Medium]
> Could this unconditional clearing of the flag combined with gating the recheck
> behind if (idle) allow a concurrent submitter's trigger to be overwritten and
> lost?
> 
> If the polling thread processes all replies and pend_ios drops to 0, it breaks
> the loop with idle = false.
> 
> If a concurrent submitter in mpi3mr_op_request_post() then posts a burst of
> I/Os, surpasses the trigger threshold, and sets enable_irq_poll = true, the
> polling thread will subsequently execute WRITE_ONCE(op_reply_q->enable_irq_poll, false).
> 
> Because idle is false, the thread skips the smp_mb() barrier and the pend_ios
> recheck. Doesn't this blindly overwrite the submitter's flag and leave
> enable_irq_poll disabled, forcing all completions into the hard IRQ handler?
> 

Yes, it can be overwritten on those exits, but that is not new: before
this patch the thread cleared the flag on its exits without a recheck,
and those exits are unchanged. The recheck is only added to the idle
exit this patch introduces, which stops polling while I/Os can still be
pending.

It also only lasts until the next submission: mpi3mr_op_request_post()
sets the flag on every submission that leaves more than
MPI3MR_IRQ_POLL_TRIGGER_IOCOUNT I/Os pending, not just on the one that
crosses it. Until then the hard IRQ handler processes the replies, as it
does below the trigger or with threaded_isr_poll=0, so no reply is lost.