[PATCH v2 0/4] Refactor to support multiple download mode

[PATCH v2 0/4] Refactor to support multiple download mode

[Mukesh Ojha]

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(-)

-- 
2.7.4

[PATCH v2 1/4] firmware: qcom_scm: Clear download bit during reboot

[Mukesh Ojha]

During normal restart of a system download bit should
be cleared irrespective of whether download mode is
set or not.

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[] = {
-- 
2.7.4

[PATCH v2 2/4] firmware: scm: Modify only the DLOAD bit in TCSR register for download mode

[Mukesh Ojha]

CrashDump collection is based on the DLOAD bit of TCSR register.
To retain other bits, we read the register and modify only the
DLOAD bit as the other bits have their own significance.

Originally-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
Changes in v2:
 - Addressed comment made by Bjorn.
 - Added download mask from patch 3 to this.

 drivers/firmware/qcom_scm.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 51eb853..c9f1fad 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -27,6 +27,8 @@ module_param(download_mode, bool, 0);
 #define SCM_HAS_IFACE_CLK	BIT(1)
 #define SCM_HAS_BUS_CLK		BIT(2)
 
+#define QCOM_DOWNLOAD_MODE_MASK 0x30
+
 struct qcom_scm {
 	struct device *dev;
 	struct clk *core_clk;
@@ -419,6 +421,7 @@ static void qcom_scm_set_download_mode(bool enable)
 {
 	bool avail;
 	int ret = 0;
+	u32 val;
 
 	avail = __qcom_scm_is_call_available(__scm->dev,
 					     QCOM_SCM_SVC_BOOT,
@@ -426,8 +429,18 @@ static void qcom_scm_set_download_mode(bool enable)
 	if (avail) {
 		ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
 	} else if (__scm->dload_mode_addr) {
-		ret = qcom_scm_io_writel(__scm->dload_mode_addr,
-				enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
+		ret = qcom_scm_io_readl(__scm->dload_mode_addr, &val);
+		if (ret) {
+			dev_err(__scm->dev,
+				"failed to read dload mode address value: %d\n", ret);
+			return;
+		}
+
+		val &= ~QCOM_DOWNLOAD_MODE_MASK;
+		if (enable)
+			val |= QCOM_SCM_BOOT_SET_DLOAD_MODE;
+
+		ret = qcom_scm_io_writel(__scm->dload_mode_addr, val);
 	} else {
 		dev_err(__scm->dev,
 			"No available mechanism for setting download mode\n");
-- 
2.7.4

[PATCH v2 3/4] firmware: qcom_scm: Refactor code to support multiple download mode

[Mukesh Ojha]

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

[PATCH v2 4/4] firmware: qcom_scm: Add multiple download mode support

[Mukesh Ojha]

Currently, scm driver only supports full dump when download
mode is selected. Add support to enable minidump as well both
dump(full dump + minidump).

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 Changes in v2:
  - Accept mini/both as acceptable output.

 drivers/firmware/qcom_scm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index ca07208..39e071a 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -27,6 +27,8 @@
 
 #define QCOM_DOWNLOAD_MODE_MASK 0x30
 #define QCOM_DOWNLOAD_FULLDUMP	0x10
+#define QCOM_DOWNLOAD_MINIDUMP	0x20
+#define QCOM_DOWNLOAD_BOTHDUMP	(QCOM_DOWNLOAD_FULLDUMP | QCOM_DOWNLOAD_MINIDUMP)
 #define QCOM_DOWNLOAD_NODUMP	0x0
 
 #define MAX_DLOAD_LEN	8
@@ -1350,6 +1352,10 @@ static int set_dload_mode(const char *val, const struct kernel_param *kp)
 
 	if (!strncmp(val, "full", strlen("full"))) {
 		dload_mode = QCOM_DOWNLOAD_FULLDUMP;
+	} else if (!strncmp(val, "mini", strlen("mini"))) {
+		dload_mode = QCOM_DOWNLOAD_MINIDUMP;
+	} else if (!strncmp(val, "both", strlen("both"))) {
+		dload_mode = QCOM_DOWNLOAD_BOTHDUMP;
 	} else if (!strncmp(val, "off", strlen("off"))) {
 		dload_mode = QCOM_DOWNLOAD_NODUMP;
 	} else {
@@ -1383,7 +1389,7 @@ static struct kparam_string dload_mode_param = {
 
 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");
+		 "download mode: off/full/mini/both(full+mini) or 0/1/off/on for existing users");
 
 static int qcom_scm_probe(struct platform_device *pdev)
 {
-- 
2.7.4

Re: [PATCH v2 2/4] firmware: scm: Modify only the DLOAD bit in TCSR register for download mode

[Dmitry Baryshkov]

On 01/03/2023 11:55, Mukesh Ojha wrote:
> CrashDump collection is based on the DLOAD bit of TCSR register.
> To retain other bits, we read the register and modify only the
> DLOAD bit as the other bits have their own significance.
> 
> Originally-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
> Changes in v2:
>   - Addressed comment made by Bjorn.
>   - Added download mask from patch 3 to this.
> 
>   drivers/firmware/qcom_scm.c | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 51eb853..c9f1fad 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -27,6 +27,8 @@ module_param(download_mode, bool, 0);
>   #define SCM_HAS_IFACE_CLK	BIT(1)
>   #define SCM_HAS_BUS_CLK		BIT(2)
>   
> +#define QCOM_DOWNLOAD_MODE_MASK 0x30
> +
>   struct qcom_scm {
>   	struct device *dev;
>   	struct clk *core_clk;
> @@ -419,6 +421,7 @@ static void qcom_scm_set_download_mode(bool enable)
>   {
>   	bool avail;
>   	int ret = 0;
> +	u32 val;
>   
>   	avail = __qcom_scm_is_call_available(__scm->dev,
>   					     QCOM_SCM_SVC_BOOT,
> @@ -426,8 +429,18 @@ static void qcom_scm_set_download_mode(bool enable)
>   	if (avail) {
>   		ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
>   	} else if (__scm->dload_mode_addr) {
> -		ret = qcom_scm_io_writel(__scm->dload_mode_addr,
> -				enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
> +		ret = qcom_scm_io_readl(__scm->dload_mode_addr, &val);
> +		if (ret) {
> +			dev_err(__scm->dev,
> +				"failed to read dload mode address value: %d\n", ret);
> +			return;
> +		}
> +
> +		val &= ~QCOM_DOWNLOAD_MODE_MASK;
> +		if (enable)
> +			val |= QCOM_SCM_BOOT_SET_DLOAD_MODE;
> +
> +		ret = qcom_scm_io_writel(__scm->dload_mode_addr, val);

Any locking for this RMW?

>   	} else {
>   		dev_err(__scm->dev,
>   			"No available mechanism for setting download mode\n");

-- 
With best wishes
Dmitry

Re: [PATCH v2 3/4] firmware: qcom_scm: Refactor code to support multiple download mode

[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

Re: [PATCH v2 3/4] firmware: qcom_scm: Refactor code to support multiple download mode

[Mukesh Ojha]

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[] = {
> 

Re: [PATCH v2 2/4] firmware: scm: Modify only the DLOAD bit in TCSR register for download mode

[Mukesh Ojha]

On 3/1/2023 4:09 PM, Dmitry Baryshkov wrote:
> On 01/03/2023 11:55, Mukesh Ojha wrote:
>> CrashDump collection is based on the DLOAD bit of TCSR register.
>> To retain other bits, we read the register and modify only the
>> DLOAD bit as the other bits have their own significance.
>>
>> Originally-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>> Changes in v2:
>>   - Addressed comment made by Bjorn.
>>   - Added download mask from patch 3 to this.
>>
>>   drivers/firmware/qcom_scm.c | 17 +++++++++++++++--
>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index 51eb853..c9f1fad 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -27,6 +27,8 @@ module_param(download_mode, bool, 0);
>>   #define SCM_HAS_IFACE_CLK    BIT(1)
>>   #define SCM_HAS_BUS_CLK        BIT(2)
>> +#define QCOM_DOWNLOAD_MODE_MASK 0x30
>> +
>>   struct qcom_scm {
>>       struct device *dev;
>>       struct clk *core_clk;
>> @@ -419,6 +421,7 @@ static void qcom_scm_set_download_mode(bool enable)
>>   {
>>       bool avail;
>>       int ret = 0;
>> +    u32 val;
>>       avail = __qcom_scm_is_call_available(__scm->dev,
>>                            QCOM_SCM_SVC_BOOT,
>> @@ -426,8 +429,18 @@ static void qcom_scm_set_download_mode(bool enable)
>>       if (avail) {
>>           ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
>>       } else if (__scm->dload_mode_addr) {
>> -        ret = qcom_scm_io_writel(__scm->dload_mode_addr,
>> -                enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
>> +        ret = qcom_scm_io_readl(__scm->dload_mode_addr, &val);
>> +        if (ret) {
>> +            dev_err(__scm->dev,
>> +                "failed to read dload mode address value: %d\n", ret);
>> +            return;
>> +        }
>> +
>> +        val &= ~QCOM_DOWNLOAD_MODE_MASK;
>> +        if (enable)
>> +            val |= QCOM_SCM_BOOT_SET_DLOAD_MODE;
>> +
>> +        ret = qcom_scm_io_writel(__scm->dload_mode_addr, val);
> 
> Any locking for this RMW?

While you ask this, i thought about who all are the user of this 
function. Only, multiple calls to module param callback where
this race could be possible.

I am doubtful, if introducing global mutex lock will be allowed to 
handle this. Any comments.


-Mukesh
> 
>>       } else {
>>           dev_err(__scm->dev,
>>               "No available mechanism for setting download mode\n");
> 

Re: [PATCH v2 1/4] firmware: qcom_scm: Clear download bit during reboot

[Srinivas Kandagatla]

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[] = {

Re: [PATCH v2 2/4] firmware: scm: Modify only the DLOAD bit in TCSR register for download mode

[Srinivas Kandagatla]

On 01/03/2023 09:55, Mukesh Ojha wrote:
> CrashDump collection is based on the DLOAD bit of TCSR register.
> To retain other bits, we read the register and modify only the
> DLOAD bit as the other bits have their own significance.
> 
> Originally-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
> Changes in v2:
>   - Addressed comment made by Bjorn.
>   - Added download mask from patch 3 to this.
> 
>   drivers/firmware/qcom_scm.c | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 51eb853..c9f1fad 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -27,6 +27,8 @@ module_param(download_mode, bool, 0);
>   #define SCM_HAS_IFACE_CLK	BIT(1)
>   #define SCM_HAS_BUS_CLK		BIT(2)
>   
> +#define QCOM_DOWNLOAD_MODE_MASK 0x30
> +
>   struct qcom_scm {
>   	struct device *dev;
>   	struct clk *core_clk;
> @@ -419,6 +421,7 @@ static void qcom_scm_set_download_mode(bool enable)
>   {
>   	bool avail;
>   	int ret = 0;
> +	u32 val;
>   
>   	avail = __qcom_scm_is_call_available(__scm->dev,
>   					     QCOM_SCM_SVC_BOOT,
> @@ -426,8 +429,18 @@ static void qcom_scm_set_download_mode(bool enable)
>   	if (avail) {
>   		ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
>   	} else if (__scm->dload_mode_addr) {
> -		ret = qcom_scm_io_writel(__scm->dload_mode_addr,
> -				enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
> +		ret = qcom_scm_io_readl(__scm->dload_mode_addr, &val);
> +		if (ret) {
> +			dev_err(__scm->dev,
> +				"failed to read dload mode address value: %d\n", ret);
> +			return;
> +		}
> +
> +		val &= ~QCOM_DOWNLOAD_MODE_MASK;
> +		if (enable)
> +			val |= QCOM_SCM_BOOT_SET_DLOAD_MODE;
> +
> +		ret = qcom_scm_io_writel(__scm->dload_mode_addr, val);


This is the second instance of such pattern of usage one is already in 
./drivers/pinctrl/qcom/pinctrl-msm.c

I think it makes sense to move setting fields in register to a dedicated 
function like this:

int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, 
unsigned int val)
{
	unsigned int old, new;
	int ret;

	ret = qcom_scm_io_readl(addr, &old);
	if (ret)
		return ret;

	new = (old & ~mask) | (val << ffs(mask) - 1);

	return qcom_scm_io_writel(addr, new);
}
EXPORT_SYMBOL(qcom_scm_io_update_field);


then we could use it like this:
ret = qcom_scm_io_update_field(__scm->dload_mode_addr, 
QCOM_DOWNLOAD_MODE_MASK, dl_mode)


--srini
>   	} else {
>   		dev_err(__scm->dev,
>   			"No available mechanism for setting download mode\n");

Re: [PATCH v2 2/4] firmware: scm: Modify only the DLOAD bit in TCSR register for download mode

[Mukesh Ojha]

On 3/8/2023 8:36 PM, Srinivas Kandagatla wrote:
> 
> 
> On 01/03/2023 09:55, Mukesh Ojha wrote:
>> CrashDump collection is based on the DLOAD bit of TCSR register.
>> To retain other bits, we read the register and modify only the
>> DLOAD bit as the other bits have their own significance.
>>
>> Originally-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>> Changes in v2:
>>   - Addressed comment made by Bjorn.
>>   - Added download mask from patch 3 to this.
>>
>>   drivers/firmware/qcom_scm.c | 17 +++++++++++++++--
>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index 51eb853..c9f1fad 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -27,6 +27,8 @@ module_param(download_mode, bool, 0);
>>   #define SCM_HAS_IFACE_CLK    BIT(1)
>>   #define SCM_HAS_BUS_CLK        BIT(2)
>> +#define QCOM_DOWNLOAD_MODE_MASK 0x30
>> +
>>   struct qcom_scm {
>>       struct device *dev;
>>       struct clk *core_clk;
>> @@ -419,6 +421,7 @@ static void qcom_scm_set_download_mode(bool enable)
>>   {
>>       bool avail;
>>       int ret = 0;
>> +    u32 val;
>>       avail = __qcom_scm_is_call_available(__scm->dev,
>>                            QCOM_SCM_SVC_BOOT,
>> @@ -426,8 +429,18 @@ static void qcom_scm_set_download_mode(bool enable)
>>       if (avail) {
>>           ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
>>       } else if (__scm->dload_mode_addr) {
>> -        ret = qcom_scm_io_writel(__scm->dload_mode_addr,
>> -                enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
>> +        ret = qcom_scm_io_readl(__scm->dload_mode_addr, &val);
>> +        if (ret) {
>> +            dev_err(__scm->dev,
>> +                "failed to read dload mode address value: %d\n", ret);
>> +            return;
>> +        }
>> +
>> +        val &= ~QCOM_DOWNLOAD_MODE_MASK;
>> +        if (enable)
>> +            val |= QCOM_SCM_BOOT_SET_DLOAD_MODE;
>> +
>> +        ret = qcom_scm_io_writel(__scm->dload_mode_addr, val);
> 
> 
> This is the second instance of such pattern of usage one is already in 
> ./drivers/pinctrl/qcom/pinctrl-msm.c
> 
> I think it makes sense to move setting fields in register to a dedicated 
> function like this:
> 
> int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, 
> unsigned int val)
> {
>      unsigned int old, new;
>      int ret;
> 
>      ret = qcom_scm_io_readl(addr, &old);
>      if (ret)
>          return ret;
> 
>      new = (old & ~mask) | (val << ffs(mask) - 1);
> 
>      return qcom_scm_io_writel(addr, new);
> }
> EXPORT_SYMBOL(qcom_scm_io_update_field);
> 
> 
> then we could use it like this:
> ret = qcom_scm_io_update_field(__scm->dload_mode_addr, 
> QCOM_DOWNLOAD_MODE_MASK, dl_mode)

Thanks for the review,

will do it in v2, will let /drivers/pinctrl/qcom/pinctrl-msm.c uses this.

-Mukesh
> 
> 
> --srini
>>       } else {
>>           dev_err(__scm->dev,
>>               "No available mechanism for setting download mode\n");

Re: [PATCH v2 3/4] firmware: qcom_scm: Refactor code to support multiple download mode

[Dmitry Baryshkov]

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

Re: [PATCH v2 3/4] firmware: qcom_scm: Refactor code to support multiple download mode

[Mukesh Ojha]

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

>