netdev - 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <DB7PR04MB46185B04DA6F714FF120A3A8E62D0@DB7PR04MB4618.eurprd04.prod.outlook.com>
Date:   Tue, 9 Apr 2019 07:36:29 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Stefan-gabriel Mirea <stefan-gabriel.mirea@....com>,
        Marc Kleine-Budde <mkl@...gutronix.de>
CC:     "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


> -----Original Message-----
> From: Stefan-gabriel Mirea
> Sent: 2019年4月9日 15:05
> To: Joakim Zhang <qiangqing.zhang@....com>; Marc Kleine-Budde
> <mkl@...gutronix.de>
> Cc: 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
> 
> > From: Joakim Zhang
> > Sent: Tuesday, April 9, 2019 5:07 AM
> > Hi Stefan,
> >
> > Thanks for your validation! Could you add your test tag if you can
> > successfully validated?
> 
> Sure, no problem. Please note that I needed to replace "flexcan_read" and
> "flexcan_write" with "priv->read" and "priv->write" respectively, in PATCH V2
> 4/5.

  Hi Stefan,

  I made a mistake here, thank you for pointing out. I will fix it in V3.

> >  [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?
> 
> This is more of a functionality issue. For example, candump from canutils 4.0.6
> never shows CAN FD frames, but should still be able to show the CAN 2.0 ones
> regardless of whether "fd on" was supplied. I suggest the following separation:
> 
> can_rx_offload_offload_one:
> 	struct sk_buff *skb = NULL;
> 	...
> 	bool drop = unlikely(skb_queue_len(&offload->skb_queue) >
> 			     offload->skb_queue_len_max);
> 
> 	if (offload->mailbox_read(offload, drop, &skb, &timestamp, n) && !skb)
> 		offload->dev->stats.rx_dropped++;
> 
> 	if (skb) {
> 		struct can_rx_offload_cb *cb = can_rx_offload_get_cb(skb);
> 
> 		cb->timestamp = timestamp;
> 	}
> 
> 	return skb;
> 
> flexcan_mailbox_read:
> 	...
> 	if (!drop) {
> 		if (reg_ctrl & FLEXCAN_MB_CNT_EDL)
> 			*skb = alloc_canfd_skb(offload->dev, &cf);
> 		else
> 			*skb = alloc_can_skb(offload->dev,
> 					     (struct can_frame **)&cf);
> 	}
> 	if (*skb) {
> 		/* use cf */
> 		...
> 	}
> 	/* mark as read */
> 
> Although not visible in the FlexCAN case, the socket buffers would be also
> freed from inside mailbox_read if errors were encountered after allocation.
  
 This is a good solution, I will add it in V3. Thank you very much.

Best Regards,
Joakim Zhang

> Regards,
> Stefan