audio: fix overriding const attribute
#10118
Conversation
|
should resolve #10117 |
lyakh
force-pushed
the
ops
branch
2 times, most recently
from
949d6d4 to
8d642b3
Compare
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
One open on driver const. Others patches all good.
src/include/sof/audio/component.h
Outdated
| /** \brief Holds constant pointer to component driver */ | ||
| struct comp_driver_info { | ||
| const struct comp_driver *drv; /**< pointer to component driver */ | ||
| struct comp_driver *dyn_drv; /**< pointer to dynamically created component driver - |
There was a problem hiding this comment.
What fields are being modified in driver ?
There was a problem hiding this comment.
@lgirdwood the module adapter operations pointer - it is set when a module is loaded fa2fe34#diff-957336c9ebdbe16dab94f6e49620e9b112d3b373d3de4d1035cdf289f745a00dL521-R532 . But thinking about it - maybe it's better to only store a pointer to that pointer here, not to the complete struct comp_driver object. Let me change that.
Zephyr inserts function names into logs automatically, no need to duplicate them in log format strings. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
comp_drivers_get()->lock is guarding the global list of component drivers, no need for the chain DMA driver to take it. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
|
having observed PTL failures https://sof-ci.01.org/sofpr/PR10118/build13953/devicetest/index.html with the original version, trying to drop supposedly the only commit, that can cause such regressions - "ipc4: remove needless interrupt disabling" |
Using a type-cast to override a "const" attribute and modify read- only data is unsafe. Add a writable pointer to struct comp_driver_info instead and use it when creating component drivers dynamically. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
|
This obviously conflicts with #10098 but should be easy to resolve either way, whichever PR gets merged first. |
lyakh
requested review from
dbaluta,
kv2019i,
lbetlej,
mmaka1 and
plbossart
as code owners
UPDATE: that isn't a regression, the issue is known #10116 |
| key = k_spin_lock(&drivers->lock); | ||
| switch (cd->chain_task.state) { | ||
| case SOF_TASK_STATE_QUEUED: | ||
| k_spin_unlock(&drivers->lock, key); |
There was a problem hiding this comment.
Even if the used lock is wrong, it does not automatically mean that no locking is needed. Even the wrong lock protects against interleaved function calls. I have no idea if those can happen, so this is just a worried comment.
There was a problem hiding this comment.
@lyakh I suppose you could followup with a comment declaring where the lock is held for the state check.
3 participants
Don't override
constattributes to modify read-only data.