audio: eq_iir: register IPC-time blob validator#10942
Conversation
| } | ||
| if (lookup) { | ||
| for (; i < SOF_EQ_IIR_MAX_RESPONSES; i++) | ||
| lookup[i] = NULL; |
There was a problem hiding this comment.
Yep, and the memset can be before the loop.
| return -EINVAL; | ||
| } | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
you never propagate this function's returned error code and always use it as a condition in if clauses. How about making it boolean
static bool eq_iir_blob_size_valid(struct comp_dev *dev, size_t size)
{
return size >= sizeof(struct sof_eq_iir_config) && size <= SOF_EQ_IIR_MAX_SIZE;
}
There was a problem hiding this comment.
OK, I'll change to this.
Hook an eq_iir blob validator into the model handler so a corrupted run-time configuration update is rejected before it can replace the working blob. Playback or capture then continues with the previously set coefficients instead of being interrupted by a bad IPC. The validator parses the blob layout end to end: the size envelope, channel and response counts, each response header and biquad section bounds, and the assign_response[] entries reachable by the per-channel loop. The layout walk is factored into a shared helper that eq_iir_init_coef() also reuses at setup time, so IPC-time and prepare-time agree on structural validity; the assign_response[] check is exclusive to the IPC path, where the full table must be validated without knowing the stream channel count. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
singalsu
force-pushed
the
model_handler_validator_eqiir
branch
from
db4e659 to
442b1d4
Compare
Use memset() to zero the unused tail of the response lookup array in eq_iir_walk_config() instead of a per-slot loop. The walk pre-clears the whole table once when a lookup is requested and then fills the in-use entries. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
There was a problem hiding this comment.
Pull request overview
This PR adds an IPC-time configuration blob validator for the EQ IIR component so malformed coefficient updates are rejected before they can replace a working runtime configuration. It aligns EQ IIR behavior with existing components (e.g., EQ FIR) that install a blob validator during prepare() and clear it on reset().
Changes:
- Introduces
eq_iir_validate_config()and exposes it viaeq_iir.h. - Refactors EQ IIR blob parsing to share a common structural validation walk (
eq_iir_walk_config()). - Registers the validator in
eq_iir_prepare()(and unregisters ineq_iir_reset()), removing the redundant run-time blob size check in the audio processing path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/audio/eq_iir/eq_iir.h | Declares the new eq_iir_validate_config() entry point used for IPC-time validation. |
| src/audio/eq_iir/eq_iir.c | Uses the validator in prepare(), registers/unregisters it, and simplifies the run-time update path. |
| src/audio/eq_iir/eq_iir_generic.c | Implements structural blob validation and assign-response validation, reusing shared parsing logic. |
| j = 0; | ||
| coef_data = ASSUME_ALIGNED(&config->data[config->channels_in_config], 4); | ||
| if (lookup) | ||
| memset(lookup, 0, SOF_EQ_IIR_MAX_RESPONSES * sizeof(*lookup)); |
There was a problem hiding this comment.
ah, now you "waste cycles" by first setting it all to 0 and then overwriting a part of it... I thought you'd do something like memset(lookup + i, 0, (SOF_EQ_IIR_MAX_RESPONSES - i) * sizeof(lookup[0])) after the loop. And you can do it directly in the previous commit
There was a problem hiding this comment.
Waste code size vs. waste cycles ;) OK, I can do this way.
There was a problem hiding this comment.
Also, it's a very small array...
There was a problem hiding this comment.
Size for memset() is unsigned, so less than zero size would be huge size, though it should never happen, but handling for zero is needed. The man page for memset isn't mentioning what happens if called with zero size.
There was a problem hiding this comment.
OK, seems zero size is legal, but I still wonder if the risk for negative size -> huge positive size risk could happen.
No description provided.