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

lib: os: reboot: a few cleanups #60920

Closed
wants to merge 39 commits into from

Conversation

gmarull
Copy link
Member

@gmarull gmarull commented Jul 28, 2023

After 62dbe72 and discussions in #60626 I realized the reboot functionality required a bit of love. So I tried to clean up things a little bit. I may have made a few mistakes, so please, review. I tried to make changes in small steps to ease the review process.

Fixes #60915

@gmarull gmarull force-pushed the reboot-cleanup branch 3 times, most recently from a61b8f3 to 4e3b7b9 Compare July 28, 2023 14:40
@nashif
Copy link
Member

nashif commented Jul 28, 2023

The cleanup lgtm, however, I would put the fix for #60915 into its own PR

gmarull added 2 commits August 7, 2023 15:08
62dbe72 introduced yet another reboot API for no reason. This patch
converts the implementation into a sys_reboot() hook. Note that shell
commands have been deleted, as there's already a shell command that uses
the standard reboot.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Add details on changes made in the reboot.h API.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
@gmarull
Copy link
Member Author

gmarull commented Aug 7, 2023

rebased to fix conflicts

Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

OK for retention/mcuboot

Copy link
Collaborator

@de-nordic de-nordic left a comment

Choose a reason for hiding this comment

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

MCUmgr/MCUboot look ok.

@gmarull
Copy link
Member Author

gmarull commented Aug 11, 2023

so what is exactly blocking this PR now?

@de-nordic
Copy link
Collaborator

so what is exactly blocking this PR now?

Four days waiting and already inpatient.

@gmarull
Copy link
Member Author

gmarull commented Aug 11, 2023

so what is exactly blocking this PR now?

Four days waiting and already inpatient.

not impatient, just unsure on why it's being blocked.

@fabiobaltieri
Copy link
Member

@gmarull I think I recall @nashif mentioning that this should go through the treewide process https://docs.zephyrproject.org/latest/contribute/guidelines.html#treewide-changes

@gmarull
Copy link
Member Author

gmarull commented Aug 21, 2023

Closing this one, if someone else has an interest and patience in this, feel free to take it.

@gmarull gmarull closed this Aug 21, 2023
@martinjaeger
Copy link
Member

It's a shame this very useful cleanup didn't make it in.

@nashif It looks like your previous concern has been addressed. Is there still something else blocking?

@nashif
Copy link
Member

nashif commented Aug 21, 2023

@nashif It looks like your previous concern has been addressed. Is there still something else blocking?

this is a treewide change, which changes behaviour and touches many platforms and other areas in the tree that needs to go the through the process defined here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Remove pm_system_reset and use sys_reboot instead for PSCI