Wolfgang Grandegger wrote:
> Marc Kleine-Budde wrote:
>> 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?
>
> The hardware does it.
>
>> 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.
>
> Cool, that should work. netif_queue_stopped() could be used instead of
> the TX_RING_FULL flag.
right...good point
I should have looked at my own driver :)
If room_in_ring() is cheap, it boils down to:
>> irq() and netif_poll()
>> {
>> if (room_in_ring()) {
>> netif_wake_queue();
>> }
Otherwise:
>> irq() and netif_poll()
>> {
>> if (netif_queue_stopped() && room_in_ring()) {
>> netif_wake_queue();
>> }
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
