Messages in this thread | | | Date | Thu, 16 Mar 2023 18:27:33 +0200 | Subject | Re: [PATCH v2 3/4] firmware: qcom_scm: Refactor code to support multiple download mode | From | Dmitry Baryshkov <> |
| |
On 03/03/2023 09:46, Mukesh Ojha wrote: > Thanks for your time in checking this.. > > On 3/1/2023 4:29 PM, Dmitry Baryshkov wrote: >> On 01/03/2023 11:55, Mukesh Ojha wrote: >>> Currently on Qualcomm SoC, download_mode is enabled if >>> CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT is selected. >>> >>> Refactor the code such that it supports multiple download >>> modes and drop CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT config >>> instead, give interface to set the download mode from >>> module parameter. >>> >>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >>> --- >>> Changes in v2: >>> - Passed download mode as parameter >>> - Accept human accepatable download mode string. >>> - enable = !!dload_mode >>> - Shifted module param callback to somewhere down in >>> the file so that it no longer need to know the >>> prototype of qcom_scm_set_download_mode() >>> - updated commit text. >>> >>> drivers/firmware/Kconfig | 11 -------- >>> drivers/firmware/qcom_scm.c | 65 >>> ++++++++++++++++++++++++++++++++++++++------- >>> 2 files changed, 56 insertions(+), 20 deletions(-) >>> >>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig >>> index b59e304..ff7e9f3 100644 >>> --- a/drivers/firmware/Kconfig >>> +++ b/drivers/firmware/Kconfig >>> @@ -215,17 +215,6 @@ config MTK_ADSP_IPC >>> config QCOM_SCM >>> tristate >>> -config QCOM_SCM_DOWNLOAD_MODE_DEFAULT >>> - bool "Qualcomm download mode enabled by default" >>> - depends on QCOM_SCM >>> - help >>> - A device with "download mode" enabled will upon an unexpected >>> - warm-restart enter a special debug mode that allows the user to >>> - "download" memory content over USB for offline postmortem >>> analysis. >>> - The feature can be enabled/disabled on the kernel command line. >>> - >>> - Say Y here to enable "download mode" by default. >>> - >>> config SYSFB >>> bool >>> select BOOT_VESA_SUPPORT >>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >>> index c9f1fad..ca07208 100644 >>> --- a/drivers/firmware/qcom_scm.c >>> +++ b/drivers/firmware/qcom_scm.c >>> @@ -17,17 +17,22 @@ >>> #include <linux/clk.h> >>> #include <linux/reset-controller.h> >>> #include <linux/arm-smccc.h> >>> +#include <linux/kstrtox.h> >>> #include "qcom_scm.h" >>> -static bool download_mode = >>> IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT); >>> -module_param(download_mode, bool, 0); >>> - >>> #define SCM_HAS_CORE_CLK BIT(0) >>> #define SCM_HAS_IFACE_CLK BIT(1) >>> #define SCM_HAS_BUS_CLK BIT(2) >>> #define QCOM_DOWNLOAD_MODE_MASK 0x30 >>> +#define QCOM_DOWNLOAD_FULLDUMP 0x10 >>> +#define QCOM_DOWNLOAD_NODUMP 0x0 >>> + >>> +#define MAX_DLOAD_LEN 8 >>> + >>> +static char download_mode[] = "off"; >>> +static u32 dload_mode; >>> struct qcom_scm { >>> struct device *dev; >>> @@ -417,8 +422,9 @@ static int __qcom_scm_set_dload_mode(struct >>> device *dev, bool enable) >>> return qcom_scm_call_atomic(__scm->dev, &desc, NULL); >>> } >>> -static void qcom_scm_set_download_mode(bool enable) >>> +static void qcom_scm_set_download_mode(u32 dload_mode) >>> { >>> + bool enable = !!dload_mode; >>> bool avail; >>> int ret = 0; >>> u32 val; >>> @@ -438,7 +444,7 @@ static void qcom_scm_set_download_mode(bool enable) >>> val &= ~QCOM_DOWNLOAD_MODE_MASK; >>> if (enable) >>> - val |= QCOM_SCM_BOOT_SET_DLOAD_MODE; >>> + val |= dload_mode; >>> ret = qcom_scm_io_writel(__scm->dload_mode_addr, val); >>> } else { >>> @@ -1338,6 +1344,47 @@ bool qcom_scm_is_available(void) >>> } >>> EXPORT_SYMBOL(qcom_scm_is_available); >>> +static int set_dload_mode(const char *val, const struct kernel_param >>> *kp) >>> +{ >>> + int ret; >>> + >>> + if (!strncmp(val, "full", strlen("full"))) { >>> + dload_mode = QCOM_DOWNLOAD_FULLDUMP; >>> + } else if (!strncmp(val, "off", strlen("off"))) { >>> + dload_mode = QCOM_DOWNLOAD_NODUMP; >>> + } else { >>> + if (kstrtouint(val, 0, &dload_mode) || >>> + !(dload_mode == 0 || dload_mode == 1)) { >>> + pr_err("unknown download mode\n"); >>> + return -EINVAL; >>> + } >>> + >>> + } >>> + >>> + ret = param_set_copystring(val, kp); >>> + if (ret) >>> + return ret; >>> + >>> + if (__scm) >>> + qcom_scm_set_download_mode(dload_mode); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct kernel_param_ops dload_mode_param_ops = { >>> + .set = set_dload_mode, >>> + .get = param_get_string, >> >> Please follow the param_get_bool approach and return sanitized data >> here. In other words, "full" / "none" depending on the dload_mode >> instead of storing the passed string and returning it later. >> > > This could be done. > >>> +}; >>> + >>> +static struct kparam_string dload_mode_param = { >>> + .string = download_mode, >>> + .maxlen = MAX_DLOAD_LEN, >> >> I think writing "full" to this param might overwrite some kernel data. >> "00000000" should be even worse. > > There is check in param_set_copystring() which would avoid that.
No. The check will validate the value's length against MAX_DLOAD_LEN. But it will not safeguard your code. download_mode's size is less than MAX_DLOAD_LEN.
-- With best wishes Dmitry
| |