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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20101202.131236.28815617.davem@davemloft.net>
Date:	Thu, 02 Dec 2010 13:12:36 -0800 (PST)
From:	David Miller <davem@...emloft.net>
To:	tomoya-linux@....okisemi.com
Cc:	wg@...ndegger.com, w.sang@...gutronix.de, chripell@...e.org,
	21cnbao@...il.com, sameo@...ux.intel.com,
	socketcan-core@...ts.berlios.de, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, qi.wang@...el.com,
	yong.y.wang@...el.com, andrew.chih.howe.khor@...el.com,
	joel.clark@...el.com, kok.howg.ewe@...el.com,
	margie.foster@...el.com
Subject: Re: [PATCH net-next-2.6 v6 04/20] can: EG20T PCH: Add Tx Flow
 Control

From: Tomoya MORINAGA <tomoya-linux@....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@....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().

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/