Re: [PATCH net-next-2.6 v6 04/20] can: EG20T PCH: Add Tx Flow Control

[David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>]

From: Tomoya MORINAGA <tomoya-linux-ECg8zkTtlr0C6LszWs/t0g@public.gmane.org>
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-ECg8zkTtlr0C6LszWs/t0g@public.gmane.org>

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().