Messages in this thread Patch in this message | | | Date | Wed, 1 Mar 2023 12:59:18 +0200 | Subject | Re: [PATCH v2 3/4] firmware: qcom_scm: Refactor code to support multiple download mode | From | Dmitry Baryshkov <> |
| |
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.
> +}; > + > +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.
> +}; > + > +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.
> + > 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[] = {
-- With best wishes Dmitry
| |