Re: [PATCH 1/5] ArmPkg: Tidy GIC code before changes.

[Leif Lindholm <leif.lindholm@linaro.org>]

On Thu, Sep 21, 2017 at 03:34:07PM +0000, Evan Lloyd wrote:
> Hi Leif.
> I agree/accept all the comments, except:
> 
> > -----Original Message-----
> > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > Sent: 14 September 2017 17:41
> > To: Evan Lloyd <Evan.Lloyd@arm.com>
> > Cc: edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> > Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
> > Subject: Re: [PATCH 1/5] ArmPkg: Tidy GIC code before changes.
> > 
> > On Mon, Sep 11, 2017 at 04:23:31PM +0100, evan.lloyd@arm.com wrote:
> > > From: Evan Lloyd <evan.lloyd@arm.com>
> > >
> ...
> > 
> > >    // whereas Affinity3 is defined at [32:39] in MPIDR
> > > -  CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 |
> > > ARM_CORE_AFF2)) | ((MpId & ARM_CORE_AFF3) >> 8);
> > > +  CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 |
> > ARM_CORE_AFF2))
> > > +                | ((MpId & ARM_CORE_AFF3) >> 8);
> > 
> > I can't find an explicit rule on this, but my preference aligns with what
> > examples I can see in the Coding Style: moving that lone '|' to the end of the
> > preceding line:
> > 
> >   CpuAffinity = (MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 |
> > ARM_CORE_AFF2)) |
> >                 ((MpId & ARM_CORE_AFF3) >> 8);
> 
> [[Evan Lloyd]] 5.2.1.6 and 5.7.2.4 have multiple examples of prefix style, 
>   with 5.2.2.11 and 5.7.2.3 providing only 2 instances of a line suffix operator.

The example of 5.2.1.6 already violates the rule of 5.2.2.11.
But the whole problem is that there is no explicit rule about
prefix/suffix, which leads to...

>   I can change it if you insist, but it will be a clear instance of a 
>   maintainer's personal prejudice overriding the coding standard.

If you can point me to a rule I have missed, then yes, you would be
absolutely correct. But I can only find uses, of both variants, in
examples explaining other rules.

If not, I am doing exactly what I am meant to be doing.

Since the maintainer is the one who a) has to review patches without
prior understanding of what the author was thinking and b) the one who
needs to review future modifications to that code, it would be highly
irresponsible to not ask for the code to be presented in the form most
clear to them, where this does not conflict with the coding style.

>   I strongly believe prefix format is much clearer, especially for
>   compound conditions with nesting.

Then why this prefix format not uniformly used in the patch?

Further down, there is
---
-    CpuTarget = MpId & (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2 | ARM_CORE_AFF3);
+    CpuTarget = MpId &
+                (ARM_CORE_AFF0 | ARM_CORE_AFF1 | ARM_CORE_AFF2 | ARM_CORE_AFF3);
---

Now, a) I think that's very clear and b) any other way pushes the
length of the second line over 80 characters. But it does mean you are
not adhering to the rules you are arguing for.

> > 
> > >    if (Revision == ARM_GIC_ARCH_REVISION_3) {
> ...
> > >      // Write set-enable register
> > > -    MmioWrite32 (GicCpuRedistributorBase +
> > ARM_GICR_CTLR_FRAME_SIZE + ARM_GICR_ISENABLER + (4 * RegOffset), 1
> > << RegShift);
> > > +    MmioWrite32 (
> > > +      (GicCpuRedistributorBase
> > > +        + ARM_GICR_CTLR_FRAME_SIZE
> > > +        + ARM_GICR_ISENABLER
> > > +        + (4 * RegOffset)),
> > > +      1 << RegShift
> > > +      );
> > 
> > Maybe rewrite as
> > 
> > #define SET_ENABLE_ADDRESS(base,offset) ((base) +
> > ARM_GICR_CTLR_FRAME_SIZE + \
> >                                          ARM_GICR_ISENABLER + (4 * offset))
> > 
> > (further up)
> > 
> > then
> > 
> >     MmioWrite32 (
> >       SET_ENABLE_ADDRESS (GicCpuRedistributorBase, RegOffset),
> >       1 << RegShift
> >       );
> > 
> > ?
> 
> [[Evan Lloyd]] Agree, but I called the macros ISENABLER_ADDRESS and ISENABLER_ADDRESS
> (using the register names) because SET_ seemed to imply writing something in this context.

Yes, this is much better.

Another reason why I keep banging on about macros - they let you
add descriptive semantics that makes the code easier to understand for
someone unfamiliar with it.

Regards,

Leif