RE: [PATCH V2 2/5] can: flexcan: add CAN FD mode support

[Stefan-gabriel Mirea <stefan-gabriel.mirea@nxp.com>]

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