1. Patchew
  2. linux
  3. [PATCH v6 0/5] Refactor to support multiple download mode
  4. View patch

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

Mukesh Ojha posted 5 patches 2 years, 10 months ago
[PATCH v6 5/5] firmware: qcom_scm: Add multiple download mode support
Posted by Mukesh Ojha 2 years, 10 months ago 
Currently, scm driver only supports full dump when download
mode is selected. Add support to enable minidump as well as
enable it along with fulldump.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/qcom_scm.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index fa6502a..780e50a 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 {
@@ -1420,13 +1422,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, "full,mini\n");
 	else if (download_mode == QCOM_DOWNLOAD_NODUMP)
 		len = sysfs_emit(buffer, "off\n");
 
@@ -1437,8 +1442,12 @@ static int set_download_mode(const char *val, const struct kernel_param *kp)
 {
 	u32 old = download_mode;
 
-	if (sysfs_streq(val, "full")) {
+	if (sysfs_streq(val, "full,mini") || sysfs_streq(val, "mini,full")) {
+		download_mode = QCOM_DOWNLOAD_BOTHDUMP;
+	} else if (sysfs_streq(val, "full")) {
 		download_mode = QCOM_DOWNLOAD_FULLDUMP;
+	} else if (sysfs_streq(val, "mini")) {
+		download_mode = QCOM_DOWNLOAD_MINIDUMP;
 	} else if (sysfs_streq(val, "off")) {
 		download_mode = QCOM_DOWNLOAD_NODUMP;
 	} else if (kstrtouint(val, 0, &download_mode) ||
@@ -1461,7 +1470,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/full,mini or mini,full and 0/1 for existing users");
 
 static int qcom_scm_probe(struct platform_device *pdev)
 {
-- 
2.7.4
Re: [PATCH v6 5/5] firmware: qcom_scm: Add multiple download mode support
Posted by andy.shevchenko@gmail.com 2 years, 8 months ago 
Wed, Mar 29, 2023 at 01:16:52PM +0530, Mukesh Ojha kirjoitti:
> Currently, scm driver only supports full dump when download
> mode is selected. Add support to enable minidump as well as
> enable it along with fulldump.

...

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

Now order is broken.

>  #define QCOM_DOWNLOAD_NODUMP	0x0

...

> @@ -1420,13 +1422,16 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -

Stray change and ping-pong style at the same time.

...

>  	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, "full,mini\n");

Why not "both" ?

>  	else if (download_mode == QCOM_DOWNLOAD_NODUMP)
>  		len = sysfs_emit(buffer, "off\n");


With an array (for streq_match_string() call suggested earlier) this become as
simple as

	if (mode >= ARRAY_SIZE(...))
		return sysfs_emit("Oh heh!\n");

	return sysfs_emit("%s\n", array[mode]);

...

> -	if (sysfs_streq(val, "full")) {

Why changing this line?

> +	if (sysfs_streq(val, "full,mini") || sysfs_streq(val, "mini,full")) {
> +		download_mode = QCOM_DOWNLOAD_BOTHDUMP;

It's way too hard, esp. taking into account that once user enters wrong order,
user can't simply validate this by reading value back.

Use "both" and that's it.

> +	} else if (sysfs_streq(val, "full")) {
>  		download_mode = QCOM_DOWNLOAD_FULLDUMP;
> +	} else if (sysfs_streq(val, "mini")) {
> +		download_mode = QCOM_DOWNLOAD_MINIDUMP;

...

>  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/full,mini or mini,full and 0/1 for existing users");

You really must be consistent with at least a couple of things:
1) capitalization;
2) indentation.

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v6 5/5] firmware: qcom_scm: Add multiple download mode support
Posted by Bjorn Andersson 2 years, 8 months ago 
On Sat, May 27, 2023 at 01:14:50AM +0300, andy.shevchenko@gmail.com wrote:
> Wed, Mar 29, 2023 at 01:16:52PM +0530, Mukesh Ojha kirjoitti:
> > Currently, scm driver only supports full dump when download
> > mode is selected. Add support to enable minidump as well as
> > enable it along with fulldump.
> 
> ...
> 
> >  #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)
> 
> Now order is broken.
> 
> >  #define QCOM_DOWNLOAD_NODUMP	0x0
> 
> ...
> 
> > @@ -1420,13 +1422,16 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > -
> 
> Stray change and ping-pong style at the same time.
> 
> ...
> 
> >  	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, "full,mini\n");
> 
> Why not "both" ?
> 

"both" isn't very future proof (and I think we've had additional
variations in the past already), so I asked for this form.

Many thanks for your thorough review, Andy!

Regards,
Bjorn

> >  	else if (download_mode == QCOM_DOWNLOAD_NODUMP)
> >  		len = sysfs_emit(buffer, "off\n");
> 
> 
> With an array (for streq_match_string() call suggested earlier) this become as
> simple as
> 
> 	if (mode >= ARRAY_SIZE(...))
> 		return sysfs_emit("Oh heh!\n");
> 
> 	return sysfs_emit("%s\n", array[mode]);
> 
> ...
> 
> > -	if (sysfs_streq(val, "full")) {
> 
> Why changing this line?
> 
> > +	if (sysfs_streq(val, "full,mini") || sysfs_streq(val, "mini,full")) {
> > +		download_mode = QCOM_DOWNLOAD_BOTHDUMP;
> 
> It's way too hard, esp. taking into account that once user enters wrong order,
> user can't simply validate this by reading value back.
> 
> Use "both" and that's it.
> 
> > +	} else if (sysfs_streq(val, "full")) {
> >  		download_mode = QCOM_DOWNLOAD_FULLDUMP;
> > +	} else if (sysfs_streq(val, "mini")) {
> > +		download_mode = QCOM_DOWNLOAD_MINIDUMP;
> 
> ...
> 
> >  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/full,mini or mini,full and 0/1 for existing users");
> 
> You really must be consistent with at least a couple of things:
> 1) capitalization;
> 2) indentation.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>
Re: [PATCH v6 5/5] firmware: qcom_scm: Add multiple download mode support
Posted by Andy Shevchenko 2 years, 8 months ago 
On Sat, May 27, 2023 at 2:32 AM Bjorn Andersson <andersson@kernel.org> wrote:
> On Sat, May 27, 2023 at 01:14:50AM +0300, andy.shevchenko@gmail.com wrote:
> > Wed, Mar 29, 2023 at 01:16:52PM +0530, Mukesh Ojha kirjoitti:

...

> > >     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, "full,mini\n");
> >
> > Why not "both" ?

> "both" isn't very future proof (and I think we've had additional
> variations in the past already), so I asked for this form.

Okay, so this should be the bit flags and we should parse a list of
the values. In that case I may agree with the approach.

> >       if (mode >= ARRAY_SIZE(...))
> >               return sysfs_emit("Oh heh!\n");
> >
> >       return sysfs_emit("%s\n", array[mode]);

...

> > > +   if (sysfs_streq(val, "full,mini") || sysfs_streq(val, "mini,full")) {
> > > +           download_mode = QCOM_DOWNLOAD_BOTHDUMP;
> >
> > It's way too hard, esp. taking into account that once user enters wrong order,
> > user can't simply validate this by reading value back.
> >
> > Use "both" and that's it.
> >
> > > +   } else if (sysfs_streq(val, "full")) {
> > >             download_mode = QCOM_DOWNLOAD_FULLDUMP;
> > > +   } else if (sysfs_streq(val, "mini")) {
> > > +           download_mode = QCOM_DOWNLOAD_MINIDUMP;

As per above.

-- 
With Best Regards,
Andy Shevchenko

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.