Propagate errors in more places

Change-Id: I5a7594d4b44c524537827f403348d0c10814546f

src/target/riscv/riscv-013.c

@@ -39,7 +39,7 @@ static void riscv013_clear_abstract_error(struct target *target);
 static int riscv013_get_register(struct target *target,
 		riscv_reg_t *value, int hid, int rid);
 static int riscv013_set_register(struct target *target, int hartid, int regid, uint64_t value);
-static void riscv013_select_current_hart(struct target *target);
+static int riscv013_select_current_hart(struct target *target);
 static int riscv013_halt_current_hart(struct target *target);
 static int riscv013_resume_current_hart(struct target *target);
 static int riscv013_step_current_hart(struct target *target);
@@ -479,7 +479,8 @@ static dmi_status_t dmi_scan(struct target *target, uint32_t *address_in,
 	return buf_get_u32(in, DTM_DMI_OP_OFFSET, DTM_DMI_OP_LENGTH);
 }
 
-static int dmi_read(struct target *target, uint32_t *value, uint32_t address)
+static int dmi_op(struct target *target, uint32_t *data_in, int dmi_op,
+		uint32_t address, uint32_t data_out)
 {
 	select_dmi(target);
 
@@ -488,49 +489,66 @@ static int dmi_read(struct target *target, uint32_t *value, uint32_t address)
 
 	unsigned i = 0;
 
-	/* This first loop ensures that the read request was actually sent
+	const char *op_name;
-	 * to the target. Note that if for some reason this stays busy,
+	switch (dmi_op) {
-	 * it is actually due to the previous dmi_read or dmi_write. */
+		case DMI_OP_NOP:
+			op_name = "nop";
+			break;
+		case DMI_OP_READ:
+			op_name = "read";
+			break;
+		case DMI_OP_WRITE:
+			op_name = "write";
+			break;
+		default:
+			LOG_ERROR("Invalid DMI operation: %d", dmi_op);
+			return ERROR_FAIL;
+	}
+
+	/* This first loop performs the request.  Note that if for some reason this
+	 * stays busy, it is actually due to the previous access. */
 	for (i = 0; i < 256; i++) {
-		status = dmi_scan(target, NULL, NULL, DMI_OP_READ, address, 0,
+		status = dmi_scan(target, NULL, NULL, dmi_op, address, data_out,
 				false);
 		if (status == DMI_STATUS_BUSY) {
 			increase_dmi_busy_delay(target);
 		} else if (status == DMI_STATUS_SUCCESS) {
 			break;
 		} else {
-			LOG_ERROR("failed read from 0x%x, status=%d", address, status);
+			LOG_ERROR("failed %s at 0x%x, status=%d", op_name, address, status);
 			return ERROR_FAIL;
 		}
 	}
 
 	if (status != DMI_STATUS_SUCCESS) {
-		LOG_ERROR("Failed read from 0x%x; status=%d", address, status);
+		LOG_ERROR("Failed %s at 0x%x; status=%d", op_name, address, status);
 		return ERROR_FAIL;
 	}
 
-	/* This second loop ensures that we got the read
+	/* This second loop ensures the request succeeded, and gets back data.
-	 * data back. Note that NOP can result in a 'busy' result as well, but
+	 * Note that NOP can result in a 'busy' result as well, but that would be
-	 * that would be noticed on the next DMI access we do. */
+	 * noticed on the next DMI access we do. */
 	for (i = 0; i < 256; i++) {
-		status = dmi_scan(target, &address_in, value, DMI_OP_NOP, address, 0,
+		status = dmi_scan(target, &address_in, data_in, DMI_OP_NOP, address, 0,
 				false);
 		if (status == DMI_STATUS_BUSY) {
 			increase_dmi_busy_delay(target);
 		} else if (status == DMI_STATUS_SUCCESS) {
 			break;
 		} else {
-			LOG_ERROR("failed read (NOP) at 0x%x, status=%d", address, status);
+			LOG_ERROR("failed %s (NOP) at 0x%x, status=%d", op_name, address,
+					status);
 			return ERROR_FAIL;
 		}
 	}
 
 	if (status != DMI_STATUS_SUCCESS) {
-		if (status == DMI_STATUS_FAILED) {
+		if (status == DMI_STATUS_FAILED || !data_in) {
-			LOG_ERROR("Failed read (NOP) from 0x%x; status=%d", address, status);
+			LOG_ERROR("Failed %s (NOP) at 0x%x; status=%d", op_name, address,
+					status);
 		} else {
-			LOG_ERROR("Failed read (NOP) from 0x%x; value=0x%x, status=%d",
+			LOG_ERROR("Failed %s (NOP) at 0x%x; value=0x%x, status=%d",
-					address, *value, status);
+					op_name, address, *data_in, status);
 		}
 		return ERROR_FAIL;
 	}
@@ -538,53 +556,14 @@ static int dmi_read(struct target *target, uint32_t *value, uint32_t address)
 	return ERROR_OK;
 }
 
+static int dmi_read(struct target *target, uint32_t *value, uint32_t address)
+{
+	return dmi_op(target, value, DMI_OP_READ, address, 0);
+}
+
 static int dmi_write(struct target *target, uint32_t address, uint32_t value)
 {
-	select_dmi(target);
+	return dmi_op(target, NULL, DMI_OP_WRITE, address, value);
-	dmi_status_t status = DMI_STATUS_BUSY;
-	unsigned i = 0;
-
-	/* The first loop ensures that we successfully sent the write request. */
-	for (i = 0; i < 256; i++) {
-		status = dmi_scan(target, NULL, NULL, DMI_OP_WRITE, address, value,
-				address == DMI_COMMAND);
-		if (status == DMI_STATUS_BUSY) {
-			increase_dmi_busy_delay(target);
-		} else if (status == DMI_STATUS_SUCCESS) {
-			break;
-		} else {
-			LOG_ERROR("failed write to 0x%x, status=%d", address, status);
-			break;
-		}
-	}
-
-	if (status != DMI_STATUS_SUCCESS) {
-		LOG_ERROR("Failed write to 0x%x;, status=%d",
-				address, status);
-		return ERROR_FAIL;
-	}
-
-	/* The second loop isn't strictly necessary, but would ensure that the
-	 * write is complete/ has no non-busy errors before returning from this
-	 * function. */
-	for (i = 0; i < 256; i++) {
-		status = dmi_scan(target, NULL, NULL, DMI_OP_NOP, address, 0,
-				false);
-		if (status == DMI_STATUS_BUSY) {
-			increase_dmi_busy_delay(target);
-		} else if (status == DMI_STATUS_SUCCESS) {
-			break;
-		} else {
-			LOG_ERROR("failed write (NOP) at 0x%x, status=%d", address, status);
-			break;
-		}
-	}
-	if (status != DMI_STATUS_SUCCESS) {
-		LOG_ERROR("failed to write (NOP) 0x%x to 0x%x; status=%d", value, address, status);
-		return ERROR_FAIL;
-	}
-
-	return ERROR_OK;
 }
 
 int dmstatus_read(struct target *target, uint32_t *dmstatus,
@@ -1352,7 +1331,8 @@ static int examine(struct target *target)
 			continue;
 
 		r->current_hartid = i;
-		riscv013_select_current_hart(target);
+		if (riscv013_select_current_hart(target) != ERROR_OK)
+			return ERROR_FAIL;
 
 		uint32_t s;
 		if (dmstatus_read(target, &s, true) != ERROR_OK)
@@ -2480,14 +2460,15 @@ static int riscv013_set_register(struct target *target, int hid, int rid, uint64
 	return ERROR_OK;
 }
 
-static void riscv013_select_current_hart(struct target *target)
+static int riscv013_select_current_hart(struct target *target)
 {
 	RISCV_INFO(r);
 
 	uint32_t dmcontrol;
-	dmi_read(target, &dmcontrol, DMI_DMCONTROL);
+	if (dmi_read(target, &dmcontrol, DMI_DMCONTROL) != ERROR_OK)
+		return ERROR_FAIL;
 	dmcontrol = set_field(dmcontrol, hartsel_mask(target), r->current_hartid);
-	dmi_write(target, DMI_DMCONTROL, dmcontrol);
+	return dmi_write(target, DMI_DMCONTROL, dmcontrol);
 }
 
 static int riscv013_halt_current_hart(struct target *target)

src/target/riscv/riscv.c

@@ -737,19 +737,20 @@ static int old_or_new_riscv_resume(
 		return riscv_openocd_resume(target, current, address, handle_breakpoints, debug_execution);
 }
 
-static void riscv_select_current_hart(struct target *target)
+static int riscv_select_current_hart(struct target *target)
 {
 	RISCV_INFO(r);
 	if (r->rtos_hartid != -1 && riscv_rtos_enabled(target))
-		riscv_set_current_hartid(target, r->rtos_hartid);
+		return riscv_set_current_hartid(target, r->rtos_hartid);
 	else
-		riscv_set_current_hartid(target, target->coreid);
+		return riscv_set_current_hartid(target, target->coreid);
 }
 
 static int riscv_read_memory(struct target *target, target_addr_t address,
 		uint32_t size, uint32_t count, uint8_t *buffer)
 {
-	riscv_select_current_hart(target);
+	if (riscv_select_current_hart(target) != ERROR_OK)
+		return ERROR_FAIL;
 	struct target_type *tt = get_target_type(target);
 	return tt->read_memory(target, address, size, count, buffer);
 }
@@ -757,7 +758,8 @@ static int riscv_read_memory(struct target *target, target_addr_t address,
 static int riscv_write_memory(struct target *target, target_addr_t address,
 		uint32_t size, uint32_t count, const uint8_t *buffer)
 {
-	riscv_select_current_hart(target);
+	if (riscv_select_current_hart(target) != ERROR_OK)
+		return ERROR_FAIL;
 	struct target_type *tt = get_target_type(target);
 	return tt->write_memory(target, address, size, count, buffer);
 }
@@ -775,7 +777,8 @@ static int riscv_get_gdb_reg_list(struct target *target,
 		return ERROR_FAIL;
 	}
 
-	riscv_select_current_hart(target);
+	if (riscv_select_current_hart(target) != ERROR_OK)
+		return ERROR_FAIL;
 
 	switch (reg_class) {
 		case REG_CLASS_GENERAL:
@@ -964,24 +967,29 @@ int riscv_blank_check_memory(struct target *target,
 
 /*** OpenOCD Helper Functions ***/
 
-/* 0 means nothing happened, 1 means the hart's state changed (and thus the
+enum riscv_poll_hart {
- * poll should terminate), and -1 means there was an error. */
+	RPH_NO_CHANGE,
-static int riscv_poll_hart(struct target *target, int hartid)
+	RPH_CHANGE,
+	RPH_ERROR
+};
+static enum riscv_poll_hart riscv_poll_hart(struct target *target, int hartid)
 {
 	RISCV_INFO(r);
-	riscv_set_current_hartid(target, hartid);
+	if (riscv_set_current_hartid(target, hartid) != ERROR_OK)
+		return RPH_ERROR;
 
-	LOG_DEBUG("polling hart %d, target->state=%d (TARGET_HALTED=%d)", hartid, target->state, TARGET_HALTED);
+	LOG_DEBUG("polling hart %d, target->state=%d (TARGET_HALTED=%d)", hartid,
+			target->state, TARGET_HALTED);
 
-	/* If OpenOCD this we're running but this hart is halted then it's time
+	/* If OpenOCD thinks we're running but this hart is halted then it's time
 	 * to raise an event. */
 	if (target->state != TARGET_HALTED && riscv_is_halted(target)) {
 		LOG_DEBUG("  triggered a halt");
 		r->on_halt(target);
-		return 1;
+		return RPH_CHANGE;
 	}
 
-	return 0;
+	return RPH_NO_CHANGE;
 }
 
 /*** OpenOCD Interface ***/
@@ -992,14 +1000,14 @@ int riscv_openocd_poll(struct target *target)
 	if (riscv_rtos_enabled(target)) {
 		/* Check every hart for an event. */
 		for (int i = 0; i < riscv_count_harts(target); ++i) {
-			int out = riscv_poll_hart(target, i);
+			enum riscv_poll_hart out = riscv_poll_hart(target, i);
 			switch (out) {
-			case 0:
+			case RPH_NO_CHANGE:
 				continue;
-			case 1:
+			case RPH_CHANGE:
 				triggered_hart = i;
 				break;
-			case -1:
+			case RPH_ERROR:
 				return ERROR_FAIL;
 			}
 		}
@@ -1018,8 +1026,12 @@ int riscv_openocd_poll(struct target *target)
 		for (int i = 0; i < riscv_count_harts(target); ++i)
 			riscv_halt_one_hart(target, i);
 	} else {
-		if (riscv_poll_hart(target, riscv_current_hartid(target)) == 0)
+		enum riscv_poll_hart out = riscv_poll_hart(target,
+				riscv_current_hartid(target));
+		if (out == RPH_NO_CHANGE)
 			return ERROR_OK;
+		else if (out == RPH_ERROR)
+			return ERROR_FAIL;
 
 		triggered_hart = riscv_current_hartid(target);
 		LOG_DEBUG("  hart %d halted", triggered_hart);
@@ -1042,6 +1054,8 @@ int riscv_openocd_poll(struct target *target)
 	case RISCV_HALT_UNKNOWN:
 		target->debug_reason = DBG_REASON_UNDEFINED;
 		break;
+	case RISCV_HALT_ERROR:
+		return ERROR_FAIL;
 	}
 
 	if (riscv_rtos_enabled(target)) {
@@ -1562,7 +1576,8 @@ int riscv_halt_one_hart(struct target *target, int hartid)
 {
 	RISCV_INFO(r);
 	LOG_DEBUG("halting hart %d", hartid);
-	riscv_set_current_hartid(target, hartid);
+	if (riscv_set_current_hartid(target, hartid) != ERROR_OK)
+		return ERROR_FAIL;
 	if (riscv_is_halted(target)) {
 		LOG_DEBUG("  hart %d requested halt, but was already halted", hartid);
 		return ERROR_OK;
@@ -1588,7 +1603,8 @@ int riscv_resume_one_hart(struct target *target, int hartid)
 {
 	RISCV_INFO(r);
 	LOG_DEBUG("resuming hart %d", hartid);
-	riscv_set_current_hartid(target, hartid);
+	if (riscv_set_current_hartid(target, hartid) != ERROR_OK)
+		return ERROR_FAIL;
 	if (!riscv_is_halted(target)) {
 		LOG_DEBUG("  hart %d requested resume, but was already resumed", hartid);
 		return ERROR_OK;
@@ -1609,7 +1625,8 @@ int riscv_step_rtos_hart(struct target *target)
 			hartid = 0;
 		}
 	}
-	riscv_set_current_hartid(target, hartid);
+	if (riscv_set_current_hartid(target, hartid) != ERROR_OK)
+		return ERROR_FAIL;
 	LOG_DEBUG("stepping hart %d", hartid);
 
 	if (!riscv_is_halted(target)) {
@@ -1659,33 +1676,35 @@ bool riscv_rtos_enabled(const struct target *target)
 	return target->rtos != NULL;
 }
 
-void riscv_set_current_hartid(struct target *target, int hartid)
+int riscv_set_current_hartid(struct target *target, int hartid)
 {
 	RISCV_INFO(r);
 	if (!r->select_current_hart)
-		return;
+		return ERROR_FAIL;
 
 	int previous_hartid = riscv_current_hartid(target);
 	r->current_hartid = hartid;
 	assert(riscv_hart_enabled(target, hartid));
 	LOG_DEBUG("setting hartid to %d, was %d", hartid, previous_hartid);
-	r->select_current_hart(target);
+	if (r->select_current_hart(target) != ERROR_OK)
+		return ERROR_FAIL;
 
 	/* This might get called during init, in which case we shouldn't be
 	 * setting up the register cache. */
 	if (!target_was_examined(target))
-		return;
+		return ERROR_OK;
 
 	/* Avoid invalidating the register cache all the time. */
 	if (r->registers_initialized
 			&& (!riscv_rtos_enabled(target) || (previous_hartid == hartid))
 			&& target->reg_cache->reg_list[GDB_REGNO_ZERO].size == (unsigned)riscv_xlen(target)
 			&& (!riscv_rtos_enabled(target) || (r->rtos_hartid != -1))) {
-		return;
+		return ERROR_OK;
 	} else
 		LOG_DEBUG("Initializing registers: xlen=%d", riscv_xlen(target));
 
 	riscv_invalidate_register_cache(target);
+	return ERROR_OK;
 }
 
 void riscv_invalidate_register_cache(struct target *target)
@@ -1775,7 +1794,8 @@ bool riscv_is_halted(struct target *target)
 enum riscv_halt_reason riscv_halt_reason(struct target *target, int hartid)
 {
 	RISCV_INFO(r);
-	riscv_set_current_hartid(target, hartid);
+	if (riscv_set_current_hartid(target, hartid) != ERROR_OK)
+		return RISCV_HALT_ERROR;
 	if (!riscv_is_halted(target)) {
 		LOG_ERROR("Hart is not halted!");
 		return RISCV_HALT_UNKNOWN;

src/target/riscv/riscv.h

@@ -31,7 +31,8 @@ enum riscv_halt_reason {
 	RISCV_HALT_BREAKPOINT,
 	RISCV_HALT_SINGLESTEP,
 	RISCV_HALT_TRIGGER,
-	RISCV_HALT_UNKNOWN
+	RISCV_HALT_UNKNOWN,
+	RISCV_HALT_ERROR
 };
 
 typedef struct {
@@ -93,7 +94,7 @@ typedef struct {
 		riscv_reg_t *value, int hid, int rid);
 	int (*set_register)(struct target *, int hartid, int regid,
 			uint64_t value);
-	void (*select_current_hart)(struct target *);
+	int (*select_current_hart)(struct target *);
 	bool (*is_halted)(struct target *target);
 	int (*halt_current_hart)(struct target *);
 	int (*resume_current_hart)(struct target *target);
@@ -191,7 +192,7 @@ bool riscv_rtos_enabled(const struct target *target);
 
 /* Sets the current hart, which is the hart that will actually be used when
  * issuing debug commands. */
-void riscv_set_current_hartid(struct target *target, int hartid);
+int riscv_set_current_hartid(struct target *target, int hartid);
 int riscv_current_hartid(const struct target *target);
 
 /*** Support functions for the RISC-V 'RTOS', which provides multihart support