IPC updates to improve logging and support ABI changes #585
Conversation
src/ipc/handler.c
Outdated
There was a problem hiding this comment.
And all MAJOR versions of the IPC ABI (up to a certain age to be determined by market information)
src/ipc/handler.c
Outdated
There was a problem hiding this comment.
Not sure I see what the size check has to do with the ABI?
It's still useful not to trust the kernel, isn't it? Even for development it helps identify mismatches.
There was a problem hiding this comment.
I think this has to work both ways since the driver also has to trust the FW too. We have 2 situations (for MINOR, PATCH updates) we need to deal with :
- Sender sends larger IPC than receiver expects - Easy to deal with as receiver only uses what it knows and by default ignores any new data.
- Sender sends smaller IPC than receiver expects - More complex, but receiver should pad any missing data as 0 i.e. all MINOR/PATCH ABI additions require non zero data to activate.
There was a problem hiding this comment.
Agree, but we are doing neither. The patch just removes all checks and does not add padding or overrides.
There was a problem hiding this comment.
Fixing that now but will be new PR. Will need something similar in the kernel.
There was a problem hiding this comment.
ah ok then, didn't realize there was a planned follow-up
Improve trace for IPC. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
On receiving an IPC IRQ the handler currently does not check the IRQ mask. This means notification received ACKs (i.e. for trace updates) from the host may be reported as duplicate host command IPCs. Fix this by checking IPC IRQ mask. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
|
SOFCI TEST |
lgirdwood
force-pushed
the
topic/ipc-updates
branch
from
06ca0ca to
2183aed
Compare
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
LGTM, only a cosmetic comment on %d vs. %u
| "pcm_dev ID = %u not found.", | ||
| pcm_params->comp_id); | ||
| trace_ipc_error("ipc: comp %d not found", pcm_params->comp_id); | ||
| return -ENODEV; |
There was a problem hiding this comment.
nit-pick: you've replaced %u by %d in the format, is this intentional?
There was a problem hiding this comment.
Will fix incrementally.
| goto out; | ||
|
|
||
| trace_ipc("ipc: not rx -> 0x%x", msg->header); | ||
|
|
There was a problem hiding this comment.
for my education, have we stopped using the 3 letter code for errors? If yes, we might want to revisit all the drivers to provide more meaningful information that Sst or ssr for SSP start / resume
There was a problem hiding this comment.
yes, that's something we have todo - I'm hoping this can be the example to copy.
Some IPC logging updates that go alongside ABI compatibility support. Also includes fixing a false positive IPC trace_error.