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

[Bjorn Andersson]

On Sat, May 27, 2023 at 01:14:50AM +0300, andy.shevchenko@xxxxxxxxx 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
> 
>