I agree with all the below and will address in v5.
Mikey
On Tue, 2019-05-28 at 23:28 -0700, Christoph Hellwig wrote:
> > +config PPC_DAWR
> > + bool
> > + default n
>
> "default n" is the default default. No need to write this line.
>
> > +++ b/arch/powerpc/kernel/dawr.c
> > @@ -0,0 +1,100 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +//
> > +// DAWR infrastructure
> > +//
> > +// Copyright 2019, Michael Neuling, IBM Corporation.
>
> Normal top of file header should be /* */, //-style comments are only
> for the actual SPDX heder line.
>
> > + /* Send error to user if they hypervisor won't allow us to write DAWR */
> > + if ((!dawr_force_enable) &&
> > + (firmware_has_feature(FW_FEATURE_LPAR)) &&
> > + (set_dawr(&null_brk) != H_SUCCESS))
>
> None of the three inner brace sets here are required, and the code
> becomes much easier to read without them.
>
> > + return -1;
>
> What about returning a proper error code?
>
> > +static int __init dawr_force_setup(void)
> > +{
> > + dawr_force_enable = false;
>
> This variable already is initialized to alse by default, so this line
> is not required.
>
> > + if (PVR_VER(mfspr(SPRN_PVR)) == PVR_POWER9) {
> > + /* Turn DAWR off by default, but allow admin to turn it on */
> > + dawr_force_enable = false;
>
> .. and neither is this one.
>
