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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <VI1PR0402MB286355821F5C81122A7C86E2DF2D0@VI1PR0402MB2863.eurprd04.prod.outlook.com>
Date:   Tue, 9 Apr 2019 07:04:53 +0000
From:   Stefan-gabriel Mirea <stefan-gabriel.mirea@....com>
To:     Joakim Zhang <qiangqing.zhang@....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

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

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

Regards,
Stefan