| Message ID | 1677664555-30191-4-git-send-email-quic_mojha@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Refactor to support multiple download mode | expand |
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[] = {
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[] = { >
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.
On 3/16/2023 9:57 PM, Dmitry Baryshkov wrote: > 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. Oops !! you are right.., Will fix it in v3. -Mukesh >
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, +}; + +static struct kparam_string dload_mode_param = { + .string = download_mode, + .maxlen = MAX_DLOAD_LEN, +}; + +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"); + 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[] = {
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(-)