Re: at91 driver lost CAN messages

[Alexander Stein <alexander.stein@systec-electronic.com>]

On Thursday 16 March 2017 11:18:33, Marc Kleine-Budde wrote:
> On 03/16/2017 10:58 AM, Alexander Stein wrote:
> >>> Let's see if I get time to cook rx-offload on a USB driver.
> >> 
> >> That would be highly appreciated :-)
> > 
> > Mh, at a first glance I don't know how to do that, actually. AFAICS
> > rx-offload does not support 'pushing' struct can_frame into the
> > queue, only pulling from rx-offload. There is no fixed connection
> > bewteen the internal can device structure and CAN frames, like
> > hardware message boxes. In order to use
> > can_rx_offload_irq_offload_fifo() to insert (mostly) a single
> > can_frame I would need to store the last urb. I think I need to add a
> > function similar to can_rx_offload_irq_queue_err_skb(), e.g.
> > can_rx_offload_irq_queue_skb, but without scheduling. This should be
> > done once the urb was completly read.
> 
> Does your device support multiple CAN frames per USB?

Yes, it does. Although this will happen rarely actually. As several URBs are 
submitted at once, each URB should only deliver a single CAN frame as this is 
much faster. So in order to receive more CAN frames per URB the USB host (or 
driver) needs to be delayed for long time so the CAN frames queue up on the 
device itself.

> Maybe rename
> 
> > int can_rx_offload_irq_queue_err_skb(struct can_rx_offload *offload,
> > struct sk_buff *skb)
> into can_rx_offload_irq_queue_skb() or
> 
> can_rx_offload_irq_queue_skb_schedule() and fix the race condition:
> > int can_rx_offload_irq_queue_skb(struct can_rx_offload *offload, struct
> > sk_buff *skb) {
> 
> take spinlock
> 
> >         if (skb_queue_len(&offload->skb_queue) >
> >         
> >             offload->skb_queue_len_max)
> 
> release in case of error
> 
> >                 return -ENOMEM;
> >         
> >         skb_queue_tail(&offload->skb_queue, skb);
> 
> use unlocked variant of skb_queue_tail, i.e. __skb_queue_tail()
> 
> release spin lock
> 
> >         can_rx_offload_schedule(offload);
> >         
> >         return 0;
> > 
> > }
> 
> Then factor all but the can_rx_offload_schedule(offload) call into a
> separate function.

I will think about this. I'm afraid but currently I can't do anything about 
that. But this will only work for (USB) CAN drivers though. Ethernet USB 
drivers also suffer from the out-of-order issue as they also push their SKB in 
IRQ context. I've seen reorder warnings in wireshark on a direct link iperf 
transfer.

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.stein@systec-electronic.com

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-0
Fax:    +49 (0) 3765 38600-4100
 
Managing Directors:
	Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
	Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
	Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010