[PATCH v4 0/5] Refactor to support multiple download mode

[PATCH v4 0/5] Refactor to support multiple download mode

[Mukesh Ojha]

Intention of this series to support multiple download mode and
only modify the required bits during setting tcsr register.

Other download modes are minidump, bothdump (full dump + minidump).

Latest minidump kernel driver patches has been sent here
https://lore.kernel.org/lkml/1679491817-2498-1-git-send-email-quic_mojha@quicinc.com/

Also, this series should be applied on
https://lore.kernel.org/lkml/1678979666-551-1-git-send-email-quic_mojha@quicinc.com/

Changes in v4:
  - val should be shifted within the function [srinivas.kandagatla]
    i.e new = (old & ~mask) | (val << ffs(mask) - 1);
  - Added Acked-by [linus.walleij] on pinctrl change.

Changes in v3 : https://lore.kernel.org/lkml/1679070482-8391-1-git-send-email-quic_mojha@quicinc.com/
 - Removed [1] from the series and sent as a separate patch[2], although this series
   should be applied on top [2].
  [1] https://lore.kernel.org/lkml/1677664555-30191-2-git-send-email-quic_mojha@quicinc.com/
  [2] https://lore.kernel.org/lkml/1678979666-551-1-git-send-email-quic_mojha@quicinc.com/
 - Introduce new exported symbol on suggestion from [srinivas.kandagatla]
 - Use the symbol from drivers/pinctrl/qcom/pinctrl-msm.c.
 - Addressed comment given by [dmitry.baryshkov]
 - Converted non-standard Originally-by to Signed-off-by.

Changes in v2: https://lore.kernel.org/lkml/1677664555-30191-1-git-send-email-quic_mojha@quicinc.com/
 - Addressed comment made by [bjorn]
 - Added download mask.
 - 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.


Mukesh Ojha (5):
  firmware: qcom_scm: provide a read-modify-write function
  pinctrl: qcom: Use qcom_scm_io_update_field()
  firmware: scm: Modify only the download bits in TCSR register
  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            | 89 +++++++++++++++++++++++++++++++---
 drivers/pinctrl/qcom/pinctrl-msm.c     | 11 ++---
 include/linux/firmware/qcom/qcom_scm.h |  2 +
 4 files changed, 87 insertions(+), 26 deletions(-)

-- 
2.7.4

[PATCH v4 1/5] firmware: qcom_scm: provide a read-modify-write function

[Mukesh Ojha]

It was realized by Srinivas K. that there is a need of
read-modify-write scm exported function so that it can
be used by multiple clients.

Let's introduce qcom_scm_io_update_field() which masks
out the bits and write the passed value to that
bit-offset. Subsequent patch will use this function.

Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/qcom_scm.c            | 15 +++++++++++++++
 include/linux/firmware/qcom/qcom_scm.h |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 5f281cb..431fe1f 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -407,6 +407,21 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
 }
 EXPORT_SYMBOL(qcom_scm_set_remote_state);
 
+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);
+
 static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
 {
 	struct qcom_scm_desc desc = {
diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
index 1e449a5..203a781 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -84,6 +84,8 @@ extern bool qcom_scm_pas_supported(u32 peripheral);
 
 extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
 extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
+extern int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask,
+				    unsigned int val);
 
 extern bool qcom_scm_restore_sec_cfg_available(void);
 extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
-- 
2.7.4

[PATCH v4 2/5] pinctrl: qcom: Use qcom_scm_io_update_field()

[Mukesh Ojha]

Use qcom_scm_io_update_field() exported function introduced
in last commit.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index daeb79a..6e1a5e4 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1016,6 +1016,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	unsigned long flags;
 	bool was_enabled;
 	u32 val;
+	u32 mask;
 
 	if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
 		set_bit(d->hwirq, pctrl->dual_edge_irqs);
@@ -1049,23 +1050,19 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	 * With intr_target_use_scm interrupts are routed to
 	 * application cpu using scm calls.
 	 */
+	mask = (7 << g->intr_target_bit);
 	if (pctrl->intr_target_use_scm) {
 		u32 addr = pctrl->phys_base[0] + g->intr_target_reg;
 		int ret;
 
-		qcom_scm_io_readl(addr, &val);
-
-		val &= ~(7 << g->intr_target_bit);
-		val |= g->intr_target_kpss_val << g->intr_target_bit;
-
-		ret = qcom_scm_io_writel(addr, val);
+		ret = qcom_scm_io_update_field(addr, mask, g->intr_target_kpss_val);
 		if (ret)
 			dev_err(pctrl->dev,
 				"Failed routing %lu interrupt to Apps proc",
 				d->hwirq);
 	} else {
 		val = msm_readl_intr_target(pctrl, g);
-		val &= ~(7 << g->intr_target_bit);
+		val &= ~mask;
 		val |= g->intr_target_kpss_val << g->intr_target_bit;
 		msm_writel_intr_target(val, pctrl, g);
 	}
-- 
2.7.4

[PATCH v4 3/5] firmware: scm: Modify only the download bits in TCSR register

[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.

Signed-off-by: Poovendhan Selvaraj <quic_poovendh@quicinc.com>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/qcom_scm.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 431fe1f..3c6c5e7 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -30,6 +30,9 @@ 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
+#define QCOM_DOWNLOAD_FULLDUMP	0x1
+
 struct qcom_scm {
 	struct device *dev;
 	struct clk *core_clk;
@@ -448,8 +451,9 @@ 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_update_field(__scm->dload_mode_addr,
+				QCOM_DOWNLOAD_MODE_MASK,
+				enable ? QCOM_DOWNLOAD_FULLDUMP : 0);
 	} else {
 		dev_err(__scm->dev,
 			"No available mechanism for setting download mode\n");
-- 
2.7.4

[PATCH v4 4/5] 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>
---
 drivers/firmware/Kconfig    | 11 ---------
 drivers/firmware/qcom_scm.c | 59 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 52 insertions(+), 18 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 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[] = {
-- 
2.7.4

[PATCH v4 5/5] 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>
---
 drivers/firmware/qcom_scm.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 0c94429..19315d0 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -32,6 +32,8 @@ static u32 download_mode;
 
 #define QCOM_DOWNLOAD_MODE_MASK 0x30
 #define QCOM_DOWNLOAD_FULLDUMP	0x1
+#define QCOM_DOWNLOAD_MINIDUMP  0x2
+#define QCOM_DOWNLOAD_BOTHDUMP	(QCOM_DOWNLOAD_FULLDUMP | QCOM_DOWNLOAD_MINIDUMP)
 #define QCOM_DOWNLOAD_NODUMP	0x0
 
 struct qcom_scm {
@@ -1421,13 +1423,16 @@ 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_MINIDUMP)
+		len = sysfs_emit(buffer, "mini\n");
+	else if (download_mode == QCOM_DOWNLOAD_BOTHDUMP)
+		len = sysfs_emit(buffer, "both\n");
 	else if (download_mode == QCOM_DOWNLOAD_NODUMP)
 		len = sysfs_emit(buffer, "off\n");
 
@@ -1440,6 +1445,10 @@ static int set_download_mode(const char *val, const struct kernel_param *kp)
 
 	if (!strncmp(val, "full", strlen("full"))) {
 		download_mode = QCOM_DOWNLOAD_FULLDUMP;
+	} else if (!strncmp(val, "mini", strlen("mini"))) {
+		download_mode = QCOM_DOWNLOAD_MINIDUMP;
+	} else if (!strncmp(val, "both", strlen("both"))) {
+		download_mode = QCOM_DOWNLOAD_BOTHDUMP;
 	} else if (!strncmp(val, "off", strlen("off"))) {
 		download_mode = QCOM_DOWNLOAD_NODUMP;
 	} else if (kstrtouint(val, 0, &download_mode) ||
@@ -1462,7 +1471,7 @@ static const struct kernel_param_ops download_mode_param_ops = {
 
 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");
+		 "download mode: off/full/mini/both(full+mini) or 0/1 for existing users");
 
 static int qcom_scm_probe(struct platform_device *pdev)
 {
-- 
2.7.4

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

[Bjorn Andersson]

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

Re: [PATCH v4 5/5] firmware: qcom_scm: Add multiple download mode support

[Bjorn Andersson]

On Mon, Mar 27, 2023 at 10:11:21PM +0530, Mukesh Ojha wrote:
> 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>
> ---
>  drivers/firmware/qcom_scm.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 0c94429..19315d0 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -32,6 +32,8 @@ static u32 download_mode;
>  
>  #define QCOM_DOWNLOAD_MODE_MASK 0x30
>  #define QCOM_DOWNLOAD_FULLDUMP	0x1
> +#define QCOM_DOWNLOAD_MINIDUMP  0x2
> +#define QCOM_DOWNLOAD_BOTHDUMP	(QCOM_DOWNLOAD_FULLDUMP | QCOM_DOWNLOAD_MINIDUMP)
>  #define QCOM_DOWNLOAD_NODUMP	0x0
>  
>  struct qcom_scm {
> @@ -1421,13 +1423,16 @@ 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_MINIDUMP)
> +		len = sysfs_emit(buffer, "mini\n");
> +	else if (download_mode == QCOM_DOWNLOAD_BOTHDUMP)
> +		len = sysfs_emit(buffer, "both\n");
>  	else if (download_mode == QCOM_DOWNLOAD_NODUMP)
>  		len = sysfs_emit(buffer, "off\n");
>  
> @@ -1440,6 +1445,10 @@ static int set_download_mode(const char *val, const struct kernel_param *kp)
>  
>  	if (!strncmp(val, "full", strlen("full"))) {
>  		download_mode = QCOM_DOWNLOAD_FULLDUMP;
> +	} else if (!strncmp(val, "mini", strlen("mini"))) {
> +		download_mode = QCOM_DOWNLOAD_MINIDUMP;
> +	} else if (!strncmp(val, "both", strlen("both"))) {

"both" isn't very future proof...

How about allowing mini,full? You don't need to do string tokenizing
etc, just strcmp mini,full (and full,mini if you want to be fancy)...

Regards,
Bjorn

> +		download_mode = QCOM_DOWNLOAD_BOTHDUMP;
>  	} else if (!strncmp(val, "off", strlen("off"))) {
>  		download_mode = QCOM_DOWNLOAD_NODUMP;
>  	} else if (kstrtouint(val, 0, &download_mode) ||
> @@ -1462,7 +1471,7 @@ static const struct kernel_param_ops download_mode_param_ops = {
>  
>  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");
> +		 "download mode: off/full/mini/both(full+mini) or 0/1 for existing users");
>  
>  static int qcom_scm_probe(struct platform_device *pdev)
>  {
> -- 
> 2.7.4
> 

Re: [PATCH v4 1/5] firmware: qcom_scm: provide a read-modify-write function

[kernel test robot]

Hi Mukesh,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20230327]
[also build test WARNING on v6.3-rc4]
[cannot apply to linusw-pinctrl/devel linusw-pinctrl/for-next linus/master v6.3-rc4 v6.3-rc3 v6.3-rc2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mukesh-Ojha/firmware-qcom_scm-provide-a-read-modify-write-function/20230328-004405
patch link:    https://lore.kernel.org/r/1679935281-18445-2-git-send-email-quic_mojha%40quicinc.com
patch subject: [PATCH v4 1/5] firmware: qcom_scm: provide a read-modify-write function
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20230328/202303280535.acb66sQT-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/47c3bb52fbb758e2238a7ab29c00e3188afe9754
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Mukesh-Ojha/firmware-qcom_scm-provide-a-read-modify-write-function/20230328-004405
        git checkout 47c3bb52fbb758e2238a7ab29c00e3188afe9754
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/firmware/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303280535.acb66sQT-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/firmware/qcom_scm.c: In function 'qcom_scm_io_update_field':
>> drivers/firmware/qcom_scm.c:419:49: warning: suggest parentheses around '-' inside '<<' [-Wparentheses]
     419 |         new = (old & ~mask) | (val << ffs(mask) - 1);
         |                                       ~~~~~~~~~~^~~


vim +419 drivers/firmware/qcom_scm.c

   409	
   410	int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, unsigned int val)
   411	{
   412		unsigned int old, new;
   413		int ret;
   414	
   415		ret = qcom_scm_io_readl(addr, &old);
   416		if (ret)
   417			return ret;
   418	
 > 419		new = (old & ~mask) | (val << ffs(mask) - 1);
   420	
   421		return qcom_scm_io_writel(addr, new);
   422	}
   423	EXPORT_SYMBOL(qcom_scm_io_update_field);
   424	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[Mukesh Ojha]

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

Re: [PATCH v4 5/5] firmware: qcom_scm: Add multiple download mode support

[Mukesh Ojha]

On 3/27/2023 11:57 PM, Bjorn Andersson wrote:
> On Mon, Mar 27, 2023 at 10:11:21PM +0530, Mukesh Ojha wrote:
>> 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>
>> ---
>>   drivers/firmware/qcom_scm.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index 0c94429..19315d0 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -32,6 +32,8 @@ static u32 download_mode;
>>   
>>   #define QCOM_DOWNLOAD_MODE_MASK 0x30
>>   #define QCOM_DOWNLOAD_FULLDUMP	0x1
>> +#define QCOM_DOWNLOAD_MINIDUMP  0x2
>> +#define QCOM_DOWNLOAD_BOTHDUMP	(QCOM_DOWNLOAD_FULLDUMP | QCOM_DOWNLOAD_MINIDUMP)
>>   #define QCOM_DOWNLOAD_NODUMP	0x0
>>   
>>   struct qcom_scm {
>> @@ -1421,13 +1423,16 @@ 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_MINIDUMP)
>> +		len = sysfs_emit(buffer, "mini\n");
>> +	else if (download_mode == QCOM_DOWNLOAD_BOTHDUMP)
>> +		len = sysfs_emit(buffer, "both\n");
>>   	else if (download_mode == QCOM_DOWNLOAD_NODUMP)
>>   		len = sysfs_emit(buffer, "off\n");
>>   
>> @@ -1440,6 +1445,10 @@ static int set_download_mode(const char *val, const struct kernel_param *kp)
>>   
>>   	if (!strncmp(val, "full", strlen("full"))) {
>>   		download_mode = QCOM_DOWNLOAD_FULLDUMP;
>> +	} else if (!strncmp(val, "mini", strlen("mini"))) {
>> +		download_mode = QCOM_DOWNLOAD_MINIDUMP;
>> +	} else if (!strncmp(val, "both", strlen("both"))) {
> 
> "both" isn't very future proof...
> 
> How about allowing mini,full? You don't need to do string tokenizing
> etc, just strcmp mini,full (and full,mini if you want to be fancy)...
> 

Thanks for the suggestion, this looks good.
I have applied the changes.

-- Mukesh

> Regards,
> Bjorn
> 
>> +		download_mode = QCOM_DOWNLOAD_BOTHDUMP;
>>   	} else if (!strncmp(val, "off", strlen("off"))) {
>>   		download_mode = QCOM_DOWNLOAD_NODUMP;
>>   	} else if (kstrtouint(val, 0, &download_mode) ||
>> @@ -1462,7 +1471,7 @@ static const struct kernel_param_ops download_mode_param_ops = {
>>   
>>   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");
>> +		 "download mode: off/full/mini/both(full+mini) or 0/1 for existing users");
>>   
>>   static int qcom_scm_probe(struct platform_device *pdev)
>>   {
>> -- 
>> 2.7.4
>>

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

[Dmitry Baryshkov]

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

-- 
With best wishes
Dmitry

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

[Mukesh Ojha]

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
>