Ira W. Snyder wrote:
> 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.
Have you asked the vendor? Sometimes the guys at the support have good
ideas.
> 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?
Er...I have only followed this thread with one eye....
How do you do the ECHO of tx'ed CAN frames?
But we want so solve the TX problem. You only have to stop the
netif_queue if you TX ring is full. Then you have to deal with calling
netif_wake_queue() eventually. But we obviously don't want to poll.
The solution might be to look at the TX queue in the interrupt handler
and/or the NAPI loop. And now we get back to my first question. If the
controller does a loopback in hardware, i.e. you receive a frame for
each frame send, you don't need to poll the TX queue length.
If you receive a CAN frame it might be on of your's, which means the TX
queue might have at least room for one frame. In pseudo code it might
look like this:
xmit()
{
send_frame_to_hardware();
if (tx_ring_full) {
netif_stop_queue();
priv->flags |= TX_RING_FULL;
}
}
irq() and netif_poll()
{
if (priv->flags & TX_RING_FULL) {
if (room_in_ring()) {
priv->flags &= ~TX_RING_FULL;
netif_wake_queue();
}
}
}
Should be implemented race-free, i.e. you might have to use atomic_*
operations and/or don't use a shared flag variable.
cheers, Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
