Skip to content

Conversation

@mathieuchopstm
Copy link
Contributor

Commit b4fb5d3 added a pop {r0, lr} to arch_pm_s2ram_resume but this instruction is not valid on ARMv6-M or ARMv8-M Baseline.

Modify the sequence to use pop {r0, pc} which is supported on all ARM M-profile implementations (v6/v7/v8 Baseline/v8 Mainline), and add comments to (hopefully) prevent this issue from re-appearing.

Copy link
Contributor

@JarmouniA JarmouniA left a comment

Choose a reason for hiding this comment

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

Good catch!

@JarmouniA JarmouniA added the bug The issue is a bug, or the PR is fixing a bug label Apr 7, 2025
Copy link
Contributor

@wearyzen wearyzen left a comment

Choose a reason for hiding this comment

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

Thank you for the change, the only thing I would suggest is updating the commit message to point to the original commit which added the pop here 24082d5. because commit b4fb5d3 just reverts to an earlier version and does not add anything of its own.

@mathieuchopstm
Copy link
Contributor Author

mathieuchopstm commented Apr 8, 2025

Thank you for the change, the only thing I would suggest is updating the commit message to point to the original commit which added the pop here 24082d5. because commit b4fb5d3 just reverts to an earlier version and does not add anything of its own.

I took the latest commit as the file's history is quite complicated. Originally (e.g., in 2590c48), there was a pop {lr} that I planned to get rid of in #76412, but 474d4c3 was merged while patch was pending so I dropped this change...

I will update the commit message based on your input though.

The original 'arch_pm_s2ram_resume' implementation saved lr on the stack
using 'push {lr}' and restored it using 'pop {lr}'. However, the Thumb-1
'pop' does not support lr as a target register, so this code would not
compile for ARMv6-M or ARMv8-M Baseline. r0 was added to these push/pop
later in 2590c48.

In 474d4c3, arch_pm_s2ram* functions were
modified to no longer use the stack, which incidentally "fixed" this issue.
b4fb5d3 reverted this commit and brought
back 'pop {r0, lr}' as-is, without taking compatibility into account.

Modify the sequence to use "pop {r0, pc}" which is supported on all
ARM M-profile implementations (v6/v7/v8 Baseline/v8 Mainline), and
add comments to (hopefully) prevent this issue from re-appearing.

Signed-off-by: Mathieu Choplain <[email protected]>
@mathieuchopstm mathieuchopstm force-pushed the arch_pm_s2ram_resume_compatfix branch from c6565d6 to 3d494c7 Compare April 8, 2025 09:27
@mathieuchopstm
Copy link
Contributor Author

@wearyzen updated commit message. Let me know what you think 🙂

@wearyzen
Copy link
Contributor

wearyzen commented Apr 8, 2025

@wearyzen updated commit message. Let me know what you think 🙂

LGTM, Thank you !

@kartben kartben merged commit 77378a8 into zephyrproject-rtos:main May 13, 2025
28 checks passed
@mathieuchopstm mathieuchopstm deleted the arch_pm_s2ram_resume_compatfix branch June 2, 2025 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ARM ARM (32-bit) Architecture bug The issue is a bug, or the PR is fixing a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants