Messages in this thread | | | Date | Fri, 3 Mar 2023 13:16:41 +0530 | Subject | Re: [PATCH v2 3/4] firmware: qcom_scm: Refactor code to support multiple download mode | From | Mukesh Ojha <> |
| |
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.
> >> +}; >> + >> +module_param_cb(download_mode, &dload_mode_param_ops, >> &dload_mode_param, 0644); >> +MODULE_PARM_DESC(download_mode, >> + "Download mode: off/full or 0/1/off/on for existing users"); > > Nit: on is not supported even for existing users.
Agree. just 0/1 would do fine there.
-Mukesh > >> + >> static int qcom_scm_probe(struct platform_device *pdev) >> { >> struct qcom_scm *scm; >> @@ -1418,12 +1465,12 @@ static int qcom_scm_probe(struct >> platform_device *pdev) >> __get_convention(); >> /* >> - * If requested enable "download mode", from this point on warmboot >> + * If download mode is requested, from this point on warmboot >> * will cause the boot stages to enter download mode, unless >> * disabled below by a clean shutdown/reboot. >> */ >> - if (download_mode) >> - qcom_scm_set_download_mode(true); >> + if (dload_mode) >> + qcom_scm_set_download_mode(dload_mode); >> return 0; >> } >> @@ -1431,7 +1478,7 @@ static int qcom_scm_probe(struct platform_device >> *pdev) >> static void qcom_scm_shutdown(struct platform_device *pdev) >> { >> /* Clean shutdown, disable download mode to allow normal restart */ >> - qcom_scm_set_download_mode(false); >> + qcom_scm_set_download_mode(QCOM_DOWNLOAD_NODUMP); >> } >> static const struct of_device_id qcom_scm_dt_match[] = { >
| |