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

nitropad-nx: use standard shutdown/reboot commands #1704

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

macpijan
Copy link
Collaborator

This commit effectively reverts commits a1c13ff and 902866c. There is no need for this special EC-based poweroff command. See more details in issue linked below.

Fixes: Dasharo/dasharo-issues#711

Not actually tested on master yet, only on this branch and forward picked: Dasharo#11

Not sure why it was implemented (guessing EC update?). Maybe we will find out in this review.

This commit effectively reverts commits a1c13ff and 902866c. There is
no need for this special EC-based poweroff command. See more details in
issue linked below.

Fixes: Dasharo/dasharo-issues#711

Signed-off-by: Maciej Pijanowski <[email protected]>
@tlaurion
Copy link
Collaborator

tlaurion commented Jun 20, 2024

This commit effectively reverts commits a1c13ff and 902866c. There is no need for this special EC-based poweroff command. See more details in issue linked below.

Fixes: Dasharo/dasharo-issues#711

@macpijan : Will let it build and happily test CircleCI zip update file on my nv41; but should work out of the box.

Not actually tested on master yet, only on this branch and forward picked: Dasharo#11

Not sure why it was implemented (guessing EC update?). Maybe we will find out in this review.

I guess it's an artifact of using proprietary EC, but I do not know the whole story nor was owning a nv41 back then.
@daringer please fill us in?

@daringer
Copy link
Collaborator

Yes, that's a "historic artifact" .... I'd suggest then also removing iotools as this is only needed here.
Although if somebody wants to overwrite the entire flash (not just the bios region) - rebooting will fail afterwards, right @macpijan ? but under the line this shouldn't happen anyways, so removing both (iotools + shutdown-instead-of-reboot) should be fine

@macpijan
Copy link
Collaborator Author

Although if somebody wants to overwrite the entire flash (not just the bios region) - rebooting will fail afterwards, right @macpijan ?

I am not aware of such interaction. @mkopec can you please confirm this?

@mkopec
Copy link
Contributor

mkopec commented Jun 21, 2024

Flashing the ME when it's not disabled may result in shutdown / reboot issues indeed, but it shouldn't matter as long as ME is disabled (by HMRFPO or any other method)

@tlaurion
Copy link
Collaborator

tlaurion commented Jun 21, 2024

Yes, that's a "historic artifact" .... I'd suggest then also removing iotools as this is only needed here. Although if somebody wants to overwrite the entire flash (not just the bios region) - rebooting will fail afterwards, right @macpijan ? but under the line this shouldn't happen anyways, so removing both (iotools + shutdown-instead-of-reboot) should be fine

Flashing the ME when it's not disabled may result in shutdown / reboot issues indeed, but it shouldn't matter as long as ME is disabled (by HMRFPO or any other method)

@daringer so this was not related to EC but ME. Retesting.

@tlaurion
Copy link
Collaborator

Reboot works. Inspecting cbmem

@tlaurion
Copy link
Collaborator

tlaurion commented Jun 21, 2024

cbmem --console ¦ grep '^ME' gives no output at all.

@tlaurion
Copy link
Collaborator

tlaurion commented Jun 21, 2024

This circleci rom artifacts contains hotp-verification v1.5 not 1.6, inspecting (commit id +2005 in filename)

Also bootsplash is bigger (1024*768 un resized) : seems like head of this PR is not master

@tlaurion
Copy link
Collaborator

tlaurion commented Jun 21, 2024

@macpijan Please rebase on master.

Retesting this as https://github.com/tlaurion/heads/tree/Dasharo_nitropad-shutdown which is this commit on master.

@tlaurion
Copy link
Collaborator

@macpijan Please rebase on master.

Retesting this as https://github.com/tlaurion/heads/tree/Dasharo_nitropad-shutdown which is this commit on master.

@macpijan nevermind. Merging this.

Copy link
Collaborator

@tlaurion tlaurion left a comment

Choose a reason for hiding this comment

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

@tlaurion tlaurion merged commit 96b619b into linuxboot:master Jun 21, 2024
51 checks passed
@tlaurion
Copy link
Collaborator

tlaurion commented Jun 21, 2024

For prosperity @daringer @mkopec @macpijan: ME was shipped activated in the past see https://github.com/Nitrokey/heads/releases/tag/v2.4.1

This explains iotools having been required.

Will do PR removing iotools

@macpijan macpijan deleted the nitropad-shutdown branch June 21, 2024 14:28
macpijan added a commit to Dasharo/heads that referenced this pull request Jun 21, 2024
As discussed in: linuxboot#1704
there is no need to include iotools module for nitropads.
Since there is no board using it, and we see no reason to use
it in the future (the EC udpate will not require it, as update
will be server by coreboot in the future), drop the module as well.

Signed-off-by: Maciej Pijanowski <[email protected]>
@tlaurion
Copy link
Collaborator

tlaurion commented Jun 21, 2024

@macpijan other boards include it under master. Can you generalize and push commit sedding all the boards please? Otherwise breaks other builds upstream.

macpijan added a commit to Dasharo/heads that referenced this pull request Jun 21, 2024
As discussed in: linuxboot#1704
there is no need to include iotools module for nitropads.

Signed-off-by: Maciej Pijanowski <[email protected]>
macpijan added a commit to Dasharo/heads that referenced this pull request Jun 21, 2024
As discussed in: linuxboot#1704
there is no need to include iotools module for nitropads.

Signed-off-by: Maciej Pijanowski <[email protected]>
macpijan added a commit to Dasharo/heads that referenced this pull request Jun 21, 2024
As discussed in: linuxboot#1704
there is no need to include iotools module for nitropads.

Since there is no board using it, and we see no reason to use
it in the future (the EC udpate will not require it, as update
will be server by coreboot in the future), drop the module as well.

Signed-off-by: Maciej Pijanowski <[email protected]>
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.

Heads shuts down instead of rebooting
4 participants