Commit 06746448 authored by Seth Howell's avatar Seth Howell Committed by Jim Harris
Browse files

nvme: fix confusion around nvme_ctrlr_set_state 



In most places, we are passing NVME_TIMEOUT_INFINITE as the
timeout_in_ms argument to nvme_ctrlr_set_state, presumably in an attempt
to specify an infinite timeout. However, nvme_ctrlr_set_state only
checked against 0 when setting the actual timeout, and we didn't have
any logic to check for overflow so we just ended up setting random
timeout_tsc values which changes the behavior of the
nvme_ctrlr_process_init function in several places.

So, change NVME_TIMEOUT_INFINITE to 0, and add some integer overflow
checking to nvme_ctrlr_set_state.

Change-Id: Ic9d0cc57ed153df30c3b20313c3742072a5f992d
Signed-off-by: default avatarSeth Howell <seth.howell@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/469485


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
Reviewed-by: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: default avatarAlexey Marchuk <alexeymar@mellanox.com>
parent a10d0ce7

lib/nvme/nvme_ctrlr.c

+26 −8
Original line number Diff line number Diff line
@@ -828,16 +828,34 @@ static void 
nvme_ctrlr_set_state(struct spdk_nvme_ctrlr *ctrlr, enum nvme_ctrlr_state state,

		     uint64_t timeout_in_ms)

{

	uint64_t ticks_per_ms, timeout_in_ticks, now_ticks;



	ctrlr->state = state;

	if (timeout_in_ms == 0) {

	if (timeout_in_ms == NVME_TIMEOUT_INFINITE) {

		goto inf;

	}



	ticks_per_ms = spdk_get_ticks_hz() / 1000;

	if (timeout_in_ms > UINT64_MAX / ticks_per_ms) {

		SPDK_ERRLOG("Specified timeout would cause integer overflow. Defaulting to no timeout.\n");

		goto inf;

	}



	now_ticks = spdk_get_ticks();

	timeout_in_ticks = timeout_in_ms * ticks_per_ms;

	if (timeout_in_ticks > UINT64_MAX - now_ticks) {

		SPDK_ERRLOG("Specified timeout would cause integer overflow. Defaulting to no timeout.\n");

		goto inf;

	}



	ctrlr->state_timeout_tsc = timeout_in_ticks + now_ticks;

	SPDK_DEBUGLOG(SPDK_LOG_NVME, "setting state to %s (timeout %" PRIu64 " ms)\n",

		      nvme_ctrlr_state_string(ctrlr->state), ctrlr->state_timeout_tsc);

	return;

inf:

	SPDK_DEBUGLOG(SPDK_LOG_NVME, "setting state to %s (no timeout)\n",

		      nvme_ctrlr_state_string(ctrlr->state));

	ctrlr->state_timeout_tsc = NVME_TIMEOUT_INFINITE;

	} else {

		SPDK_DEBUGLOG(SPDK_LOG_NVME, "setting state to %s (timeout %" PRIu64 " ms)\n",

			      nvme_ctrlr_state_string(ctrlr->state), timeout_in_ms);

		ctrlr->state_timeout_tsc = spdk_get_ticks() + (timeout_in_ms * spdk_get_ticks_hz()) / 1000;

	}

}



static void

lib/nvme/nvme_internal.h

+1 −1
Original line number Diff line number Diff line
@@ -559,7 +559,7 @@ enum nvme_ctrlr_state { 
	NVME_CTRLR_STATE_ERROR

};



#define NVME_TIMEOUT_INFINITE	UINT64_MAX

#define NVME_TIMEOUT_INFINITE	0



/*

 * Used to track properties for all processes accessing the controller.