linux-kernel - Re: [PATCH 5/8] firmware: qcom: scm: Use atomic SCM for cold boot

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160423044651.GC6968@hector.attlocal.net>
Date:	Fri, 22 Apr 2016 23:46:51 -0500
From:	Andy Gross <andy.gross@...aro.org>
To:	Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	Stephen Boyd <sboyd@...eaurora.org>,
	devicetree@...r.kernel.org,
	Bjorn Andersson <bjorn.andersson@...ymobile.com>,
	jilai wang <jilaiw@...eaurora.org>
Subject: Re: [PATCH 5/8] firmware: qcom: scm: Use atomic SCM for cold boot

On Fri, Apr 22, 2016 at 04:50:05PM -0700, Bjorn Andersson wrote:
> On Fri 22 Apr 15:17 PDT 2016, Andy Gross wrote:
> 
> > This patch changes the cold_set_boot_addr function to use atomic SCM
> > calls.  This removes the need for memory allocation and instead places
> > all arguments in registers.
> > 
> > Signed-off-by: Andy Gross <andy.gross@...aro.org>
> > ---
> >  drivers/firmware/qcom_scm-32.c | 40 ++++++++++++++++++++++++++--------------
> >  1 file changed, 26 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
> [..]
> >  /*
> >   * Set the cold/warm boot address for one of the CPU cores.
> >   */
> > -static int qcom_scm_set_boot_addr(u32 addr, int flags)
> > +static int qcom_scm_set_boot_addr(u32 addr, int flags, bool do_atomic)
> >  {
> >  	struct {
> >  		__le32 flags;
> >  		__le32 addr;
> >  	} cmd;
> >  
> > -	cmd.addr = cpu_to_le32(addr);
> > -	cmd.flags = cpu_to_le32(flags);
> > -	return qcom_scm_call(QCOM_SCM_SVC_BOOT, QCOM_SCM_BOOT_ADDR,
> > -			&cmd, sizeof(cmd), NULL, 0);
> > +	if (do_atomic) {
> > +		return qcom_scm_call_atomic(QCOM_SCM_SVC_BOOT,
> > +					    QCOM_SCM_BOOT_ADDR, 2, flags, addr);
> > +	} else {
> > +
> > +		cmd.addr = cpu_to_le32(addr);
> > +		cmd.flags = cpu_to_le32(flags);
> > +
> > +		return qcom_scm_call(QCOM_SCM_SVC_BOOT, QCOM_SCM_BOOT_ADDR,
> > +				     &cmd, sizeof(cmd), NULL, 0);
> > +	}
> 
> I would prefer that you split this into two functions, rather than
> hiding two functions bodies in one function.
> 
> Perhaps qcom_scm_set_boot_addr and qcom_scm_set_boot_addr_atomic?

Fair enough.  It does get a little contrived when you start throwing the extra
options in there.