Re: [PATCH 2/2] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 05.07.2012 22:58, Ira W. Snyder wrote:

> The Janz VMOD-ICAN3 firmware does not support any sort of TX-done
> notification or interrupt. The driver previously used the hardware
> loopback to attempt to work around this deficiency, but this caused all
> sockets to receive all messages, even if CAN_RAW_RECV_OWN_MSGS is off.
> 
> Using the new function can_cmp_echo_skb(), we can drop the loopback
> messages and return the original skbs. This fixes the issues with
> CAN_RAW_RECV_OWN_MSGS, but leaves the driver vulnerable to a condition
> where it will overwrite the echo skb stack.
> 
> NOT EVEN CLOSE TO Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> ---
> 
> WARNING: BROKEN CODE!
> 
> If a user uses non-loopback mode and sends packets very quickly, the driver
> will stop accepting packets until the bus is re-started.
> 
> I've managed to trigger the BUG printk() in can_put_echo_skb(), since the
> receive queue can fall behind the transmit queue, and I don't know how to
> detect that situation.
> 
> In short, the patch "works", but I'm not happy with it. See my next posting
> for a working patch that takes a different approach.
> 


Hello Ira,

i tried to get behind your driver this weekend.

The first thing:

ICAN3_TX_BUFFERS is 512 !! (whow)

Usually we use the netdev tx queue to pick one or two CAN frames (when there's
a shadow tx register available) but not 512 ...

Please try one or two tx buffers to take advantage of Linux CAN frame
handling. This should also make the things easier in your napi function that
handles the interrupts and incoming DPM messages.

If you only need to check the received CAN frames against 1-2 echo_skbs this
should be easy and fast to do in the napi function.

ndev = alloc_candev(sizeof(*mod), ICAN3_TX_BUFFERS);

is nice with ICAN3_TX_BUFFERS = 1 or 2 ... (preferably 1 for the moment)

E.g. when you detect a CAN frame in the napi function, that is in the
echo_skbs and you consume/"get" these echo_skbs, you may restart the queue
with netif_start_queue(dev) at that point.

Of course this means that there must not be any implementation problem with
the echo mode in the Janz card: If you don't get the frames back the echo_skbs
are not cleared and the tx-queue is not re-started again.

If you get any problems with the Janz card according this topic we might fix
that by adding a timestamp to the echo_skbs to detect timeouts (e.g. to
automatically clear echo_skbs after 200ms/500ms).

Btw. you have an interesting problem to solve :-)

Best regards,
Oliver