Remove redundant check for return value #10006
Conversation
ranj063
requested review from
a team,
bardliao,
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
marcinszkudlinski,
mmaka1,
pblaszko,
plbossart,
singalsu,
tlissows and
tmleman
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR removes redundant buffer validation checks across multiple audio processing components by consolidating and repositioning validity checks in prepare functions and removing duplicate assignments.
- Consolidated buffer validity checks in prepare routines
- Removed duplicate reassignments for source and sink buffers
- Improved consistency in error handling for missing buffers
Reviewed Changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/audio/rtnr/rtnr.c | Removed redundant buffer reassignments and added a buffer validity check |
| src/audio/multiband_drc/multiband_drc_ipc4.c | Added sink buffer validation before parameter setup |
| src/audio/multiband_drc/multiband_drc.c | Introduced source buffer validation |
| src/audio/module_adapter/module/waves/waves.c | Added buffer existence check before processing |
| src/audio/mixer/mixer.c | Added sink buffer validation; note slight inconsistency in error message phrasing |
| src/audio/mfcc/mfcc.c | Added combined source/sink buffer check |
| src/audio/igo_nr/igo_nr.c | Added combined source/sink buffer check before processing |
| src/audio/google/google_ctc_audio_processing.c | Added source buffer validation in prepare routine |
| src/audio/eq_iir/eq_iir_ipc4.c | Clarified via comment that buffer connectivity is assumed valid |
| src/audio/eq_iir/eq_iir_ipc3.c | Clarified via comment that buffer connectivity is assumed valid |
| src/audio/eq_iir/eq_iir.c | Consolidated buffer checks and removed redundant reassignments |
| src/audio/eq_fir/eq_fir_ipc4.c | Added comment to note buffer validation performed by caller |
| src/audio/eq_fir/eq_fir.c | Consolidated buffer checks and removed redundant reassignments |
| src/audio/drc/drc.c | Moved buffer validation prior to parameter setup; eliminated duplicate checks |
| src/audio/dcblock/dcblock_ipc4.c | Added comment clarifying buffer connectivity assumption |
| src/audio/dcblock/dcblock.c | Added buffer validation check before calling parameters |
| src/audio/crossover/crossover_ipc4.c | Updated comment to indicate that source buffer validity has been verified |
| src/audio/crossover/crossover.c | Added source buffer check and removed a redundant reassignment |
| src/audio/asrc/asrc.c | Added source/sink buffer check and clarified redundant check comment |
| src/audio/aria/aria.c | Inserted a buffer validation check before setting stream parameters |
Comments suppressed due to low confidence (1)
src/audio/mixer/mixer.c:218
- [nitpick] Consider using a consistent error message (e.g. "no sink buffer") to match the phrasing used in other modules.
comp_err(dev, "no sink");
7b62669 to
ed43ad0
Compare
pm_state_force() sets the forced state and always returns true. So remove the redundant check for failure. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
| tr_err(&zephyr_tr, "failed to set PM_STATE_SOFT_OFF on core %d", id); | ||
| return; | ||
| } | ||
| pm_state_force(id, &(struct pm_state_info){PM_STATE_SOFT_OFF, 0, 0}); |
There was a problem hiding this comment.
yeah, but the function returns a "success status" supposedly. Should we add an assert in case the implementation changes? And in production builds it will be removed anyway
There was a problem hiding this comment.
I think its wise keeping the check - its safe for future API changes, and its non time critical code.
There was a problem hiding this comment.
ok ill close this PR. Anyway, I want to remove the call for pm_state_force() during disable and use sys_poweroff() instead.
tmleman
left a comment
tmleman
left a comment
There was a problem hiding this comment.
I made a PR to Zephyr that checks if a given state was successfully set; let's see what the PM maintainers from Zephyr say about it (zephyrproject-rtos/zephyr#89876).
| tr_err(&zephyr_tr, "failed to set PM_STATE_SOFT_OFF on core %d", id); | ||
| return; | ||
| } | ||
| pm_state_force(id, &(struct pm_state_info){PM_STATE_SOFT_OFF, 0, 0}); |
There was a problem hiding this comment.
If I remember correctly, at the time it was written, it was possible to receive false. But currently, that possibility no longer exists; the function returns true even if the specified state cannot be set.
4 participants
No description provided.