diff mbox series

[v6,4/5] firmware: qcom_scm: Refactor code to support multiple download mode

Message ID 1680076012-10785-5-git-send-email-quic_mojha@quicinc.com
State Superseded
Headers show
Series Refactor to support multiple download mode | expand

Commit Message

Mukesh Ojha March 29, 2023, 7:46 a.m. UTC
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 | 60 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 52 insertions(+), 19 deletions(-)

Comments

Andy Shevchenko May 26, 2023, 10:08 p.m. UTC | #1
Wed, Mar 29, 2023 at 01:16:51PM +0530, Mukesh Ojha kirjoitti:
> 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.

...

>  #include <linux/clk.h>
>  #include <linux/reset-controller.h>
>  #include <linux/arm-smccc.h>

> +#include <linux/kstrtox.h>

Can this be located after clk.h which makes (some) order in this block?

...

>  #define QCOM_DOWNLOAD_MODE_MASK 0x30
>  #define QCOM_DOWNLOAD_FULLDUMP	0x1
> +#define QCOM_DOWNLOAD_NODUMP	0x0

Okay, so you start backward ordering.
But see comments to the next patch.

...

>  		ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
> -				QCOM_DOWNLOAD_MODE_MASK,
> -				enable ? QCOM_DOWNLOAD_FULLDUMP : 0);
> +				QCOM_DOWNLOAD_MODE_MASK, download_mode);

Can ping-pong style be avoided? I.e. do the right thing in the previous patch,
so you won't change lines that were introduced just before.

...

>  }
>  
> +

Stray change.

> +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;

You can return directly.

Also, what about download_mode that doesn't fit to the above two?

> +}

...

> +static int set_download_mode(const char *val, const struct kernel_param *kp)
> +{
> +	u32 old = download_mode;
> +
> +	if (sysfs_streq(val, "full")) {
> +		download_mode = QCOM_DOWNLOAD_FULLDUMP;
> +	} else if (sysfs_streq(val, "off")) {
> +		download_mode = QCOM_DOWNLOAD_NODUMP;

NIH sysfs_match_string().

> +	} else if (kstrtouint(val, 0, &download_mode) ||
> +		   !(download_mode == 0 || download_mode == 1)) {
> +		download_mode = old;
> +		pr_err("qcom_scm: unknown download mode: %s\n", val);

> +		return -EINVAL;

Do not shadow the error code from kstrtouint() it can be different to this one.

> +	}
> +
> +	if (__scm)
> +		qcom_scm_set_download_mode(download_mode);
> +
> +	return 0;
> +}

...

Have you updated corresponding documentation about this parameter?
Or there is none?
Mukesh Ojha June 26, 2023, 4:27 p.m. UTC | #2
On 5/27/2023 3:38 AM, andy.shevchenko@gmail.com wrote:
> Wed, Mar 29, 2023 at 01:16:51PM +0530, Mukesh Ojha kirjoitti:
>> 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.
> 
> ...
> 
>>   #include <linux/clk.h>
>>   #include <linux/reset-controller.h>
>>   #include <linux/arm-smccc.h>
> 
>> +#include <linux/kstrtox.h>
> 
> Can this be located after clk.h which makes (some) order in this block?

Sure.

> 
> ...
> 
>>   #define QCOM_DOWNLOAD_MODE_MASK 0x30
>>   #define QCOM_DOWNLOAD_FULLDUMP	0x1
>> +#define QCOM_DOWNLOAD_NODUMP	0x0
> 
> Okay, so you start backward ordering.
> But see comments to the next patch.

Will fix this by doing it in ascending order..


> 
> ...
> 
>>   		ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
>> -				QCOM_DOWNLOAD_MODE_MASK,
>> -				enable ? QCOM_DOWNLOAD_FULLDUMP : 0);
>> +				QCOM_DOWNLOAD_MODE_MASK, download_mode);
> 
> Can ping-pong style be avoided? I.e. do the right thing in the previous patch,
> so you won't change lines that were introduced just before.

If you notice, I have just converted download mode data type from bool
to int in this patch and hence the changing the line here. Last patch 
was about just using the exported API, so i hope you would be fine here.

> 
> ...
> 
>>   }
>>   
>> +
> 
> Stray change.
> 
>> +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;
> 
> You can return directly.

Ok.

>  > Also, what about download_mode that doesn't fit to the above two?

return sysfs_emit(buffer, "unknown\n"); ?

> 
>> +}
> 
> ...
> 
>> +static int set_download_mode(const char *val, const struct kernel_param *kp)
>> +{
>> +	u32 old = download_mode;
>> +
>> +	if (sysfs_streq(val, "full")) {
>> +		download_mode = QCOM_DOWNLOAD_FULLDUMP;
>> +	} else if (sysfs_streq(val, "off")) {
>> +		download_mode = QCOM_DOWNLOAD_NODUMP;
> 
> NIH sysfs_match_string().

NIH ?

My apology, if i did not get this..
Do you want me to use sysfs_match_string()
and how would that help compare to what is present now ?

> 
>> +	} else if (kstrtouint(val, 0, &download_mode) ||
>> +		   !(download_mode == 0 || download_mode == 1)) {
>> +		download_mode = old;
>> +		pr_err("qcom_scm: unknown download mode: %s\n", val);
> 
>> +		return -EINVAL;
> 
> Do not shadow the error code from kstrtouint() it can be different to this one.

Will fix this.

> 
>> +	}
>> +
>> +	if (__scm)
>> +		qcom_scm_set_download_mode(download_mode);
>> +
>> +	return 0;
>> +}
> 
> ...
> 
> Have you updated corresponding documentation about this parameter?
> Or there is none?

There is none as of yet outside this file; should that be good what i 
have added in 5/5..

> 

-Mukesh
Andy Shevchenko June 26, 2023, 5:12 p.m. UTC | #3
On Mon, Jun 26, 2023 at 7:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:
> On 5/27/2023 3:38 AM, andy.shevchenko@gmail.com wrote:
> > Wed, Mar 29, 2023 at 01:16:51PM +0530, Mukesh Ojha kirjoitti:

...

> >>              ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
> >> -                            QCOM_DOWNLOAD_MODE_MASK,
> >> -                            enable ? QCOM_DOWNLOAD_FULLDUMP : 0);
> >> +                            QCOM_DOWNLOAD_MODE_MASK, download_mode);
> >
> > Can ping-pong style be avoided? I.e. do the right thing in the previous patch,
> > so you won't change lines that were introduced just before.
>
> If you notice, I have just converted download mode data type from bool
> to int in this patch and hence the changing the line here. Last patch
> was about just using the exported API, so i hope you would be fine here.

Thank you for elaboration. I'm fine with that.

...

> >  > Also, what about download_mode that doesn't fit to the above two?
>
> return sysfs_emit(buffer, "unknown\n"); ?

For example, yes.

...

> >> +static int set_download_mode(const char *val, const struct kernel_param *kp)
> >> +{
> >> +    u32 old = download_mode;
> >> +
> >> +    if (sysfs_streq(val, "full")) {
> >> +            download_mode = QCOM_DOWNLOAD_FULLDUMP;
> >> +    } else if (sysfs_streq(val, "off")) {
> >> +            download_mode = QCOM_DOWNLOAD_NODUMP;
> >
> > NIH sysfs_match_string().
>
> NIH ?

Not Invented Here

> My apology, if i did not get this..
> Do you want me to use sysfs_match_string()

Yes.

> and how would that help compare to what is present now ?

This will make your ABi gathered in one place (all strings and all
values) and less code will be used esp. if it's going to be expanded
in the future (isn't it in the next patches?).


> >> +    }
> >> +
> >> +    if (__scm)
> >> +            qcom_scm_set_download_mode(download_mode);
> >> +
> >> +    return 0;
> >> +}

...

> > Have you updated corresponding documentation about this parameter?
> > Or there is none?
>
> There is none as of yet outside this file; should that be good what i
> have added in 5/5..

It was a long time ago when I reviewed this. Just a note that all ABI
must be documented, debugfs is highly recommended to be documented.
diff mbox series

Patch

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 8e39b97..fa6502a 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;
 
@@ -452,8 +454,7 @@  static void qcom_scm_set_download_mode(bool enable)
 		ret = __qcom_scm_set_dload_mode(__scm->dev, 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);
+				QCOM_DOWNLOAD_MODE_MASK, download_mode);
 	} else {
 		dev_err(__scm->dev,
 			"No available mechanism for setting download mode\n");
@@ -1419,6 +1420,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 (sysfs_streq(val, "full")) {
+		download_mode = QCOM_DOWNLOAD_FULLDUMP;
+	} else if (sysfs_streq(val, "off")) {
+		download_mode = QCOM_DOWNLOAD_NODUMP;
+	} else if (kstrtouint(val, 0, &download_mode) ||
+		   !(download_mode == 0 || download_mode == 1)) {
+		download_mode = old;
+		pr_err("qcom_scm: unknown download mode: %s\n", val);
+		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 +1556,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 +1569,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[] = {