netdev - Re: [PATCH v2 2/5] 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: <CAMZ6Rq+o6Di8wAQeAB4_yq+jNBoWvGTZ297+5jCc1=KSC9f0EA@mail.gmail.com>
Date:   Mon, 31 Jan 2022 20:04:06 +0900
From:   Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To:     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 v2 2/5] can: rcar_canfd: Add support for r8a779a0 SoC

 Two more comments!

On Mon. 12 Jan 2022 at 01:22, Ulrich Hecht <uli+renesas@...nd.eu> wrote:
> Adds support for the CANFD IP variant in the V3U SoC.
>
> Differences to controllers in other SoCs are limited to an increase in
> the number of channels from two to eight, an absence of dedicated
> registers for "classic" CAN mode, and a number of differences in magic
> numbers (register offsets and layouts).
>
> Inspired by BSP patch by Kazuya Mizuguchi.
>
> Signed-off-by: Ulrich Hecht <uli+renesas@...nd.eu>
> ---
>  drivers/net/can/rcar/rcar_canfd.c | 231 ++++++++++++++++++++----------
>  1 file changed, 153 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index ff9d0f5ae0dd..b1c9870d2a82 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c

...

> @@ -1488,22 +1543,29 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
>  static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
>  {
>         struct net_device_stats *stats = &priv->ndev->stats;
> +       struct rcar_canfd_global *gpriv = priv->gpriv;
>         struct canfd_frame *cf;
>         struct sk_buff *skb;
>         u32 sts = 0, id, dlc;
>         u32 ch = priv->channel;
>         u32 ridx = ch + RCANFD_RFFIFO_IDX;
>
> -       if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> +       if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) ||
> +           gpriv->chip_id == RENESAS_R8A779A0) {
>                 id = rcar_canfd_read(priv->base, RCANFD_F_RFID(ridx));
>                 dlc = rcar_canfd_read(priv->base, RCANFD_F_RFPTR(ridx));
>
>                 sts = rcar_canfd_read(priv->base, RCANFD_F_RFFDSTS(ridx));
> -               if (sts & RCANFD_RFFDSTS_RFFDF)
> -                       skb = alloc_canfd_skb(priv->ndev, &cf);
> -               else
> +               if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> +                       if (sts & RCANFD_RFFDSTS_RFFDF)
> +                               skb = alloc_canfd_skb(priv->ndev, &cf);
> +                       else
> +                               skb = alloc_can_skb(priv->ndev,
> +                                                   (struct can_frame **)&cf);
> +               } else {
>                         skb = alloc_can_skb(priv->ndev,
>                                             (struct can_frame **)&cf);

It seems to me that we can factorize the two alloc_can_skb() calls:

+               if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
+                   sts & RCANFD_RFFDSTS_RFFDF)
+                       skb = alloc_canfd_skb(priv->ndev, &cf);
+               else
+                       skb = alloc_can_skb(priv->ndev, (struct
can_frame **)&cf);

> +               }
>         } else {
>                 id = rcar_canfd_read(priv->base, RCANFD_C_RFID(ridx));
>                 dlc = rcar_canfd_read(priv->base, RCANFD_C_RFPTR(ridx));
> @@ -1541,10 +1603,16 @@ static void rcar_canfd_rx_pkt(struct rcar_canfd_channel *priv)
>                 }
>         } else {
>                 cf->len = can_cc_dlc2len(RCANFD_RFPTR_RFDLC(dlc));
> -               if (id & RCANFD_RFID_RFRTR)
> +               if (id & RCANFD_RFID_RFRTR) {
>                         cf->can_id |= CAN_RTR_FLAG;
> -               else
> -                       rcar_canfd_get_data(priv, cf, RCANFD_C_RFDF(ridx, 0));
> +               } else {
> +                       if (gpriv->chip_id == RENESAS_R8A779A0)
> +                               rcar_canfd_get_data(priv, cf,
> +                                                   RCANFD_F_RFDF(ridx, 0));
> +                       else
> +                               rcar_canfd_get_data(priv, cf,
> +                                                   RCANFD_C_RFDF(ridx, 0));
> +               }

Put the else if on a single line and remove one level of indentation:

+               else if (gpriv->chip_id == RENESAS_R8A779A0)
+                       rcar_canfd_get_data(priv, cf, RCANFD_F_RFDF(ridx, 0));
+               else
+                       rcar_canfd_get_data(priv, cf, RCANFD_C_RFDF(ridx, 0));

Also, a global comment, once you turn IS_V3U to an inline
function, you can use it in place of the many
"gpriv->chip_id == RENESAS_R8A779A0" checks.


Yours sincerely,
Vincent Mailhol