Re: [PATCH v2 3/4] firmware: qcom: scm: Add minidump SRAM support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The global `__scm` pointer is read concurrently in `set_minidump_dest()` without the required `smp_load_acquire()` or `READ_ONCE()` barrier, creating a data race.
--

commit 46d7aebbb8abe9c9e0e6c6cfdb3495908a439bbd
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

firmware: qcom: scm: Add minidump SRAM support

This commit adds support for writing minidump destination values to
always-on SRAM. It exposes a module parameter to let users select the
destination, which updates the SRAM immediately if minidump mode is active.

> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
[ ... ]
> @@ -2733,6 +2785,40 @@ static const struct kernel_param_ops download_mode_param_ops = {
[ ... ]
> +static int set_minidump_dest(const char *val, const struct kernel_param *kp)
> +{
> +	int i;
> +
> +	i = sysfs_match_string(minidump_dest_name, val);
> +	if (i < 0)
> +		return -EINVAL;
> +
> +	minidump_dest = minidump_dest_val[i];
> +	if (__scm && __scm->minidump_sram && (download_mode & QCOM_DLOAD_MINIDUMP))

[Severity: Medium]
Could this plain read of __scm result in a data race?

In qcom_scm_probe(), the global __scm pointer is published using
smp_store_release(&__scm, scm). Since module parameters are exposed in
sysfs early, this setter might be executed concurrently while the probe
function is running.

Without a matching smp_load_acquire() or READ_ONCE(), is it possible for
the compiler or CPU to reorder the memory accesses? This might result in
observing a non-NULL __scm but an uninitialized __scm->minidump_sram value,
which could then lead to an arbitrary memory write via writel_relaxed().

Would it be safer to safely acquire the pointer first, for example using:
struct qcom_scm *scm = smp_load_acquire(&__scm);

> +		writel_relaxed(minidump_dest, __scm->minidump_sram);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519171442.1582987-1-mukesh.ojha@oss.qualcomm.com?part=3