ams and clk fixes

[lgirdwood]

Small fixes for AMS and clk manager.

[Copilot]

Pull request overview

This PR applies two small safety/correctness fixes in core infrastructure code: CPU clock frequency selection and AMS slot payload copying.

Changes:

• Prevent out-of-bounds indexing in request_freq_change() when the requested frequency exceeds the highest entry in the clock frequency table.
• Fix ams_push_slot() to copy only the struct ams_message_payload into a slot, avoiding an out-of-bounds read caused by using AMS_MESSAGE_SIZE() with a payload type that contains a pointer (not inline data).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/lib/cpu-clk-manager.c	Clamps selected_freq_id to the last valid frequency-table entry before indexing, preventing OOB access.
src/lib/ams.c	Adjusts slot copy size to sizeof(*msg) to avoid over-reading past struct ams_message_payload.

[kv2019i]

Code changes good, commit messages have invalid line wrapping.

[kv2019i]

Commit message needs fixing.

[lgirdwood]

Reflowed the commit message body to wrap at <=72 columns (kernel/git convention, under the old checkpatch 75-char limit) and force-pushed.

[lyakh]

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]

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]

@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]

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.)

[lyakh]

I'd make the condition == for clarity

[lgirdwood]

Done, changed to ==. After the loop selected_freq_id can only be < freqs_num (loop break) or exactly == freqs_num (ran off the end), so == is precise and reads clearer.