Re: [PATCH v3 04/18] 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:

Pre-existing issues:
- [High] Missing smp_load_acquire() when reading the global __scm pointer in set_download_mode() risks a race condition and stale reads.
--

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

firmware: qcom: scm: Add minidump SRAM support

On most Qualcomm SoCs where minidump is supported, a word in always-on
SRAM is shared between the kernel and boot firmware. This commit adds
minidump SRAM support and exposes a minidump_dest module parameter so
the user can select the destination.

> 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
[ ... ]
> @@ -568,6 +583,14 @@ static void qcom_scm_set_download_mode(u32 dload_mode)
>  
>  	if (ret)
>  		dev_err(__scm->dev, "failed to set download mode: %d\n", ret);
> +
> +	/*
> +	 * Write the destination into the always-on SRAM so boot firmware
> +	 * can read it before DDR is initialised on the next warm reset.
> +	 * Only written when minidump is active;
> +	 */
> +	if (__scm->minidump_sram && (dload_mode & QCOM_DLOAD_MINIDUMP))
> +		writel_relaxed(minidump_dest, __scm->minidump_sram);
>  }

[Severity: High]
This is a pre-existing issue, but is it safe to read the global __scm
pointer here without using smp_load_acquire()?

The driver publishes the global __scm pointer at the end of qcom_scm_probe()
using smp_store_release(&__scm, scm). Module parameters are exposed via
sysfs before probe completes, allowing userspace to concurrently trigger the
set_download_mode() callback.

This callback reads __scm using a plain, lockless memory access. Without a
matching smp_load_acquire(), the CPU can observe a non-NULL __scm pointer
while still seeing stale or uninitialized values for its members, including
the newly added __scm->minidump_sram.

Could this lead to an unaligned access or page fault if writel_relaxed()
is passed an uninitialized pointer? 

Note that the newly added set_minidump_dest() correctly uses
smp_load_acquire() to prevent this exact issue, but
qcom_scm_set_download_mode() remains unprotected.

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