logger - invalid filename length

[lgirdwood]

Logger should be more robust when parsing data to cope with corruption and unexpected values.

root@up2:~# sof-logger -t -l sof-apl.ldc 
 CORE  LEVEL  COMP_ID        TIMESTAMP            DELTA                FILE_NAME	CONTENT
Error: Invalid filename length. 

hexdump -C /sys/kernel/debug/sof/trace

00000000  00 00 00 00 eb bc 18 00  00 00 00 00 74 67 00 20  |............tg. |
00000010  00 00 00 00 bb c3 18 00  00 00 00 00 80 66 00 20  |.............f. |
00000020  00 00 00 00 97 c5 18 00  00 00 00 00 4c 67 00 20  |............Lg. |
00000030  00 00 00 00 64 d8 19 00  00 00 00 00 08 15 00 20  |....d.......... |
00000040  00 00 00 00 aa d8 19 00  00 00 00 00 d8 12 00 20  |............... |
00000050  00 00 00 00 fd d8 19 00  00 00 00 00 bc 45 00 20  |.............E. |
00000060  00 00 00 00 4b da 19 00  00 00 00 00 dc 15 00 20  |....K.......... |
00000070  08 dc 02 be 02 00 00 00  00 00 00 00 dc e3 19 00  |................|
00000080  00 00 00 00 08 15 00 20  00 00 00 00 22 e4 19 00  |....... ...."...|
00000090  00 00 00 00 d8 12 00 20  00 00 00 00 6b e4 19 00  |....... ....k...|
000000a0  00 00 00 00 8c 37 00 20  00 00 00 00 b0 eb 19 00  |.....7. ........|

[akloniex]

@lgirdwood
I'm quite certain that it's a human error, meaning that You've tried parsing with incompatible .ldc file, relative to firmware running on the machine, not an actual tool malfunction.
I agree that it should be more robust and provide more sensible information, to help user take correct actions in order to overcome the issue he's facing.

I'm changing this to an enhancement as I don't think that tool malfunctioned. We'll cooperate with driver team to dump fw_ready message to debugfs so that sof-logger could read it, along with info that we'll put in .ldc file during FW build, for comparison and verification.

[xiulipan]

@akloniex
I have go through the logger code and here is my understanding:

The string is actually stored in *.ldc files, and dma_trace only send the entry pointer to the structure store the char * string.
Which means the *.ldc and the *.ri you are running must be build from the same EFI file.
Any mismatch will make the entry pointer have bias and cause errors.

dump fw_ready message to debugfs

This is actually done when the ready message is send from the FW, but this info is not kept as the FW running . We can have a another debugfs to store the ready info when we get it and let sof-logger to read it. This will have a early notification if there is a mismatch.

BUT NOTE:

sof-audio sof-audio: Firmware info: version 1.1-b9717 build 32 on Oct 18 2018:13:54:44

The version will not be change if we have some dirty build (only diff, no commit)
So we must check version and build or even build time.

[xiulipan]

@lgirdwood

sof-audio sof-audio: Firmware info: version 1.1-b9717 build 32 on Oct 18 2018:13:54:44

BTW, we may need some tag on master branch. The latest tag on master branch is still v1.1

$git describe --abbrev=4
v1.1-840-gb971

[lgirdwood]

The error being printed by logger also needs improved if it really more about using the wrong ldc file than file name length.

[akloniex]

@xiulipan That is correct, .ldc and .ri have to come from the same build. I agree, that checking by commit SHA wouldn't be enough, and we would like to compare whole sof_ipc_fw_ready.version struct byte by byte, so if kernel team could dump it to separate debugfs, for sof-logger to check up on, that would be best. We will extract version with rimage and put at top of .ldc file from FW side and implement checking mechanism in sof-logger.

[xiulipan]

@akloniex
So let make some alignment here.

1. rimge need patch for version structure into .ldc file
2. logger need to check the version in .ldc and debugfs from firmware ready info:

• May also need some check about if the debugfs is there.
• can override by some flag? default value inside the code.

3. Kernel part: what file name would you like to store the file.

And we may have some per merge test about this patch sets

[akloniex]

@xiulipan
0. FW needs patch allowing separation of fw_ready.version from .text section and linker script to fit pieces together, linking version to loadable section for runtime use and non-loadable, for extraction by rimage into .ldc file

1. rimage needs patch to fit new section into both .ri and .ldc
2. logger will check default location (tbd), overrided by parameter to allow custom path, or completely disabled verification with different parameter
3. for the kernel part, I believe that /sys/kernel/debug/sof/fw_version is fair enough name

All of these should not break current behaviour, so these can be implemented independently. We could stage logger part in 2 steps:

1. verification disabled by default, enabled to default path with parameter
2. verification enabled with default path as default behaviour, disabled with parameter.
   This way user experience would not be impacted throughout the process, and all steps could be carried out in parallel.

[xiulipan]

@akloniex
I think this should be enough.
Would you like to track this tasks with github issues? We can label them with enhancement.

[akloniex]

@xiulipan
I believe that we could keep track of it in this thread, but if you wish you could create issue for kernel repository and link it here.