| Message ID | 1677664555-30191-1-git-send-email-quic_mojha@quicinc.com |
|---|---|
| Headers | show |
| Series | Refactor to support multiple download mode | expand |
On 01/03/2023 09:55, Mukesh Ojha wrote: > During normal restart of a system download bit should > be cleared irrespective of whether download mode is > set or not. Looks like this is a fix, Fixes tag would help here. --srini > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > --- > Changes in v2: > - No change > > drivers/firmware/qcom_scm.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index cdbfe54..51eb853 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -1418,8 +1418,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 */ > - if (download_mode) > - qcom_scm_set_download_mode(false); > + qcom_scm_set_download_mode(false); > } > > static const struct of_device_id qcom_scm_dt_match[] = {
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 >
While 1/4 and 2/4 are the fixes and 3/4 and 4/4 are trying to support multiple download mode like full/mini/both dump. 4/4 will help to enable minidump mode for Qualcomm SoC. Minidump kernel driver patches has been sent here https://lore.kernel.org/lkml/1676978713-7394-1-git-send-email-quic_mojha@quicinc.com/ Mukesh Ojha (4): firmware: qcom_scm: Clear download bit during reboot firmware: scm: Modify only the DLOAD bit in TCSR register for download mode firmware: qcom_scm: Refactor code to support multiple download mode firmware: qcom_scm: Add multiple download mode support drivers/firmware/Kconfig | 11 ------ drivers/firmware/qcom_scm.c | 87 +++++++++++++++++++++++++++++++++++++++------ 2 files changed, 76 insertions(+), 22 deletions(-)