Skip to content

ams and clk fixes #10881

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.

Sign up for GitHub

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
lgirdwood wants to merge 3 commits into
base: main
Choose a base branch
from
+12 −5
Open

ams and clk fixes #10881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7042e49
ams: copy the payload struct size into the message slot
lrgirdwo Jun 11, 2026
9d815b4
clk: clamp frequency index when no threshold matches
lrgirdwo Jun 11, 2026
4d39bb0
ams: remove unused message-size macros
lrgirdwo Jun 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/include/sof/lib/ams.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,6 @@
/* Space allocated for async message content*/
#define AMS_MAX_MSG_SIZE 0x1000

/* Size of slots message, module id and instance id */
#define AMS_SLOT_SIZE(msg) (AMS_MESSAGE_SIZE(msg) + sizeof(uint16_t) * 2)
#define AMS_MESSAGE_SIZE(msg) (sizeof(*msg) - sizeof(char) + (sizeof(char) * (msg->message_length)))

/**
* \brief IXC message payload
*
Expand Down
6 changes: 5 additions & 1 deletion src/lib/ams.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,9 +285,13 @@ static uint32_t ams_push_slot(struct ams_shared_context __sparse_cache *ctx_shar

for (uint32_t i = 0; i < ARRAY_SIZE(ctx_shared->slots); ++i) {
if (ctx_shared->slot_uses[i] == 0) {
/* the slot only carries the payload struct (read back
* via u.msg); message points to a caller-owned buffer
* rather than inline data, so copy exactly the struct
*/
err = memcpy_s((__sparse_force void *)ctx_shared->slots[i].u.msg_raw,
sizeof(ctx_shared->slots[i].u.msg_raw),
msg, AMS_MESSAGE_SIZE(msg));
msg, sizeof(*msg));

@lyakh lyakh Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't know how AMS works, but this looks suspicious - the type of msg is also called "...payload" which seems to suggest that the data is also attached to the header, which would then be dropped by this change?

@lgirdwood lgirdwood Jun 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The message field is a pointer (uint8_t *message), not inline data — ams_helper_prepare_payload() does payload->message = message, pointing at a caller-owned buffer. So the payload bytes are referenced by the pointer, not appended after the struct.

AMS_MESSAGE_SIZE() = sizeof(*msg) - 1 + message_length therefore reads message_length - 1 bytes past the end of the struct (adjacent memory), not the actual payload. The slot only needs to carry the struct itself (which already includes the message pointer and message_length); ams_process_slot() copies the slot back into a struct ams_message_payload and uses those fields. So copying sizeof(*msg) preserves exactly what the consumer reads back and just removes the over-read — no payload is dropped.

(Whether that message pointer stays valid by the time another core consumes the slot is a separate, pre-existing concern this patch doesn't change.)

@lyakh lyakh Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@lgirdwood yes, I saw that comment, and I don't quite understand or follow it. "The message field is a pointer (uint8_t *message), not inline data" - of course it is a pointer - a local variable, containing a pointer. A pointer to a buffer that begins with a header which is followed by data. What's the difference?

@lgirdwood lgirdwood Jun 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I traced the full path to be sure, and the by-pointer reading holds:

  • Producer (detect_test.c: ams_notify_kpb): ams_helper_prepare_payload() sets message = (uint8_t *)&cd->client_data (persistent component data) and message_length = sizeof(struct kpb_client).
  • Consumer (kpb.c: kpb_ams_kpd_notification): struct kpb_client *cli_data = (struct kpb_client *)payload->message; — it casts the message pointer and dereferences it; nothing reads inline bytes.
  • Same-core delivery passes the producer's payload straight to the callback (no slot copy at all).
  • Cross-core delivery: send_message_over_ixc() sends an IDC message carrying only the slot index (size = 0, payload = NULL); the target core reads the slot struct from the shared coherent context and dereferences message (valid across SMP cores).

So the payload is never stored inline — the slot only needs the struct (which holds the message pointer + length). AMS_MESSAGE_SIZE() = sizeof(*msg) - 1 + message_length was reading message_length - 1 bytes past the producer struct and copying bytes the consumer never looks at; sizeof(*msg) copies exactly what's read back. So the fix is correct and no payload is dropped.

It was also the only user of AMS_MESSAGE_SIZE(), and AMS_SLOT_SIZE() had none, so I've removed both as dead/misleading in a follow-up commit.

(Orthogonal, pre-existing: the by-pointer design requires the producer's buffer to outlive async cross-core delivery — current clients point at persistent state, so that's fine; just noting the contract.)


if (err != 0)
return AMS_INVALID_SLOT;
Expand Down
7 changes: 7 additions & 0 deletions src/lib/cpu-clk-manager.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,13 @@ static int request_freq_change(unsigned int core, int freq)
break;
}

/* if no entry was larger than the requested frequency the loop ends with
* selected_freq_id == clk->freqs_num; clamp to the last valid entry
* before indexing
*/
if (selected_freq_id == clk->freqs_num)
selected_freq_id = clk->freqs_num - 1;

/* don't change clock frequency if already using proper clock */
current_freq = clock_get_freq(core);
if (clk->freqs[selected_freq_id].freq != current_freq)
Expand Down
Loading