> -----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, ×tamp, 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
