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

Date:   Mon, 31 Jan 2022 10:24:28 +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 v2 2/5] can: rcar_canfd: Add support for r8a779a0 SoC

Thanks for your review.

> On 01/31/2022 3:08 AM Vincent MAILHOL <mailhol.vincent@...adoo.fr> wrote:
> > @@ -1435,13 +1488,15 @@ static netdev_tx_t rcar_canfd_start_xmit(struct sk_buff *skb,
> >
> >         dlc = RCANFD_CFPTR_CFDLC(can_fd_len2dlc(cf->len));
> >
> > -       if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> > +       if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) ||
> > +           gpriv->chip_id == RENESAS_R8A779A0) {
> >                 rcar_canfd_write(priv->base,
> >                                  RCANFD_F_CFID(ch, RCANFD_CFFIFO_IDX), id);
> >                 rcar_canfd_write(priv->base,
> >                                  RCANFD_F_CFPTR(ch, RCANFD_CFFIFO_IDX), dlc);
> >
> > -               if (can_is_canfd_skb(skb)) {
> > +               if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
> > +                   can_is_canfd_skb(skb)) {
> 
> Could you explain why this additional check is needed?
> My understanding is that can_is_canfd_skb(skb) being true implies
> that the CAN_CTRLMODE_FD flag is set.

That might indeed be redundant.

> 
> >                         /* CAN FD frame format */
> >                         sts |= RCANFD_CFFDCSTS_CFFDF;
> >                         if (cf->flags & CANFD_BRS)
> > @@ -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) {
> 
> I guess that this is linked to the above comment. Does the
> R8A779A0 chip support CAN-FD? If yes, why not simply use the
> CAN_CTRLMODE_FD instead of adding this additional check?

The non-V3U Gen3 CAN controllers have two different ways to be driven, depending on whether they are in classic or CAN-FD mode. The V3U controller is driven the CAN-FD way in both modes and thus needs to have this branch taken no matter what mode it is in.

CU
Uli