Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 26 May 2023 16:36:04 -0700
From:   Bjorn Andersson <andersson@kernel.org>
To:     andy.shevchenko@gmail.com
Cc:     Mukesh Ojha <quic_mojha@quicinc.com>, agross@kernel.org,
        konrad.dybcio@linaro.org, linus.walleij@linaro.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-gpio@vger.kernel.org
Subject: Re: [PATCH v6 5/5] firmware: qcom_scm: Add multiple download mode
 support
Message-ID: <20230526233604.3eiqbfrkecir3ga5@ripper>
References: <1680076012-10785-1-git-send-email-quic_mojha@quicinc.com>
 <1680076012-10785-6-git-send-email-quic_mojha@quicinc.com>
 <ZHEvWpCqg_oyWyZW@surfacebook>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZHEvWpCqg_oyWyZW@surfacebook>
Precedence: bulk

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