Skip to content
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

Revert "[opentitantool] Refuse downgrading HyperDebug firmware" #25934

Merged
merged 1 commit into from
Jan 18, 2025

Conversation

jesultra
Copy link
Contributor

@jesultra jesultra commented Jan 18, 2025

This reverts commit 7a0134a, such that opentitantool no longer requires the --force switch in order to downgrade firmware with opentitantool transport update-firmware. Instead, opentitantool will be back to the original behavior of overwriting firmware in any case when the version number does not match exactly.

The original reason for not downgrading was that our branches had diverged, such that they had different firmware versions, resulting in CI machines constantly upgrading/downgrading as CI runs from different branches were scheduled.

PR #25917, however introduced a faulty HyperDebug firmware with never version number, which is now on a number of CI machines, and will not be downgraded automatically, with the result that "innocent" other PRs will have CI failures.

I think it is clear that we should avoid such excessive flashing by keeping the HyperDebug firmware version in sync between branches. (We do not need to keep all of opentitantool logic in sync, just the built-in firmware version.) For now, in order to avoid CI failures, we should merge this PR, and accept some upgrading/downgrading.

@jesultra jesultra added SW:opentitantool CherryPick:earlgrey_es_sival This PR should be cherry-picked to earlgrey_es_sival CherryPick:integrated_dev This PR should be cherry-picked to integrated_dev CherryPick:earlgrey_1.0.0 This PR should be cherry-picked to earlgrey_1.0.0 labels Jan 18, 2025
@jesultra jesultra requested a review from a team as a code owner January 18, 2025 04:53
@nbdd0121
Copy link
Contributor

What's the rationale of making the change?

Copy link
Contributor

@nbdd0121 nbdd0121 left a comment

Choose a reason for hiding this comment

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

The change LGTM and I think it probably makes sense to use exact version, although I think

  1. before/after merging this change, keep HyperDebug firmware in sync (we haven't done so already I think)
  2. In the PR body and commit message, articulate the rationale of making this change.

This reverts commit 7a0134a, such that
opentitantool no longer requires the `--force` switch in order to
downgrade firmware with `opentitantool transport update-firmware`.
Instead, opentitantool will be back to the original behavior of
overwriting firmware in any case when the version number does not match
exactly.

The original reason for not downgrading was that our branches had
diverged, such that they had different firmware versions, resulting in
CI machines constantly upgrading/downgrading as CI runs from different
branches were scheduled.

PR lowRISC#25917, however introduced a faulty HyperDebug firmware with never
version number, which is now on a number of CI machines, and will not be
downgraded automatically, with the result that "innocent" other PRs will
have CI failures.

I think it is clear that we should avoid such excessive flashing by
keeping the HyperDebug firmware version in sync between branches.  (We
do not need to keep all of opentitantool logic in sync, just the
built-in firmware version.)  For now, in order to avoid CI failures, we
should merge this PR, and accept some upgrading/downgrading.

Change-Id: I57739777ab3d86fa7e0375834cf9140e8e6ac984
Signed-off-by: Jes B. Klinke <[email protected]>
@jesultra
Copy link
Contributor Author

I have updated the commit and PR descriptions to mention that PR #25917 introduced flawed HyperDebug firmware with newer version number, which is a problem as once it has run on CI, those machines will retain the newer buggy version, causing CI failures for other unrelated PRs, unless this PR is merged.

@jesultra jesultra merged commit 8c5e761 into lowRISC:master Jan 18, 2025
3 of 9 checks passed
Copy link

Successfully created backport PR for earlgrey_es_sival:

Copy link

Backport failed for integrated_dev, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin integrated_dev
git worktree add -d .worktree/backport-25934-to-integrated_dev origin/integrated_dev
cd .worktree/backport-25934-to-integrated_dev
git switch --create backport-25934-to-integrated_dev
git cherry-pick -x 2667a3a2469bfbd78ddeab2a021bf79e700e83d1

Copy link

Successfully created backport PR for earlgrey_1.0.0:

@nbdd0121
Copy link
Contributor

integrated_dev isn't active so doesn't need backporting

@nbdd0121 nbdd0121 removed Manually CherryPick This PR should be manually cherry picked. CherryPick:integrated_dev This PR should be cherry-picked to integrated_dev labels Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CherryPick:earlgrey_es_sival This PR should be cherry-picked to earlgrey_es_sival CherryPick:earlgrey_1.0.0 This PR should be cherry-picked to earlgrey_1.0.0 SW:opentitantool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants