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

[Joakim Zhang]

> -----Original Message-----
> From: Stefan-gabriel Mirea
> Sent: 2019年4月9日 15:05
> To: Joakim Zhang <qiangqing.zhang@xxxxxxx>; Marc Kleine-Budde
> <mkl@xxxxxxxxxxxxxx>
> Cc: linux-can@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxx, 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