Re: [PATCH v6 08/18] nvme: Implement cross-controller reset recovery

[Mohamed Khalfella <mkhalfella@purestorage.com>]

On Wed 2026-10-21 10:29:52 +0200, Daniel Wagner wrote:
> On Tue, Oct 20, 2026 at 04:56:24PM -0700, Mohamed Khalfella wrote:
> > > This part could be made a bit simpler and there is no need for the local
> > > completed variable:
> > > 
> > > [...]
> > >         if (!wait_for_completion_timeout(&ccr.complete, tmo))
> > >                 ret = -ETIMEDOUT;
> > > 
> > > out:
> > >         spin_lock_irqsave(&sctrl->lock, flags);
> > >         list_del(&ccr.list);
> > >         spin_unlock_irqrestore(&sctrl->lock, flags);
> > > 
> > >         if (ret || !completion_done(&ccr.complete))
> > >                 return ret;
> > >         if (ccr.ccrs == NVME_CCR_STATUS_SUCCESS)
> > >                 return 0;
> > >         return -EREMOTEIO;
> > > }
> > 
> > You are right, this is deleting "completed" local variable at the
> > expense of checking the ccr.complete again. I find the current code
> > easier to read and reason about. I prefer to keep it as it is.
> 
> The point is that with the local variable, the reader has to track back
> and work out where it is set. I had to do exactly that while reviewing.
> Putting !completion_done() in the condition makes it self-documenting
> and ties it directly to the wait_for_completion_timeout() above.

I think this is a subjective point. For me seeing completion_done()
after wait_for_completion_timeout() raises the question "Do we not
already know if CCR completed or not? Why are we checking again?"