| Message ID | 1679935281-18445-5-git-send-email-quic_mojha@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Refactor to support multiple download mode | expand |
On Mon, Mar 27, 2023 at 10:11:20PM +0530, Mukesh Ojha wrote: [..] > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index 3c6c5e7..0c94429 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -20,11 +20,11 @@ > #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); > +static u32 download_mode; > > #define SCM_HAS_CORE_CLK BIT(0) > #define SCM_HAS_IFACE_CLK BIT(1) > @@ -32,6 +32,7 @@ module_param(download_mode, bool, 0); > > #define QCOM_DOWNLOAD_MODE_MASK 0x30 > #define QCOM_DOWNLOAD_FULLDUMP 0x1 > +#define QCOM_DOWNLOAD_NODUMP 0x0 > > struct qcom_scm { > struct device *dev; > @@ -440,8 +441,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 download_mode) > { > + bool enable = !!download_mode; > bool avail; > int ret = 0; > > @@ -453,7 +455,7 @@ static void qcom_scm_set_download_mode(bool enable) > } else if (__scm->dload_mode_addr) { > ret = qcom_scm_io_update_field(__scm->dload_mode_addr, > QCOM_DOWNLOAD_MODE_MASK, > - enable ? QCOM_DOWNLOAD_FULLDUMP : 0); > + enable ? download_mode : 0); Afaict, with QCOM_DOWNLOAD_NODUMP as 0, this says: when download_mode is non-zero, write that value, otherwise write 0 That should be the same as "write download_mode", so you should be able to drop the enable part. > } else { > dev_err(__scm->dev, > "No available mechanism for setting download mode\n"); > @@ -1419,6 +1421,49 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data) > return IRQ_HANDLED; > } > > + > +static int get_download_mode(char *buffer, const struct kernel_param *kp) > +{ > + int len = 0; > + > + if (download_mode == QCOM_DOWNLOAD_FULLDUMP) > + len = sysfs_emit(buffer, "full\n"); > + else if (download_mode == QCOM_DOWNLOAD_NODUMP) > + len = sysfs_emit(buffer, "off\n"); > + > + return len; > +} > + > +static int set_download_mode(const char *val, const struct kernel_param *kp) > +{ > + u32 old = download_mode; > + > + if (!strncmp(val, "full", strlen("full"))) { strcmp loops over the two string until they differ and/or both are '\0'. As such, the only thing you achieve by using strncmp(.., T, strlen(T)) is that the code has to iterate over T twice - and you make the code harder to read. > + download_mode = QCOM_DOWNLOAD_FULLDUMP; > + } else if (!strncmp(val, "off", strlen("off"))) { > + download_mode = QCOM_DOWNLOAD_NODUMP; > + } else if (kstrtouint(val, 0, &download_mode) || > + !(download_mode == 0 || download_mode == 1)) { > + download_mode = old; > + pr_err("unknown download mode\n"); This will result in a lone "unknown download mode" line somewhere in the kernel log, without association to any driver or any indication what the unknown value was. pr_err("qcom_scm: unknown download mode: %s\n", val); Would give both context and let the reader know right there what value the code wasn't able to match. Regards, Bjorn
On 3/27/2023 11:53 PM, Bjorn Andersson wrote: > On Mon, Mar 27, 2023 at 10:11:20PM +0530, Mukesh Ojha wrote: > [..] >> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >> index 3c6c5e7..0c94429 100644 >> --- a/drivers/firmware/qcom_scm.c >> +++ b/drivers/firmware/qcom_scm.c >> @@ -20,11 +20,11 @@ >> #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); >> +static u32 download_mode; >> >> #define SCM_HAS_CORE_CLK BIT(0) >> #define SCM_HAS_IFACE_CLK BIT(1) >> @@ -32,6 +32,7 @@ module_param(download_mode, bool, 0); >> >> #define QCOM_DOWNLOAD_MODE_MASK 0x30 >> #define QCOM_DOWNLOAD_FULLDUMP 0x1 >> +#define QCOM_DOWNLOAD_NODUMP 0x0 >> >> struct qcom_scm { >> struct device *dev; >> @@ -440,8 +441,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 download_mode) >> { >> + bool enable = !!download_mode; >> bool avail; >> int ret = 0; >> >> @@ -453,7 +455,7 @@ static void qcom_scm_set_download_mode(bool enable) >> } else if (__scm->dload_mode_addr) { >> ret = qcom_scm_io_update_field(__scm->dload_mode_addr, >> QCOM_DOWNLOAD_MODE_MASK, >> - enable ? QCOM_DOWNLOAD_FULLDUMP : 0); >> + enable ? download_mode : 0); > > Afaict, with QCOM_DOWNLOAD_NODUMP as 0, this says: > > when download_mode is non-zero, write that value, otherwise write 0 > > That should be the same as "write download_mode", so you should be able > to drop the enable part. > >> } else { >> dev_err(__scm->dev, >> "No available mechanism for setting download mode\n"); >> @@ -1419,6 +1421,49 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data) >> return IRQ_HANDLED; >> } >> >> + >> +static int get_download_mode(char *buffer, const struct kernel_param *kp) >> +{ >> + int len = 0; >> + >> + if (download_mode == QCOM_DOWNLOAD_FULLDUMP) >> + len = sysfs_emit(buffer, "full\n"); >> + else if (download_mode == QCOM_DOWNLOAD_NODUMP) >> + len = sysfs_emit(buffer, "off\n"); >> + >> + return len; >> +} >> + >> +static int set_download_mode(const char *val, const struct kernel_param *kp) >> +{ >> + u32 old = download_mode; >> + >> + if (!strncmp(val, "full", strlen("full"))) { > > strcmp loops over the two string until they differ and/or both are > '\0'. > > As such, the only thing you achieve by using strncmp(.., T, strlen(T)) > is that the code has to iterate over T twice - and you make the code > harder to read. If we use strcmp, i need to use "full\n" which we would not want to do. I think, we need to take this hit. -- Mukesh > >> + download_mode = QCOM_DOWNLOAD_FULLDUMP; >> + } else if (!strncmp(val, "off", strlen("off"))) { >> + download_mode = QCOM_DOWNLOAD_NODUMP; >> + } else if (kstrtouint(val, 0, &download_mode) || >> + !(download_mode == 0 || download_mode == 1)) { >> + download_mode = old; >> + pr_err("unknown download mode\n"); > > This will result in a lone "unknown download mode" line somewhere in the > kernel log, without association to any driver or any indication what the > unknown value was. > > pr_err("qcom_scm: unknown download mode: %s\n", val); > > Would give both context and let the reader know right there what value > the code wasn't able to match. > > Regards, > Bjorn
On 28/03/2023 11:18, Mukesh Ojha wrote: > > > On 3/27/2023 11:53 PM, Bjorn Andersson wrote: >> On Mon, Mar 27, 2023 at 10:11:20PM +0530, Mukesh Ojha wrote: >> [..] >>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >>> index 3c6c5e7..0c94429 100644 >>> --- a/drivers/firmware/qcom_scm.c >>> +++ b/drivers/firmware/qcom_scm.c >>> @@ -20,11 +20,11 @@ >>> #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); >>> +static u32 download_mode; >>> #define SCM_HAS_CORE_CLK BIT(0) >>> #define SCM_HAS_IFACE_CLK BIT(1) >>> @@ -32,6 +32,7 @@ module_param(download_mode, bool, 0); >>> #define QCOM_DOWNLOAD_MODE_MASK 0x30 >>> #define QCOM_DOWNLOAD_FULLDUMP 0x1 >>> +#define QCOM_DOWNLOAD_NODUMP 0x0 >>> struct qcom_scm { >>> struct device *dev; >>> @@ -440,8 +441,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 download_mode) >>> { >>> + bool enable = !!download_mode; >>> bool avail; >>> int ret = 0; >>> @@ -453,7 +455,7 @@ static void qcom_scm_set_download_mode(bool enable) >>> } else if (__scm->dload_mode_addr) { >>> ret = qcom_scm_io_update_field(__scm->dload_mode_addr, >>> QCOM_DOWNLOAD_MODE_MASK, >>> - enable ? QCOM_DOWNLOAD_FULLDUMP : 0); >>> + enable ? download_mode : 0); >> >> Afaict, with QCOM_DOWNLOAD_NODUMP as 0, this says: >> >> when download_mode is non-zero, write that value, otherwise write 0 >> >> That should be the same as "write download_mode", so you should be able >> to drop the enable part. >> >>> } else { >>> dev_err(__scm->dev, >>> "No available mechanism for setting download mode\n"); >>> @@ -1419,6 +1421,49 @@ static irqreturn_t qcom_scm_irq_handler(int >>> irq, void *data) >>> return IRQ_HANDLED; >>> } >>> + >>> +static int get_download_mode(char *buffer, const struct kernel_param >>> *kp) >>> +{ >>> + int len = 0; >>> + >>> + if (download_mode == QCOM_DOWNLOAD_FULLDUMP) >>> + len = sysfs_emit(buffer, "full\n"); >>> + else if (download_mode == QCOM_DOWNLOAD_NODUMP) >>> + len = sysfs_emit(buffer, "off\n"); >>> + >>> + return len; >>> +} >>> + >>> +static int set_download_mode(const char *val, const struct >>> kernel_param *kp) >>> +{ >>> + u32 old = download_mode; >>> + >>> + if (!strncmp(val, "full", strlen("full"))) { >> >> strcmp loops over the two string until they differ and/or both are >> '\0'. >> >> As such, the only thing you achieve by using strncmp(.., T, strlen(T)) >> is that the code has to iterate over T twice - and you make the code >> harder to read. > > > If we use strcmp, i need to use "full\n" which we would not want to do. > I think, we need to take this hit. There is a special helper for the sysfs files. See sysfs_streq(). > > -- Mukesh >> >>> + download_mode = QCOM_DOWNLOAD_FULLDUMP; >>> + } else if (!strncmp(val, "off", strlen("off"))) { >>> + download_mode = QCOM_DOWNLOAD_NODUMP; >>> + } else if (kstrtouint(val, 0, &download_mode) || >>> + !(download_mode == 0 || download_mode == 1)) { >>> + download_mode = old; >>> + pr_err("unknown download mode\n"); >> >> This will result in a lone "unknown download mode" line somewhere in the >> kernel log, without association to any driver or any indication what the >> unknown value was. >> >> pr_err("qcom_scm: unknown download mode: %s\n", val); >> >> Would give both context and let the reader know right there what value >> the code wasn't able to match. >> >> Regards, >> Bjorn
On 3/29/2023 3:44 AM, Dmitry Baryshkov wrote: > On 28/03/2023 11:18, Mukesh Ojha wrote: >> >> >> On 3/27/2023 11:53 PM, Bjorn Andersson wrote: >>> On Mon, Mar 27, 2023 at 10:11:20PM +0530, Mukesh Ojha wrote: >>> [..] >>>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >>>> index 3c6c5e7..0c94429 100644 >>>> --- a/drivers/firmware/qcom_scm.c >>>> +++ b/drivers/firmware/qcom_scm.c >>>> @@ -20,11 +20,11 @@ >>>> #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); >>>> +static u32 download_mode; >>>> #define SCM_HAS_CORE_CLK BIT(0) >>>> #define SCM_HAS_IFACE_CLK BIT(1) >>>> @@ -32,6 +32,7 @@ module_param(download_mode, bool, 0); >>>> #define QCOM_DOWNLOAD_MODE_MASK 0x30 >>>> #define QCOM_DOWNLOAD_FULLDUMP 0x1 >>>> +#define QCOM_DOWNLOAD_NODUMP 0x0 >>>> struct qcom_scm { >>>> struct device *dev; >>>> @@ -440,8 +441,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 download_mode) >>>> { >>>> + bool enable = !!download_mode; >>>> bool avail; >>>> int ret = 0; >>>> @@ -453,7 +455,7 @@ static void qcom_scm_set_download_mode(bool enable) >>>> } else if (__scm->dload_mode_addr) { >>>> ret = qcom_scm_io_update_field(__scm->dload_mode_addr, >>>> QCOM_DOWNLOAD_MODE_MASK, >>>> - enable ? QCOM_DOWNLOAD_FULLDUMP : 0); >>>> + enable ? download_mode : 0); >>> >>> Afaict, with QCOM_DOWNLOAD_NODUMP as 0, this says: >>> >>> when download_mode is non-zero, write that value, otherwise write 0 >>> >>> That should be the same as "write download_mode", so you should be able >>> to drop the enable part. >>> >>>> } else { >>>> dev_err(__scm->dev, >>>> "No available mechanism for setting download mode\n"); >>>> @@ -1419,6 +1421,49 @@ static irqreturn_t qcom_scm_irq_handler(int >>>> irq, void *data) >>>> return IRQ_HANDLED; >>>> } >>>> + >>>> +static int get_download_mode(char *buffer, const struct >>>> kernel_param *kp) >>>> +{ >>>> + int len = 0; >>>> + >>>> + if (download_mode == QCOM_DOWNLOAD_FULLDUMP) >>>> + len = sysfs_emit(buffer, "full\n"); >>>> + else if (download_mode == QCOM_DOWNLOAD_NODUMP) >>>> + len = sysfs_emit(buffer, "off\n"); >>>> + >>>> + return len; >>>> +} >>>> + >>>> +static int set_download_mode(const char *val, const struct >>>> kernel_param *kp) >>>> +{ >>>> + u32 old = download_mode; >>>> + >>>> + if (!strncmp(val, "full", strlen("full"))) { >>> >>> strcmp loops over the two string until they differ and/or both are >>> '\0'. >>> >>> As such, the only thing you achieve by using strncmp(.., T, strlen(T)) >>> is that the code has to iterate over T twice - and you make the code >>> harder to read. >> >> >> If we use strcmp, i need to use "full\n" which we would not want to do. >> I think, we need to take this hit. > > There is a special helper for the sysfs files. See sysfs_streq(). You are awesome !! Thanks. Have applied the change. -- Mukesh > >> >> -- Mukesh >>> >>>> + download_mode = QCOM_DOWNLOAD_FULLDUMP; >>>> + } else if (!strncmp(val, "off", strlen("off"))) { >>>> + download_mode = QCOM_DOWNLOAD_NODUMP; >>>> + } else if (kstrtouint(val, 0, &download_mode) || >>>> + !(download_mode == 0 || download_mode == 1)) { >>>> + download_mode = old; >>>> + pr_err("unknown download mode\n"); >>> >>> This will result in a lone "unknown download mode" line somewhere in the >>> kernel log, without association to any driver or any indication what the >>> unknown value was. >>> >>> pr_err("qcom_scm: unknown download mode: %s\n", val); >>> >>> Would give both context and let the reader know right there what value >>> the code wasn't able to match. >>> >>> Regards, >>> Bjorn >
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 3c6c5e7..0c94429 100644 --- a/drivers/firmware/qcom_scm.c +++ b/drivers/firmware/qcom_scm.c @@ -20,11 +20,11 @@ #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); +static u32 download_mode; #define SCM_HAS_CORE_CLK BIT(0) #define SCM_HAS_IFACE_CLK BIT(1) @@ -32,6 +32,7 @@ module_param(download_mode, bool, 0); #define QCOM_DOWNLOAD_MODE_MASK 0x30 #define QCOM_DOWNLOAD_FULLDUMP 0x1 +#define QCOM_DOWNLOAD_NODUMP 0x0 struct qcom_scm { struct device *dev; @@ -440,8 +441,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 download_mode) { + bool enable = !!download_mode; bool avail; int ret = 0; @@ -453,7 +455,7 @@ static void qcom_scm_set_download_mode(bool enable) } else if (__scm->dload_mode_addr) { ret = qcom_scm_io_update_field(__scm->dload_mode_addr, QCOM_DOWNLOAD_MODE_MASK, - enable ? QCOM_DOWNLOAD_FULLDUMP : 0); + enable ? download_mode : 0); } else { dev_err(__scm->dev, "No available mechanism for setting download mode\n"); @@ -1419,6 +1421,49 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data) return IRQ_HANDLED; } + +static int get_download_mode(char *buffer, const struct kernel_param *kp) +{ + int len = 0; + + if (download_mode == QCOM_DOWNLOAD_FULLDUMP) + len = sysfs_emit(buffer, "full\n"); + else if (download_mode == QCOM_DOWNLOAD_NODUMP) + len = sysfs_emit(buffer, "off\n"); + + return len; +} + +static int set_download_mode(const char *val, const struct kernel_param *kp) +{ + u32 old = download_mode; + + if (!strncmp(val, "full", strlen("full"))) { + download_mode = QCOM_DOWNLOAD_FULLDUMP; + } else if (!strncmp(val, "off", strlen("off"))) { + download_mode = QCOM_DOWNLOAD_NODUMP; + } else if (kstrtouint(val, 0, &download_mode) || + !(download_mode == 0 || download_mode == 1)) { + download_mode = old; + pr_err("unknown download mode\n"); + return -EINVAL; + } + + if (__scm) + qcom_scm_set_download_mode(download_mode); + + return 0; +} + +static const struct kernel_param_ops download_mode_param_ops = { + .get = get_download_mode, + .set = set_download_mode, +}; + +module_param_cb(download_mode, &download_mode_param_ops, NULL, 0644); +MODULE_PARM_DESC(download_mode, + "Download mode: off/full or 0/1 for existing users"); + static int qcom_scm_probe(struct platform_device *pdev) { struct qcom_scm *scm; @@ -1512,12 +1557,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); + qcom_scm_set_download_mode(download_mode); return 0; } @@ -1525,7 +1570,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> --- drivers/firmware/Kconfig | 11 --------- drivers/firmware/qcom_scm.c | 59 +++++++++++++++++++++++++++++++++++++++------ 2 files changed, 52 insertions(+), 18 deletions(-)