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,
+};
+
+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[] = {
-- 
2.7.4

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

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.

-- 
With best wishes
Dmitry

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

>