Skip to content

target/riscv: Fix some timeout check order #1265

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.

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
wants to merge 3 commits into
base: riscv
Choose a base branch
from

Conversation

lz-bro
Copy link
Contributor

@lz-bro lz-bro commented Jun 13, 2025

Reorder the timeout check and conditional judgment to ensure that timeout check is not wasted.

en-sc and others added 2 commits May 7, 2025 09:37
Ubuntu 20.04 is no longer available.
See actions/runner-images#11101

Checkpatch-ignore: BAD_SIGN_OFF
Change-Id: I0ec3e3342f9212a2a79d8dca6274e7db62ecedab
Signed-off-by: Evgeniy Naydanov <evgeniy.naydanov@syntacore.com>
when Debug Module is inactive, accesses to the nextdm may fail.
Specifically, nextdm might not return correct data.
@lz-bro lz-bro changed the title Reorder timeout check target/riscv: Fix some timeout check order Jun 13, 2025
MarekVCodasip
MarekVCodasip previously approved these changes Jun 13, 2025
Copy link
Collaborator

@MarekVCodasip MarekVCodasip left a comment

Choose a reason for hiding this comment

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

Thanks for adding the change elsewhere as well.

Comment on lines 1894 to 1896
if (!get_field32(dmcontrol, DM_DMCONTROL_DMACTIVE))
break;
Copy link
Collaborator

@JanMatCodasip JanMatCodasip Jun 16, 2025

Choose a reason for hiding this comment

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

Detail:

During the refactor, the debug message DM reset initiated got removed. I am not sure if this was intended or simply an oversight.

I would prefer to keep the message there. The message can also be slightly reworded for clarity.

if (!get_field32(dmcontrol, DM_DMCONTROL_DMACTIVE)) {
	LOG_TARGET_DEBUG(target, "The DM has just become deactivated (dmcontrol.dmactive: 1 -> 0).");
	break;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed, thanks.

Reorder the timeout check and conditional judgment
to ensure that timeout check is not wasted.
Copy link
Collaborator

@en-sc en-sc left a comment

Choose a reason for hiding this comment

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

@lz-bro, to be honest, I don't understand the need for this patch.

  1. dmi_write() is time-bound by the same riscv_get_command_timeout_sec() (see
    static int batch_run_timeout(struct target *target, struct riscv_batch *batch)
    ), therefore this is mostly a hypothetical issue.
  2. IMHO, this is more straightforward when an operation that took longer then the timeout fails, indicating that you need to increase the timeout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants