tools: Add a new script for firmware boot stress testing #1180
Conversation
ranj063
force-pushed
the
fw_boot_stress_test
branch
from
242e7fc
to
a20ca97
Compare
There was a problem hiding this comment.
Please move everything but imports to a function and use the usual "main" function:
https://www.geeksforgeeks.org/python-main-function/
Great opportunity to fix these 100 indentation warnings from pylint!
https://github.com/thesofproject/sof-test/actions/runs/8819932717/job/24212345609?pr=1180
tools/sof-fw-boot-test.py
Outdated
| # get current fw_state and continue to boot only if the current state is 'PREPARE' | ||
| debugfs_entry = debugfs_path + '/fw_state' | ||
| cmd = 'cat ' + debugfs_entry | ||
| proc = subprocess.run(cmd, shell=True, capture_output=True, check=True, text=True) |
There was a problem hiding this comment.
Thank God you don't need subprocess to read or write to a file in Python, you can just use open(). There must be a gazillion of tutorials for this. Which means some AI bot can also show you.
This bit can probably be 3 times shorter. Right now it's shell script disguised in Python: the worst of both worlds :-(
There was a problem hiding this comment.
I've replaced subprocess with python open for reading but I kept the subprocess for writing the power_state and booting so that I can use the timeout
tools/sof-fw-boot-test.py
Outdated
| full_fw_path = '/lib/firmware/' + fw_path | ||
| if not os.path.exists(full_fw_path): | ||
| print("Invalid firmware path: %s" %fw_path) | ||
| sys.exit(1) |
There was a problem hiding this comment.
Python's default error messages are usually good enough, you don't C-style checks like these. You can save about 50 lines here.
tools/sof-fw-boot-test.py
Outdated
| debugfs_entry = debugfs_path + '/boot_fw' | ||
| cmd = 'echo 1 > ' + debugfs_entry | ||
| start = time.time() | ||
| proc = subprocess.run(cmd, shell=True, check=True, timeout=2, capture_output=True, text=True) |
There was a problem hiding this comment.
Interesting: this echo blocks until the firmware have successfully booted? Is this new? So far sof-test has been scanning the kernel logs...
There was a problem hiding this comment.
we didnt have the ability to boot the firmware from userspace before
ranj063
force-pushed
the
fw_boot_stress_test
branch
from
a20ca97
to
9036809
Compare
tools/sof-fw-boot-test.py
Outdated
| full_fw_path = '/lib/firmware/' + fw_path | ||
| if not os.path.exists(full_fw_path): | ||
| print(f"Firmware path {fw_path} does not exist") | ||
| sys.exit(1) |
There was a problem hiding this comment.
This is a test script, you generally don't need such checks. This is not C, Python does all that for you. A Python exception will provide more information because it has a stack trace, the script name etc.
|
Try to fix most |
ranj063
force-pushed
the
fw_boot_stress_test
branch
from
9036809
to
148a360
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
I'm afraid you're using tabs. Please avoid turn off tabs (at least) for Python. |
Quickest fix ever: Done! |
ranj063
force-pushed
the
fw_boot_stress_test
branch
from
148a360
to
9482827
Compare
ranj063
force-pushed
the
fw_boot_stress_test
branch
from
9482827
to
13578e2
Compare
@marc-hb I'm still not a Python expert, so cannot comment much on the implementation, as for the concept - I'm not sure why this testing mode is needed, do any other drivers for devices with loadable firmware implement such capabilities? Why isn't just module loading / unloading or driver binding / unbinding enough? |
tools/sof-fw-boot-test.py
Outdated
|
|
||
| # check if the DSP is in D3 | ||
| with open(debugfs_entry, "r", encoding="utf8") as dbgf: | ||
| power_state = dbgf.read(1024) |
There was a problem hiding this comment.
Style: basically everything the script does is just reading/writing whole buffers to debugfs nodes in a single directory. Consider making that the core abstraction rather than repeating the file I/O every time:
def dsp_set(node, state):
open(f"{debugfs_path}/{node}", "w").write(f"{state}\n")
def dsp_get(node):
return open(f"{debugfs_path}/{node}".read().rstrip()Noting:
- You don't need a "with" for a single-line expression that uses the opened file object and immediately discards it
- You don't need to specify a buffer size to read(), it will read to EOF by default
- Files are opened with "r" mode by default
- While technically the default encoding is locale-specific, it's always utf8 in practice, and at a command line 100% guaranteed to be an ascii superset anyway, which is all this script cares about.
There was a problem hiding this comment.
While technically the default encoding is locale-specific, it's always utf8 in practice, and at a command line 100% guaranteed to be an ascii superset anyway, which is all this script cares about.
Specifying the encoding silences a pylint warning.
We could probably silence that warning once at the top of this file.
tools/sof-fw-boot-test.py
Outdated
| "--fw_path", | ||
| type=str, | ||
| required=True, | ||
| help="path to the firmware file relative to /lib/firmware", |
There was a problem hiding this comment.
Why /lib/firmware? Would be simpler to just specify the file directly. Seems like a very common use case for a script like this would be stress testing firmware initialization, and so you'd want to be loading custom firmware and not whatever the system shipped.
There was a problem hiding this comment.
Agreed but don't forget about sof-ipc4-lib/. Not sure what will happen with these yet.
tools/sof-fw-boot-test.py
Outdated
| start = time.time() | ||
| with open(debugfs_entry, "w", encoding="utf8") as dbgf: | ||
| dbgf.write("1\n") | ||
| end = time.time() |
There was a problem hiding this comment.
Is the firmware startup synchronous with the debugfs write? My guess is that it's not, and this is just timestamping the moment when control gets back to this python script while the firmware continues its journey. Seems like the right way to do this would be to parse the kernel logs looking for SOF to report a successful load.
There was a problem hiding this comment.
@andyross the answer can very likely be found in:
There was a problem hiding this comment.
@andyross OK let me modify the script to parse the journalctl to get the accurate boot time
There was a problem hiding this comment.
@andyross OK let me modify the script to parse the journalctl to get the accurate boot time
@ranj063 please do this in a different script that invokes this one. A shell script would probably be best for this. Try git -C sof-test grep -C 5 journalctl for plenty of examples and recipes.
Divide and conquer! Right now this script is short, simple and focus on one thing. journalctl would be pretty big ddependency to add.
Better try to have each script "do one thing and do it well" and let users compose them as they see fit
https://www.catb.org/~esr/writings/taoup/html/ch01s06.html
BTW there are many people who still prefer /var/log/messages and friends (hi, @ujfalusi ! #1169)
There was a problem hiding this comment.
test-case/verify-sof-firmware-load.sh is a good starting point.
There was a problem hiding this comment.
@marc-hb i used systemd jounral for this. Let me know how it looks
ranj063
force-pushed
the
fw_boot_stress_test
branch
2 times, most recently
from
b4ad2f7
to
a0a43d4
Compare
This requires the kernel changes that expose the debugfs entries for executing the different DSP ops and reading the current DSP firmware and power states. The script takes the firmware name and the path as inputs along with the number of iterations of firmware boot. It takes care of putting the DSP in the D3 state and unloading any existing firmware before starting firmware boot. Signed-off-by: Ranjani Sridharan <[email protected]>
ranj063
force-pushed
the
fw_boot_stress_test
branch
from
a0a43d4
to
81309d8
Compare
There was a problem hiding this comment.
Wow, there is a python module for journalctl! Great to know, thanks! People who don't like journalctl can make this optional later; good enough for a first shot.
LGTM except for some of @andyross 's suggestions resurrecting some old pylint warnings.
Please add the with clauses back. Now that you have created the dsp_set() and dsp_get() functions (nice!) you need only two with clauses. Yes they're overkill but they please pylint, they show good practice and only 2 of them shouldn't hurt @andyross too much :-)
Same thing with encoding=utf-8.
pylint isn't always right but when it's easy to please then please just go and do that.
BTW you could have simply dropped the timestamping entirely in this PR and moved it to a later PR. This could have got this one merged faster. It is counter-intuitive and there is a fine line but splitting into smaller PRs can often be faster: I'm NOT asking to split this PR, just something to keep in mind. |
| message = message.decode('utf-8', errors='replace') | ||
| entries.append((timestamp, message)) | ||
|
|
||
| return entries |
There was a problem hiding this comment.
In the future it would be good to make this an iterator with yield instead of loading everything in memory.
|
This is not used anywhere yet, so no need to hold this any longer. |
This requires the kernel changes that expose the debugfs entries for executing the different DSP ops and reading the current DSP firmware and power states.
The script takes the firmware name and the path as inputs along with the number of iterations of firmware boot. It takes care of putting the DSP in the D3 state and unloading any existing firmware before starting firmware boot.
This needs the kernel PR for testing: thesofproject/linux#4874