Added UFS functional tests

[jeevanandan-sandan]

No description provided.

[vnarapar]

Please add commit messages

[vnarapar]

Ensure the script has execute permissions

[vnarapar]

Squash to single commit

[vnarapar]

Are these tests validated on qcom-next?

[smuppand]

These comments apply to the other tests as well. Please also take care of the https://github.com/qualcomm-linux/qcom-linux-testkit/actions/runs/19762511972/job/56772745461.

[smuppand]

I suggest enabling these tests in qcom-next instead of meta-qcom.

[jeevanandan-sandan]

Enabled the tests on qcom-next by adding the .yaml files for each test case.

[smuppand]

you never set __INIT_ENV_LOADED, so this guard is effectively useless if this script is ever sourced again.

[jeevanandan-sandan]

exporting the __INIT_ENV_LOADED variable after setting it to 1 once the source is loaded.

[smuppand]

cut, head, tail, udevadm are not used anywhere in this script. You do use findmnt and awk, and rely on readlink (though that's almost always present)

[jeevanandan-sandan]

Removed unused dependencies and checking only the required dependencies.

[smuppand]

"$MANDATORY_CONFIGS" is one arugment with a space inside, not two config names. That'll fail.

[smuppand]

Include log_skip so it is visible to the user.

[jeevanandan-sandan]

added log_skip when UFS dt_nodes are missing.

[smuppand]

Called twice on the PASS path

[jeevanandan-sandan]

Fixed the PASS criteria and updating to .res file only once all the test steps are successful.

[smuppand]

NIT

[smuppand]

.res file is written PASS twice

[jeevanandan-sandan]

Fixed the PASS criteria and updating to .res file only once all the test steps are successful.

[smuppand]

NIT

[smuppand]

Please use already available functions from functestlib.sh

[smuppand]

In Lava: keep in mind that if one of them fails and exits with non-zero status, the whole job terminates. So update all scripts to exit 0 for PASS/ FAIL/ SKIP

[jeevanandan-sandan]

Changed all the exit codes to zero irrespective of test result.

[smuppand]

Centralize common UFS pre-checks
Every script repeats:

• mandatory/optional config checks
• DT node check
• detect_ufs_partition_block

This should be a reusable helper in functestlib.sh, e.g.:
ufs_common_precheck that returns SKIP reason and exports UFS_BLOCK_DEV / UFS_SYSFS_BASE.
Less duplication = fewer inconsistencies.

[smuppand]

Use log_info/log_warn/log_fail from functestlib (or keep it silent and let caller log).

[jeevanandan-sandan]

Removed echo statements and used logger functions to log the messages

[smuppand]

It's the same here as well.

[jeevanandan-sandan]

Removed echo statements and used logger functions to log the messages

[smuppand]

It's the same here as well.

[jeevanandan-sandan]

Removed echo statements and used logger functions to log the messages

[smuppand]

It's the same here as well.

[jeevanandan-sandan]

Removed echo statements and used logger functions to log the messages

[smuppand]

It's the same here as well.

[jeevanandan-sandan]

Removed echo statements and used logger functions to log the messages

[smuppand]

Wait times: 60s/180s hardcoded, will be flaky across boards.

Provide env overrides:
LOAD_MB=${LOAD_MB:-128}
IDLE_WAIT_SEC=${IDLE_WAIT_SEC:-60}

[jeevanandan-sandan]

Instead of waiting indefinitely implemented timeout mechanism.

[smuppand]

Failing to read max_freq currently leads to FAIL (good), but elsewhere similar read failures are treated as SKIP. Rule of thumb:
“Feature/sysfs node not present” → SKIP
“Node present but unreadable / invalid value” → FAIL (because kernel/userspace is broken)

[smuppand]

is too broad. Might match unreleated nodes or multiple controllers. Detect actual UFS dir once and reuse.
e.g., locate .../ufshcd* / .../ufs base and store as UFS_SYSFS.

[smuppand]

For every test using temp files:
tmpfile=$(mktemp "$test_path/${TESTNAME}.XXXXXX") (fallback if mktemp not available)
trap 'rm -f "$tmpfile"' EXIT INT TERM
This avoids stale files and collisions.

[smuppand]

The script starts both dd and a continuous sync loop, but there is no trap to clean them up if the script is interrupted. Background processes can be left running on interruption or unexpected exit.
In LAVA/CI, orphaned dd or sync loops can interfere with later tests.
Recommended fix:

cleanup() {
    [ -n "${DD_PID:-}" ] && kill "$DD_PID" 2>/dev/null || true
    [ -n "${sync_pid:-}" ] && kill "$sync_pid" 2>/dev/null || true
    [ -n "${tmpfile:-}" ] && rm -f "$tmpfile"
}
trap cleanup EXIT INT TERM

[smuppand]

find -printf is not POSIX and absent in busybox builds sometimes.
If stat isn’t available, just ls -l "$tmpfile" or wc -c < "$tmpfile".

[jeevanandan-sandan]

added fallbacks with word count if stat is not available

[smuppand]

expand via set -- $pattern with unquoted expansion (it will expand globs), then iterate.

Or require callers to pass unquoted patterns (not acceptable; will break static analysis and is fragile).

[jeevanandan-sandan]

the ufs_get_dt_node_path function uses set -- $pattern and iterates over the glob expansion to check the match.

[smuppand]

The spec version node returns the value in the format specified by JEDEC (E.g. 0x0310)

version1 is like 0x0310. POSIX arithmetic expansion supports bases, but $((version1)) treats version1 as a variable name, not the contents. In many shells this becomes 0 (or error), so version compare breaks.

ver1_dec=$((16#$(printf '%s' "$version1" | sed 's/^0x//')))
ver2_dec=$((16#$(printf '%s' "$version2" | sed 's/^0x//')))

Or simplest: avoid arithmetic compare; compare strings by known set

[smuppand]

this is not a SKIP condition, you mark FAIL, but log_skip. Use log_fail

Also you mean Failed to read min frequency from $UFS_MIN_FREQ_NODE ?

[jeevanandan-sandan]

Updated the log_skip statement to log_fail and UFS_MIN_FREQ to UFS_MIN_FREQ_NODE

[smuppand]

You added:
get_dt_node_path
check_node_status
check_node_status_with_timeout
ufs_precheck_common
The repo already has extensive helpers (and you’ve standardized function names), this risks:
name collisions (same function name defined elsewhere)

If they don’t, at least namespace them: ufs_get_dt_node_path, ufs_check_node_status, etc., to avoid generic collisions.

[jeevanandan-sandan]

Updated all the ufs specific functions with prefix ufs_*

[smuppand]

But function takes no args and uses logging + exports UFS_BLOCK_DEV.
Fix the comment and clearly document outputs:
exports UFS_BLOCK_DEV
returns 0/1
requires check_kernel_config, check_dt_nodes, detect_ufs_partition_block

[jeevanandan-sandan]

Updated the function documentation based on the function implementation

[smuppand]

Still it is not addressed.

Default smaller (64–256MB) and allow override via env:
LOAD_MB=${LOAD_MB:-256}
Or use count=128 (256MB at 2M blocks) as default.

[smuppand]

SC3034 is about local in dash/ash etc, not relevant here. If you meant another warning, clean it up.

[jeevanandan-sandan]

Removed the shellcheck disable SC3034.

[smuppand]

check_node_status* should handle empty reads explicitly
If cat fails, status="", and you log:
“Current status: ''” repeatedly
Suggestion:
When empty, log a distinct message (“node unreadable”) and maybe break earlier with FAIL/SKIP depending on node existence.

[jeevanandan-sandan]

If the node status is empty the function terminates the loop and returns 1 by adding a log_fail statement.

[smuppand]

order got changed from scan_dmesg_errors "ufs" "$test_path" to scan_dmesg_errors "$test_path" "ufs"

[jeevanandan-sandan]

The argument order is same as the function implementation in functestlib.sh so kept unchanged

[smuppand]

Still not addressed. Potentially destructive load generation (2GB dd tempfile everywhere, no free-space guard).

[smuppand]

You already do ./run.sh || true, so send-to-lava can usually be without || true if run.sh always writes res. But keeping it is ok for robustness.

[smuppand]

No newline at EOF

[jeevanandan-sandan]

added newline at the EOF for all yaml files

[smuppand]

should be more generic using ufs_get_dt_node_path with multiple fallback globs.

[jeevanandan-sandan]

Getting the primary ufs controller where the rootfs is mounted and using that to get the ufs clock freq nodes

[smuppand]

Now the repo license is changed to BSD-3-Clause. Fix it in all the files.
scan_dmesg_errors argument order got changed. scan_dmesg_errors "$test_path" "ufs"

[github-actions]

@ualcomm/qualcomm-linux-testing.triage This pull request has been marked as stale due to 30 days of inactivity.

[smuppand]

Have proper commit message along with body about the changes.

[smuppand]

The guard is inverted. This sources init_env only when __INIT_ENV_LOADED is already set. On a normal first-time run, init_env will not be sourced, but the script still immediately sources "$TOOLS/functestlib.sh"

recommended fix, applies to other run.sh files as well

if [ -z "${__INIT_ENV_LOADED:-}" ]; then
    # shellcheck disable=SC1090
    . "$INIT_ENV"
fi

[smuppand]

The script also uses cat, tr, kill, sync, and rm, but only checks dd sleep.

[smuppand]

Return value is ignored. If rootfs is too small, the test still starts a 2 GiB write, which can fill the filesystem or create noisy CI failures.
Recommended fix:

if ! ensure_rootfs_min_size 3; then
    log_skip "Insufficient rootfs free space for UFS clock scaling load"
    echo "$TESTNAME SKIP" > "$res_file"
    exit 0
fi

This same fix applies to ufs_gear_check, ufs_hibern8, and ufs_write_booster.

[smuppand]

This writes a 2 GiB tmpfile in the test directory. This is heavy for CI, can be slow on smaller devices, and can cause space pressure. The test only needs enough sustained write pressure to trigger devfreq it does not necessarily need a fixed 2 GiB write on every board.

Recommended fix: Parameterize the load size and default to a smaller value, for example:

UFS_LOAD_MB="${UFS_LOAD_MB:-512}"
count=$((UFS_LOAD_MB / 2))
dd if=/dev/zero of="$tmpfile" bs=2M count="$count" >/dev/null 2>&1 &

Set larger values from YAML/LAVA only when needed.

[smuppand]

The script starts both dd and a continuous sync loop, but there is no trap to clean them up if the script is interrupted. Background processes can be left running on interruption or unexpected exit.
In LAVA/CI, orphaned dd or sync loops can interfere with later tests.
Recommended fix:

cleanup() {
    [ -n "${DD_PID:-}" ] && kill "$DD_PID" 2>/dev/null || true
    [ -n "${sync_pid:-}" ] && kill "$sync_pid" 2>/dev/null || true
    [ -n "${tmpfile:-}" ] && rm -f "$tmpfile"
}
trap cleanup EXIT INT TERM

[smuppand]

This checks whether dd accepts iflag=direct, but it does not test whether direct I/O works with the actual UFS block device.
Recommended fix: log it as “dd supports iflag=direct” rather than assuming the device will. Current fallback is acceptable because the later real command gates PASS/FAIL.

[smuppand]

The tmpfile path is unquoted inside the constructed command string. Avoid command strings and call dd directly in branches, or keep the path safe with positional execution. Since paths here are controlled, this is non-blocking.

[smuppand]

Redirecting check_kernel_config stderr hides useful diagnostic output. Also, because check_kernel_config already logs config status, suppressing part of its output can reduce triage quality.
CI logs may not clearly show exactly which mandatory config failed.

Recommended fix:

if ! check_kernel_config "$MANDATORY_CONFIGS"; then
or explicitly log the missing config after checking each one

[smuppand]

This helper starts a background continuous sync loop every time it checks a node. This can create unnecessary I/O and make helper behavior surprising.

Recommended fix: Split into two helpers:
ufs_check_node_status() should only poll.
Callers that need sync pressure should start/stop sync explicitly or pass an option.

[smuppand]

Parsing mount output is less robust than findmnt, and findmnt is already used in some scripts.
Recommended fix:
dev=$(findmnt -n -o SOURCE "$user_partition" 2>/dev/null)
with /proc/mounts fallback when findmnt is unavailable.