Re: [PATCH 3/3] nvme: use helper nvme_ctrl_state in nvme_keep_alive_end_io function

[Keith Busch <kbusch@kernel.org>]

On Mon, Oct 28, 2024 at 10:13:25AM +0530, Nilay Shroff wrote:
> On 10/28/24 03:37, Sagi Grimberg wrote:
> > On 27/10/2024 19:02, Nilay Shroff wrote:
> >> We no more need acquiring ctrl->lock before accessing the
> >> NVMe controller state and instead we can now use the helper
> >> nvme_ctrl_state. So replace the use of ctrl->lock from
> >> nvme_keep_alive_end_io function with nvme_ctrl_state call.
> > 
> > The patch description is not clear. Why do we "no longer need"
> > to take the ctrl->lock? What exactly changed for this?
> > 
> The update of ctrl->state is now done using WRITE_ONCE(). So it 
> makes sense to pair the read of ctrl->state using READ_ONCE(). And
> the helper nvme_ctrl_state(), in fact, reads the ctrl->state
> using READ_ONCE(). So, if we use the nvme_ctrl_state() for 
> retrieving ctrl->state value then we don't need locking. 
> 
> I think I would better format the commit message of this patch
> to describe the above intent.

Makes sense to me. If the code was racing with another thread attempting
to change the state, then we either saw the old state or the new state.
Same is true with using the state accessor without the ctrl->lock.