On Tue, Feb 23, 2010 at 08:41:24PM +0100, Oliver Hartkopp wrote:
> Ira W. Snyder wrote:
> > On Mon, Feb 22, 2010 at 08:42:56PM +0100, Wolfgang Grandegger wrote:
>
> >>
> >> - Don't include sja1000/sja1000.h. It's only for drivers in sja1000.
> >> I know that some other drivers use SJA1000 definitions as well, but
> >> that requires a general solution.
> >>
> >
> > Why not? I need some of the definitions for the SJA1000 error registers.
> > Is there any reason why it can't be include/linux/can/sja1000.h instead?
> >
> > It seems stupid to duplicate the register definitions in each new driver
> > that comes along.
>
> Register definitions are always hardware specific.
>
> The fact that the Janz card has some registers in SJA1000 layout does not
> justify to push sja1000.h to a public place like include/linux/can/* IMO.
>
> The ESD USB and the EMS USB drivers also have some registers in SJA1000
> layout. I would prefer following their approach.
>
> >
> >> - About xmit flow control. What happens if you send messages quickly at
> >> 125 KB/s. You could use "cangen -g 0 can0" for that test. How many
> >> messages get dropped?
> >>
> >
> > I let the cangen command run for a while:
> > $ ifconfig -a
> > can0 Link encap:UNSPEC HWaddr
> > 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
> > UP RUNNING NOARP MTU:16 Metric:1
> > RX packets:473455 errors:0 dropped:0 overruns:0 frame:0
> > TX packets:473455 errors:0 dropped:1831983 overruns:0 carrier:0
>
> Oops!
>
> This looks weird in janz_xmit() !
>
> + /* check that we can actually transmit */
> + if (!(desc.control & DESC_VALID)) {
> + dev_err(mod->dev, "%s: no buffers\n", __func__);
> + stats->tx_dropped++;
> + kfree_skb(skb);
> + goto out_unlock;
> + }
>
> You should enable/wake the tx_queue only when your hardware is potentially
> able to process new CAN frames. You are getting lot's of trashed frames with
> your implementation.
>
Yep. I know how a network driver is supposed to work. Unfortunately,
AFAICT from the datasheet, this hardware has no notification mechanism
for completed packets.
The only way I can think of to track TX ring usage is to have the TX
thread keep an index of where the last used packet was, and then check
to see how many are left in the ring, for each TX.
That text description is hard to understand, I mean something like this:
start up (first TX packet)
index buffer state
=======================================
0 empty
1 empty
last_processed 2 empty
3 full (waiting to be sent by firmware)
4 full (waiting to be sent by firmware)
fasttx_num 5 empty (this is where the next packet will go)
6 empty
7 empty
8 empty
So, when we call xmit, we first start at the last_processed index, and
iterate until we find a packet waiting to be sent by the firmware. Then
we do the correct subtraction to know if we have free space left.
Of course, we still must poll to call netif_wake_queue(). :( This is a
pretty horrible solution. Thoughts?
Ira
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
