Re: [Minios-devel] [UNIKRAFT PATCHv6 02/23] plat/include: Use macro-ed helper to simplify ioreg_read/write for Arm64

["Justin He (Arm Technology China)" <Justin.He@xxxxxxx>]

Hi Sharan
Our outlook server converted all  "tab" to "spaces" in my patches.
I will dig into this issue and find how to avoid this.
Sorry about that

Cheers,
Justin (Jia He)

> -----Original Message-----
> From: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> Sent: 2019年2月27日 0:05
> To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx; Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> Cc: Florian Schmidt <florian.schmidt@xxxxxxxxx>; Felipe Huici
> <felipe.huici@xxxxxxxxx>; Julien Grall <Julien.Grall@xxxxxxx>;
> yuri.volchkov@xxxxxxxxx; Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>;
> Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx>
> Subject: Re: [UNIKRAFT PATCHv6 02/23] plat/include: Use macro-ed helper to
> simplify ioreg_read/write for Arm64
>
> Hello Justin He,
>
> I downloaded the patch series from the patchwork server
> (https://patchwork.unikraft.org/) series 438. I could see space instead
> of tab in the patch series. This is also true of the patch "clean up
> kernel image symbols".
>
> Thanks & Regards
> Sharan
>
> On 2/26/19 3:07 AM, Justin He (Arm Technology China) wrote:
> > Hi Sharan
> > Sorry, it is a little bit strange to me. I can't reproduce to get your 
> > checkpatch
> warning locally ☹
> > Could you please tell how do you" git am" the patch set?
> >
> > Here is my result, although there are some false positive error/warnings you
> once mentioned.
> > # ./support/scripts/checkpatch.pl patch_v6/0002-plat-include-Use-macro-ed-
> helper-to-simplify-ioreg_r.patch
> > ERROR: need consistent spacing around '*' (ctx:WxV)
> > #69: FILE: plat/common/include/arm/arm64/cpu.h:40:
> > +       ioreg_read##bits(const volatile uint##bits##_t *addr) \
> >                                                         ^
> >
> > WARNING: Use of volatile is usually wrong: see
> Documentation/process/volatile-considered-harmful.rst
> > #69: FILE: plat/common/include/arm/arm64/cpu.h:40:
> > +       ioreg_read##bits(const volatile uint##bits##_t *addr) \
> >
> > ERROR: need consistent spacing around '*' (ctx:WxV)
> > #74: FILE: plat/common/include/arm/arm64/cpu.h:45:
> > +       ioreg_write##bits(volatile uint##bits##_t *addr, \
> >                                                    ^
> >
> > WARNING: Use of volatile is usually wrong: see
> Documentation/process/volatile-considered-harmful.rst
> > #74: FILE: plat/common/include/arm/arm64/cpu.h:45:
> > +       ioreg_write##bits(volatile uint##bits##_t *addr, \
> >
> > ERROR: Macros with complex values should be enclosed in parentheses
> > #79: FILE: plat/common/include/arm/arm64/cpu.h:50:
> > +#define __IOREG_READ_ALL() __IOREG_READ(8)  \
> > +                          __IOREG_READ(16) \
> > +                          __IOREG_READ(32) \
> > +                          __IOREG_READ(64) \
> > +
> >
> > ERROR: Macros with complex values should be enclosed in parentheses
> > #84: FILE: plat/common/include/arm/arm64/cpu.h:55:
> > +#define __IOREG_WRITE_ALL()    __IOREG_WRITE(8)  \
> > +                          __IOREG_WRITE(16) \
> > +                          __IOREG_WRITE(32) \
> > +                          __IOREG_WRITE(64) \
> > +
> >
> > total: 4 errors, 2 warnings, 70 lines checked
> >
> > NOTE: For some of the reported defects, checkpatch may be able to
> >        mechanically convert to the typical style using --fix or 
> > --fix-inplace.
> >
> > patch_v6/0002-plat-include-Use-macro-ed-helper-to-simplify-ioreg_r.patch
> has style problems, please review.
> >
> > NOTE: Ignored message types: ASSIGN_IN_IF NEW_TYPEDEFS
> >
> > NOTE: If any of the errors are false positives, please report
> >        them to the maintainer, see CHECKPATCH in MAINTAINERS.
> >
> > In the other way, you can also get the whole patch series by
> > git clone https://github.com/hejianet/unikraft_arm64.git -b 1stset_v6
> >
> >> -----Original Message-----
> >> From: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> >> Sent: 2019年2月25日 20:39
> >> To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> >> devel@xxxxxxxxxxxxxxxxxxxx; Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> >> Cc: Florian Schmidt <florian.schmidt@xxxxxxxxx>; Felipe Huici
> >> <felipe.huici@xxxxxxxxx>; Julien Grall <Julien.Grall@xxxxxxx>;
> >> yuri.volchkov@xxxxxxxxx; Kaly Xin (Arm Technology China)
> <Kaly.Xin@xxxxxxx>;
> >> Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx>
> >> Subject: Re: [UNIKRAFT PATCHv6 02/23] plat/include: Use macro-ed helper
> to
> >> simplify ioreg_read/write for Arm64
> >>
> >> Hello Jia He,
> >>
> >> We have introduced some checkpatch errors/warning in this patch series.
> >> We have replaced all the tabs with spaces.
> >>
> >> Please find warning/errors inline.
> >>
> >> Thanks & Regards
> >> Sharan
> >>
> >> On 2/22/19 7:21 AM, Jia He wrote:
> >>> From: Wei Chen <wei.chen@xxxxxxx>
> >>> As discussed in mailing list [1], we can use macro-ed helpers to avoid
> >>> having to write 4 times of the same things of ioreg_read_8/16/32/64 and
> >>> ioreg_write8/16/32/64.
> >>>
> >>> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> >>> Signed-off-by: Jia He <justin.he@xxxxxxx>
> >>> Reviewed-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> >>> ---
> >>>    plat/common/include/arm/arm64/cpu.h | 64 +++++++++++------------------
> >>>    1 file changed, 25 insertions(+), 39 deletions(-)
> >>>
> >>> diff --git a/plat/common/include/arm/arm64/cpu.h
> >> b/plat/common/include/arm/arm64/cpu.h
> >>> index 503bc68..c009fc8 100644
> >>> --- a/plat/common/include/arm/arm64/cpu.h
> >>> +++ b/plat/common/include/arm/arm64/cpu.h
> >>> @@ -34,45 +34,31 @@
> >>>
> >>>    #include <inttypes.h>
> >>>
> >>> -static inline uint8_t ioreg_read8(const volatile uint8_t *addr)
> >>> -{
> >>> -       return *addr;
> >>> -}
> >>> -
> >>> -static inline void ioreg_write8(volatile uint8_t *addr, uint8_t value)
> >>> -{
> >>> -       *addr = value;
> >>> -}
> >>> -
> >>> -static inline uint16_t ioreg_read16(const volatile uint16_t *addr)
> >>> -{
> >>> -       return *addr;
> >>> -}
> >>> -
> >>> -static inline void ioreg_write16(volatile uint16_t *addr, uint16_t value)
> >>> -{
> >>> -       *addr = value;
> >>> -}
> >>> -
> >>> -static inline uint32_t ioreg_read32(const volatile uint32_t *addr)
> >>> -{
> >>> -       return *addr;
> >>> -}
> >>> -
> >>> -static inline void ioreg_write32(volatile uint32_t *addr, uint32_t value)
> >>> -{
> >>> -       *addr = value;
> >>> -}
> >>> -
> >>> -static inline uint64_t ioreg_read64(const volatile uint64_t *addr)
> >>> -{
> >>> -       return *addr;
> >>> -}
> >>> -
> >>> -static inline void ioreg_write64(volatile uint64_t *addr, uint64_t value)
> >>> -{
> >>> -       *addr = value;
> >>> -}
> >>
> >>
> >> ERROR: code indent should use tabs where possible
> >> #66: FILE: plat/common/include/arm/arm64/cpu.h:41:
> >> +               { return *addr; }$
> >>> +/* Define macros to access IO registers */
> >>> +#define __IOREG_READ(bits) \
> >>> +static inline uint##bits##_t \
> >>> +       ioreg_read##bits(const volatile uint##bits##_t *addr) \
> >>> +               { return *addr; }
> >>> +
> >>> +#define __IOREG_WRITE(bits) \
> >>> +static inline void \
> >>> +       ioreg_write##bits(volatile uint##bits##_t *addr, \
> >>
> >> ERROR: code indent should use tabs where possible
> >>> +                                               uint##bits##_t value) \
> >>
> >> ERROR: code indent should use tabs where possible
> >>> +               { *addr = value; }
> >>> +
> >>> +
> >>
> >> ERROR: code indent should use tabs where possible
> >>> +#define __IOREG_READ_ALL() __IOREG_READ(8)  \
> >>> +                          __IOREG_READ(16) \
> >>> +                          __IOREG_READ(32) \
> >>> +                          __IOREG_READ(64) \
> >>> +
> >>> +#define __IOREG_WRITE_ALL()    __IOREG_WRITE(8)  \
> >>> +                          __IOREG_WRITE(16) \
> >>> +                          __IOREG_WRITE(32) \
> >>> +                          __IOREG_WRITE(64) \
> >>> +
> >>> +__IOREG_READ_ALL()
> >>> +__IOREG_WRITE_ALL()
> >>>
> >>>    static inline void _init_cpufeatures(void)
> >>>    {
> >>> --
> >>> 2.17.1
> >>>
> >>> IMPORTANT NOTICE: The contents of this email and any attachments are
> >> confidential and may also be privileged. If you are not the intended 
> >> recipient,
> >> please notify the sender immediately and do not disclose the contents to 
> >> any
> >> other person, use it for any purpose, or store or copy the information in 
> >> any
> >> medium. Thank you.
> >>>
> > IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended 
> recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
> >
IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel