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]

Message-ID: <DB7PR04MB46181D2AC261EAB6DEECEFCCE62D0@DB7PR04MB4618.eurprd04.prod.outlook.com>
Date:   Tue, 9 Apr 2019 02:07:20 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Stefan-gabriel Mirea <stefan-gabriel.mirea@....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>,
        Marc Kleine-Budde <mkl@...gutronix.de>
Subject: RE: [PATCH V2 2/5] can: flexcan: add CAN FD mode support


Hi Stefan,

Thanks for your validation! Could you add your test tag if you can successfully validated?

Comments in-lined about your points.

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Stefan-gabriel Mirea
> Sent: 2019年4月9日 1:46
> To: Joakim Zhang <qiangqing.zhang@....com>; linux-can@...r.kernel.org;
> netdev@...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] Yes, you are right. I checked in i.MX8QM reference manual, CAN_CTRLMODE_FD and CAN_CTRLMODE_3_SAMPLES should not set together. I will fix it in V3.

> 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
 
 [Joakim Zhang] Good catch! I will fix it in V3.

> 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.

 [Joakim Zhang] We added can fd support in 4.9 kernel which let mailbox_read call alloc_can(fd)_skb in the past. Now the driver allocate skb before mailbox_read in rx_offload and also read the overflow frames.
              I add the "is_canfd" since I don't want to change the rx_offload framework too much. @mkl@...gutronix.de, could you give some advice, which solution is better?
              
> Regards,
> Stefan