linux-kernel - Re: [Patch v3 1/2] arm: kernel: Add SMC structure parameter

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: <20170119161658.GC9631@hector.attlocal.net>
Date:   Thu, 19 Jan 2017 10:16:58 -0600
From:   Andy Gross <andy.gross@...aro.org>
To:     Russell King - ARM Linux <linux@...linux.org.uk>
Cc:     lorenzo.pieralisi@....com, linux-arm-msm@...r.kernel.org,
        will.deacon@....com, linux-kernel@...r.kernel.org,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Kevin Hilman <khilman@...libre.com>,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [Patch v3 1/2] arm: kernel: Add SMC structure parameter

On Thu, Jan 19, 2017 at 03:48:18PM +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 19, 2017 at 09:45:06AM -0600, Andy Gross wrote:
> > On Thu, Jan 19, 2017 at 02:40:08PM +0000, Russell King - ARM Linux wrote:
> > > On Wed, Jan 11, 2017 at 04:31:57PM -0600, Andy Gross wrote:
> > > > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> > > > index b5abfda..3e28d08 100644
> > > > --- a/include/linux/arm-smccc.h
> > > > +++ b/include/linux/arm-smccc.h
> > > > @@ -72,19 +72,33 @@ struct arm_smccc_res {
> > > >  };
> > > >  
> > > >  /**
> > > > - * arm_smccc_smc() - make SMC calls
> > > > + * struct arm_smccc_quirk - Contains quirk information
> > > > + * id contains quirk identification
> > > > + * state contains the quirk specific information
> > > 
> > > Given that this is a kerneldoc comment, it should really conform to the
> > > kerneldoc requirements - see Documentation/kernel-doc-nano-HOWTO.txt:
> > > 
> > > /**
> > >  * struct arm_smccc_quirk - Contains quirk information
> > >  * @id: quirk identification
> > >  * @state: the quirk specific information
> > 
> > Ah, I flubbed that.  I'll fix this.
> > 
> > > 
> > > > + */
> > > > +struct arm_smccc_quirk {
> > > > +	int	id;
> > > > +	union {
> > > > +		unsigned long a6;
> > > > +	} state;
> > > > +};
> > > > +
> > > > +/**
> > > > + * __arm_smccc_smc() - make SMC calls
> > > >   * @a0-a7: arguments passed in registers 0 to 7
> > > >   * @res: result values from registers 0 to 3
> > > > + * @quirk: optional quirk structure
> > > >   *
> > > >   * This function is used to make SMC calls following SMC Calling Convention.
> > > >   * The content of the supplied param are copied to registers 0 to 7 prior
> > > >   * to the SMC instruction. The return values are updated with the content
> > > > - * from register 0 to 3 on return from the SMC instruction.
> > > > + * from register 0 to 3 on return from the SMC instruction.  An optional
> > > > + * quirk structure provides vendor specific behavior.
> > > 
> > > It's quite odd to have the result buried in the middle of arguments
> > > passed to a function, but I guess for the sake of simplicity in the
> > > assembly code that's what we need.
> > > 
> > > Also:
> > > 
> > > "@quirk points to an arm_smccc_quirk, or NULL when no quirks are required."
> > 
> > Fixed.
> > 
> > > 
> > > And... should this not be const?  Are we expecting anyone to modify
> > > the quirk structure?
> > 
> > No, unfortunately.  For qcom, we are stuffing the contents of a6 into the
> > structure so the caller can get that information.
> 
> Please add that detail to the kerneldoc comments for the structure
> concerning state.a6.  It's important that such information is not
> lost in the depths of history.

For documentation, should I split out the union separately?  I don't see any
good examples for documenting union members in unions defined inside structures.

Regards,
Andy