netdev - Re: [PATCH v3 1/4] can: rcar_canfd: Add support for r8a779a0 SoC

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] [day] [month] [year] [list]

Message-ID: <616372185.25555.1646843159934@webmail.strato.com>
Date:   Wed, 9 Mar 2022 17:25:59 +0100 (CET)
From:   Ulrich Hecht <uli@...nd.eu>
To:     Vincent MAILHOL <mailhol.vincent@...adoo.fr>,
        Ulrich Hecht <uli+renesas@...nd.eu>
Cc:     linux-renesas-soc@...r.kernel.org, netdev@...r.kernel.org,
        davem@...emloft.net, linux-can@...r.kernel.org,
        prabhakar.mahadev-lad.rj@...renesas.com,
        biju.das.jz@...renesas.com, wsa@...nel.org,
        yoshihiro.shimoda.uh@...esas.com, wg@...ndegger.com,
        mkl@...gutronix.de, kuba@...nel.org, socketcan@...tkopp.net,
        geert@...ux-m68k.org, kieran.bingham@...asonboard.com
Subject: Re: [PATCH v3 1/4] can: rcar_canfd: Add support for r8a779a0 SoC

Thanks for your review!

> On 02/14/2022 8:10 AM Vincent MAILHOL <mailhol.vincent@...adoo.fr> wrote:
> > -#define RCANFD_GERFL_ERR(gpriv, x)     ((x) & (RCANFD_GERFL_EEF1 |\
> > -                                       RCANFD_GERFL_EEF0 | RCANFD_GERFL_MES |\
> > -                                       (gpriv->fdmode ?\
> > -                                        RCANFD_GERFL_CMPOF : 0)))
> > +#define RCANFD_GERFL_ERR(x)            ((x) & (reg_v3u(gpriv, RCANFD_GERFL_EEF0_7, \
> > +                                       RCANFD_GERFL_EEF0 | RCANFD_GERFL_EEF1) | \
> > +                                       RCANFD_GERFL_MES | ((gpriv)->fdmode ? \
> > +                                       RCANFD_GERFL_CMPOF : 0)))
> 
> Instead of packing everything on the right, I suggest putting in a bit more air.
> Something like that:
> 
> #define RCANFD_GERFL_ERR(x)                                             \
>         ((x) & (reg_v3u(gpriv, RCANFD_GERFL_EEF0_7,                     \
>                         RCANFD_GERFL_EEF0 | RCANFD_GERFL_EEF1) |        \
>                 RCANFD_GERFL_MES |                                      \
>                 ((gpriv)->fdmode ? RCANFD_GERFL_CMPOF : 0)))
> 
> Same comment applies to other macros.

That does indeed look a lot better.

> >  /* Helper functions */
> > +static inline bool is_v3u(struct rcar_canfd_global *gpriv)
> > +{
> > +       return gpriv->chip_id == RENESAS_R8A779A0;
> > +}
> > +
> > +static inline u32 reg_v3u(struct rcar_canfd_global *gpriv,
> > +                         u32 v3u, u32 not_v3u)
> > +{
> > +       return is_v3u(gpriv) ? v3u : not_v3u;
> > +}
> 
> Nitpick but I would personally prefer if is_v3u() and reg_v3u()
> were declared before the macros in which they are being used.

So would I, but that would require extensive reshuffling of the code, which I think is not worth the effort for such a minor issue.

> > +               if (is_v3u(gpriv)) {
> > +                       cfg = (RCANFD_NCFG_NTSEG1(tseg1) |
> > +                              RCANFD_NCFG_NBRP(brp) |
> > +                              RCANFD_NCFG_NSJW(sjw) |
> > +                              RCANFD_NCFG_NTSEG2(tseg2));
> > +               } else {
> > +                       cfg = (RCANFD_CFG_TSEG1(tseg1) |
> > +                              RCANFD_CFG_BRP(brp) |
> > +                              RCANFD_CFG_SJW(sjw) |
> > +                              RCANFD_CFG_TSEG2(tseg2));
> > +               }
> 
> Nitpick: can't you use one of your reg_v3u() functions here?
> |        cfg = reg_v3u(gpriv, ..., ...)?

Technically yes, but I think of reg_v3u() as choosing between two register layouts, and this is something else.

CU
Uli