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 19:46:59 +0900
From:   Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To:     Ulrich Hecht <uli@...nd.eu>
Cc:     Ulrich Hecht <uli+renesas@...nd.eu>,
        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

On Mon. 31 Jan 2022 at 18:24, Ulrich Hecht <uli@...nd.eu> wrote:
>
> 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.

Ack.
Makes sense. So actually, this isn’t related to the previous comments :)

In my previous message, I added two comments toward the macro. I
just want to double check that you have seen these because they
are missing from your reply.