linux-kernel - Re: [PATCH V2 2/5] can: flexcan: add CAN FD mode support

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, 8 Apr 2019 17:46:02 +0000
From:   Stefan-gabriel Mirea <stefan-gabriel.mirea@....com>
To:     Joakim Zhang <qiangqing.zhang@....com>,
        "linux-can@...r.kernel.org" <linux-can@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 2/5] can: flexcan: add CAN FD mode support

Hello Joakim,

I successfully validated your CAN FD support on an S32V234 based board.
Here are some points which are not very clear to me:

Joakim Zhang <qiangqing.zhang@....com> writes:
> +		priv->write(reg_mcr | FLEXCAN_MCR_FDEN, &regs->mcr);

According to the S32V234 reference manual (not sure if this applies to
i.MX8QM as well), CAN_CTRL1[SMP] cannot be asserted when CAN FD is
enabled. Is it safe to configure the interface with both "fd on" and
"triple-sampling on"? If not, flexcan_open should return an error when
both CAN_CTRLMODE_FD and CAN_CTRLMODE_3_SAMPLES are set in ctrlmode.

Joakim Zhang <qiangqing.zhang@....com> writes:
> +		priv->offload.is_canfd = true;

Shouldn't is_canfd be assigned false in the "else" branch? Otherwise,
is_canfd will stay true after the following sequence:

root@...v234evb:~# ip link set can0 type can bitrate 500000 dbitrate
2000000 fd on
root@...v234evb:~# ip link set can0 up
root@...v234evb:~# ip link set can0 down
root@...v234evb:~# ip link set can0 type can bitrate 500000 fd off
root@...v234evb:~# ip link set can0 up

Moreover, I wonder if we even need is_canfd, because frames should be
fed to the kernel as classic or FD based only on the EDL bit in the C/S
word of the Rx MB, to keep their original format. That is, you may
either read EDL from within a new callback (but I think that
mailbox_read should remain the only one which both locks and unlocks the
mailboxes) or let mailbox_read call alloc_can(fd)_skb itself.

Regards,
Stefan