Skip to content

Fix timeouts for operations over spidev #205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
xorptr wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix timeouts for operations over spidev #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions examples/htool.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1777,10 +1777,10 @@ static const struct htool_param GLOBAL_FLAGS[] = {
{HTOOL_FLAG_VALUE, .name = "spidev_speed_hz", .default_value = "0",
.desc = "Clock speed (in Hz) to use when using spidev transport. Default "
"behavior (with input 0) is to not change the clock speed"},
{HTOOL_FLAG_VALUE, .name = "spidev_device_busy_wait_timeout",
.default_value = "180000000",
.desc = "Maximum duration (in microseconds) to wait when SPI device "
"indicates that it is busy"},
{HTOOL_FLAG_VALUE, .name = "spidev_page_program_busy_wait_timeout",
.default_value = "10",
.desc = "Maximum duration (in milliseconds) to wait after a page program "
"command when SPI device indicates that it is busy"},
{HTOOL_FLAG_VALUE, .name = "spidev_device_busy_wait_check_interval",
Copy link
Collaborator

@stevenportley stevenportley Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have one spidev parameter here in ms and one in us. Can we just use the same function that the usb_retry logic uses so that it's clear?

.default_value = "100",
.desc = "Interval duration (in microseconds) to wait before checking SPI "
Expand Down
8 changes: 4 additions & 4 deletions examples/htool_spi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ struct libhoth_device* htool_libhoth_spi_device(void) {
uint32_t mailbox_location;
bool atomic;
uint32_t spidev_speed_hz;
uint32_t spidev_device_busy_wait_timeout;
uint32_t spidev_page_program_busy_wait_timeout;
uint32_t spidev_device_busy_wait_check_interval;
rv = htool_get_param_string(htool_global_flags(), "spidev_path",
&spidev_path_str) ||
Expand All @@ -45,8 +45,8 @@ struct libhoth_device* htool_libhoth_spi_device(void) {
htool_get_param_u32(htool_global_flags(), "spidev_speed_hz",
&spidev_speed_hz) ||
htool_get_param_u32(htool_global_flags(),
"spidev_device_busy_wait_timeout",
&spidev_device_busy_wait_timeout) ||
"spidev_page_program_busy_wait_timeout",
&spidev_page_program_busy_wait_timeout) ||
htool_get_param_u32(htool_global_flags(),
"spidev_device_busy_wait_check_interval",
&spidev_device_busy_wait_check_interval);
Expand All @@ -64,7 +64,7 @@ struct libhoth_device* htool_libhoth_spi_device(void) {
.mailbox = mailbox_location,
.atomic = atomic,
.speed = spidev_speed_hz,
.device_busy_wait_timeout = spidev_device_busy_wait_timeout,
.page_program_busy_wait_timeout = spidev_page_program_busy_wait_timeout,
.device_busy_wait_check_interval = spidev_device_busy_wait_check_interval,
};
rv = libhoth_spi_open(&opts, &result);
Expand Down
2 changes: 1 addition & 1 deletion transports/libhoth_device.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ int libhoth_send_request(struct libhoth_device* dev, const void* request,
// be written. Errors if libhoth_send_request() wasn't called previously.
// Returns LIBHOTH_ERR_TIMEOUT if the response is not ready by the
// specified timeout, and the user can call again later. If timeout_ms is zero,
// returns immediately.
// the waiting behavior is implementation defined
// This function is not thread-safe. In multi-threaded contexts, callers must
// ensure libhoth_send_request() and libhoth_receive_response() occur
// atomically (with respect to other calls to those functions).
Expand Down
65 changes: 39 additions & 26 deletions transports/libhoth_spi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ struct libhoth_spi_device {

void* buffered_request;
size_t buffered_request_size;
uint32_t device_busy_wait_timeout;
uint32_t page_program_busy_wait_timeout;
uint32_t device_busy_wait_check_interval;
};

Expand Down Expand Up @@ -99,7 +99,7 @@ static int get_monotonic_ms(uint64_t* time_ms) {
return 0;
}

static libhoth_status spi_nor_busy_wait(const int fd, uint32_t timeout_us,
static libhoth_status spi_nor_busy_wait(const int fd, uint32_t timeout_ms,
uint32_t check_interval_us) {
uint8_t tx_buf[2];
uint8_t rx_buf[2];
Expand Down Expand Up @@ -128,20 +128,22 @@ static libhoth_status spi_nor_busy_wait(const int fd, uint32_t timeout_us,
return LIBHOTH_OK;
}

uint64_t current_time_ms;
if (get_monotonic_ms(&current_time_ms) != 0) {
return LIBHOTH_ERR_FAIL;
}
uint64_t time_elapsed_ms = 0;
if (current_time_ms < start_time_ms) {
// Wrap around
time_elapsed_ms = (UINT64_MAX - start_time_ms) + current_time_ms;
} else {
time_elapsed_ms = current_time_ms - start_time_ms;
}

if (time_elapsed_ms > (timeout_us / 1000)) {
return LIBHOTH_ERR_TIMEOUT;
if (timeout_ms != 0) {
Copy link
Collaborator

@stevenportley stevenportley Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really not want to have a timeout at all for this? I think it might make sense to have a maximum of something very high (e.g. 60s?) so that it at least eventually fails and we return the proper timeout error code to the caller.

uint64_t current_time_ms;
if (get_monotonic_ms(&current_time_ms) != 0) {
return LIBHOTH_ERR_FAIL;
}
uint64_t time_elapsed_ms = 0;
if (current_time_ms < start_time_ms) {
// Wrap around
time_elapsed_ms = (UINT64_MAX - start_time_ms) + current_time_ms;
} else {
time_elapsed_ms = current_time_ms - start_time_ms;
}

if (time_elapsed_ms > timeout_ms) {
return LIBHOTH_ERR_TIMEOUT;
}
}
usleep(check_interval_us);
}
Expand All @@ -167,13 +169,24 @@ static int spi_nor_write_enable(const int fd) {

static int spi_nor_write(int fd, bool address_mode_4b, unsigned int address,
const void* data, size_t data_len,
uint32_t device_busy_wait_timeout,
uint32_t page_program_busy_wait_timeout,
uint32_t device_busy_wait_check_interval) {
if (fd < 0 || !data || !data_len) return LIBHOTH_ERR_INVALID_PARAMETER;

// Page program operations
size_t bytes_sent = 0;
while (bytes_sent < data_len) {
// - For the first iteration, wait for any previous operations that might
// have set the busy bit to finish
// - For rest of the iterations, wait for each page program operation to be
// handled except the last one. Waiting for the last page program will be
Copy link
Collaborator

@stevenportley stevenportley Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you document what this is for?

I presume what you're going for is that we can send a host command and let it process while doing other things?

// done in `libhoth_spi_receive_response` since RoT will take some time to
// execute the host command
libhoth_status busy_wait_status = spi_nor_busy_wait(
Copy link
Collaborator Author

@xorptr xorptr Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the previous htool invocation was interrupted during host command execution, the timeout here may be insufficient. Currently this covers only the case if the previous invocation was interrupted after a intermediate page program operation. Do you folks think that this should handle the first case as well?

fd, page_program_busy_wait_timeout, device_busy_wait_check_interval);
if (busy_wait_status != LIBHOTH_OK) {
return busy_wait_status;
}
// Send write enable before each Page program operation
int status = spi_nor_write_enable(fd);
if (status != LIBHOTH_OK) {
Expand Down Expand Up @@ -206,13 +219,6 @@ static int spi_nor_write(int fd, bool address_mode_4b, unsigned int address,
}
bytes_sent += chunk_send_size;
address += chunk_send_size;

// Wait for each page program operation to be handled
libhoth_status busy_wait_status = spi_nor_busy_wait(
fd, device_busy_wait_timeout, device_busy_wait_check_interval);
if (busy_wait_status != LIBHOTH_OK) {
return busy_wait_status;
}
}
return LIBHOTH_OK;
}
Expand Down Expand Up @@ -363,7 +369,8 @@ int libhoth_spi_open(const struct libhoth_spi_device_init_options* options,
spi_dev->fd = fd;
spi_dev->mailbox_address = options->mailbox;
spi_dev->address_mode_4b = true;
spi_dev->device_busy_wait_timeout = options->device_busy_wait_timeout;
spi_dev->page_program_busy_wait_timeout =
options->page_program_busy_wait_timeout;
spi_dev->device_busy_wait_check_interval =
options->device_busy_wait_check_interval;

Expand Down Expand Up @@ -408,7 +415,7 @@ int libhoth_spi_send_request(struct libhoth_device* dev, const void* request,

return spi_nor_write(spi_dev->fd, spi_dev->address_mode_4b,
spi_dev->mailbox_address, request, request_size,
spi_dev->device_busy_wait_timeout,
spi_dev->page_program_busy_wait_timeout,
spi_dev->device_busy_wait_check_interval);
}

Expand All @@ -429,6 +436,12 @@ int libhoth_spi_receive_response(struct libhoth_device* dev, void* response,
struct libhoth_spi_device* spi_dev =
(struct libhoth_spi_device*)dev->user_ctx;

libhoth_status busy_wait_status = spi_nor_busy_wait(
spi_dev->fd, timeout_ms, spi_dev->device_busy_wait_check_interval);
if (busy_wait_status != LIBHOTH_OK) {
return busy_wait_status;
}

// Read Header From Mailbox
status = spi_nor_read(spi_dev->fd, spi_dev->address_mode_4b,
spi_dev->mailbox_address, response,
Expand Down
2 changes: 1 addition & 1 deletion transports/libhoth_spi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ struct libhoth_spi_device_init_options {
int mode;
int speed;
int atomic;
uint32_t device_busy_wait_timeout;
uint32_t page_program_busy_wait_timeout;
uint32_t device_busy_wait_check_interval;
};

Expand Down
Loading