Messages in this thread | | | Date | Thu, 02 Dec 2010 13:12:36 -0800 (PST) | Subject | Re: [PATCH net-next-2.6 v6 04/20] can: EG20T PCH: Add Tx Flow Control | From | David Miller <> |
| |
From: Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com> Date: Tue, 30 Nov 2010 13:18:52 +0900
> Currently, there is no flow control processing. > Thus, Add flow control processing as > when there is no empty of tx buffer, > netif_stop_queue is called. > When there is empty buffer, netif_wake_queue is called. > > Signed-off-by: Tomoya MORINAGA <tomoya-linux@dsn.okisemi.com>
When implementing functionality like this it is better to use other existing well tested network drivers as a guide rather then trying to be unique and clever, as you are doing here.
First of all, your netif_wake_queue() call is racy. Because another thread can be queueing up packets, fill the queue, and execute a stop queue right when you have made the decision to invoke netif_wake_queue().
Second of all, checking the state of the device to determine if a stop queue should be performed has two problems:
1) The test uses a magic constant mask, which is undocumented.
2) It causes the race you have on the wake queue side
Use pure software state to guide your actions, and let the hardware interrupt trigger the wake queue.
Also, you don't implement this as a true ring buffer, you only consider to stop the queue when you hit the last TX object entry. But all the previous slots could be available.
Your head and tail pointer need to be maintained by advancing the head pointer only during pch_xmit(), and advancing the tail pointer only in the NAPI code as you get indications from the hardware.
Then, after the NAPI TX code advances the tail pointer, you see if 1) the queue is stopped and 2) TX space is now available. If both are true you wake the queue.
Use a well tested and mature driver like drivers/net/tg3.c as a guide. Search for netif_tx_{stop,wake}_queue().
| |