netdev - Re: [PATCH 6/7] can: m_can: update to support CAN FD features

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: <20141104071146.GA17367@shlinux1.ap.freescale.net>
Date:	Tue, 4 Nov 2014 15:12:08 +0800
From:	Dong Aisheng <b29396@...escale.com>
To:	Oliver Hartkopp <socketcan@...tkopp.net>
CC:	<linux-can@...r.kernel.org>, <mkl@...gutronix.de>,
	<wg@...ndegger.com>, <varkabhadram@...il.com>,
	<netdev@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 6/7] can: m_can: update to support CAN FD features

On Thu, Oct 30, 2014 at 09:32:49PM +0100, Oliver Hartkopp wrote:
> 
> On 10/30/2014 03:42 AM, Dong Aisheng wrote:
> > On Wed, Oct 29, 2014 at 08:21:28PM +0100, Oliver Hartkopp wrote:
> 
> >> So first I would suggest to check the core release register (CREL) to be
> >> version 3.0.x and quit the driver initialization if it doesn't fit this
> >> version. I would suggest to provide a separate patch for that check. What
> >> about printing the core release version into the kernel log at driver
> >> initialization time?
> >>
> > 
> > One question is that if v3.1.0 and v3.2.0 will be backward compatible with
> > v3.0.1, if yes, how about let the driver still work for them instead of
> > simply quit?
> 
> There are several important differences between 3.0.x and 3.1.x.
> E.g. the CCCR, BTP, PSR and others are changed and a register for the
> transmitter delay compensation is added.
> 
> I assume from 3.1.x to 3.2.x the controller registers will only change in
> small details as the main update will be on the wire and not in the functionality.
> 
> > And then we can add new features according new released IP version.
> 
> Yes. We probably can wait for 3.[12].x to become available before adding the
> special code that behaves according the core release register content.
> 

Okay

> >>> @@ -375,7 +414,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
> >>>  		if (rxfs & RXFS_RFL)
> >>>  			netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
> >>>  
> >>> -		skb = alloc_can_skb(dev, &frame);
> >>> +		skb = alloc_canfd_skb(dev, &frame);
> >>
> >> You are *always* allocating CAN FD frames now?
> >>
> > 
> > Yes, currently it is.
> > The test seemed ok using CAN FD frames even receive normal frame.
> 
> When you put CAN frame content into a CAN FD skb it becomes a CAN FD frame -
> which is wrong.
> 
> CAN 2.0 frame (EDL is clear) -> alloc_can_skb()
> CAN FD frame (EDL is set) -> alloc_canfd_skb()
> 
> You can have a CAN FD frame with a DLC of 8, which does *not* mean that you
> have a CAN 2.0 frame.
> 
> > The issue i know is that candump seemed can not recognize remote frame reported
> > by the driver.
> 
> Do you use the latest candump from
> 

Yes, i'm using latest candump.

> 	https://gitorious.org/linux-can/can-utils/
> ??
> 
> The latest candump switches the CAN_RAW socket into the mode to accept both
> CAN *and* CAN FD frames and displays the frames correctly.
> 
> > Not sure if it's caused by canfd_frame used.
> 
> Yes. CAN FD frames do not have a RTR bit.
> 

You're right.
It's indeed caused by using the CAN FD frames to receive RTR frame.
After switch to normal frame, candump showed it well.

> > Will test and check.
> > 
> >> Depending on RX_BUF_EDL in the RX FIFO message you should create a CAN or CAN
> >> FD frame.
> >>
> >> Of course you can use the struct canfd_frame in m_can_read_fifo() as the
> >> layout of the struct can_frame is identical when filled with 'normal' CAN
> >> frame content.
> >>
> >> But you need to distinguish whether it is a CAN or CAN FD frame when
> >> allocating the skb based on the RX_BUF_EDL value.
> >>
> > 
> > Yes, i think it's good to do that.
> > One obvious benefit is it saves memory at least.
> 
> The main point is that CAN frames and CAN FD frames are separated by this
> (MTU) length information. It's not about saving memory.
> A CAN FD frame with DLC 8 still has 64 data bytes inside it's data structure.
> 

For normal can frame, the CAN_MAX_DLEN is 8 while CANFD_MAX_DLEN is 64.
So i meant using struct canfd_frame to receive normal frame is a bit waste memory.
And besides, actually it's wrong as you already indicated.
I will send out the updated patch with this changed in v2 soon.
Thanks for pointing out this.

Regards
Dong Aisheng

> Regards,
> Oliver
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html