[PATCH] tilepro ethernet driver: fix a few minor issues

Subsystems: networking drivers, the rest

STALE5253d

9 messages, 4 authors, 2012-04-04 · open the first message on its own page

[PATCH] tilepro ethernet driver: fix a few minor issues

From: Chris Metcalf <hidden>
Date: 2012-03-30 23:25:53

This commit fixes a number of issues seen with the driver:

- Improve handling of return credits to the hardware shim
- Use skb_frag_size() appropriately
- Add netpoll support to run console over UDP

Signed-off-by: Chris Metcalf <redacted>
---
 drivers/net/ethernet/tile/tilepro.c |   75 +++++++++++++++++++++++------------
 1 files changed, 50 insertions(+), 25 deletions(-)
diff --git a/drivers/net/ethernet/tile/tilepro.c b/drivers/net/ethernet/tile/tilepro.c
index d9951af..eb4ea08 100644
--- a/drivers/net/ethernet/tile/tilepro.c
+++ b/drivers/net/ethernet/tile/tilepro.c
@@ -342,6 +342,21 @@ inline int __netio_fastio1(u32 fastio_index, u32 arg0)
 }
 
 
+static void tile_net_return_credit(struct tile_net_cpu *info)
+{
+	struct tile_netio_queue *queue = &info->queue;
+	netio_queue_user_impl_t *qup = &queue->__user_part;
+
+	/* Return four credits after every fourth packet. */
+	if (--qup->__receive_credit_remaining == 0) {
+		u32 interval = qup->__receive_credit_interval;
+		qup->__receive_credit_remaining = interval;
+		__netio_fastio_return_credits(qup->__fastio_index, interval);
+	}
+}
+
+
+
 /*
  * Provide a linux buffer to LIPP.
  */
@@ -864,19 +879,11 @@ static bool tile_net_poll_aux(struct tile_net_cpu *info, int index)
 
 		stats->rx_packets++;
 		stats->rx_bytes += len;
-
-		if (small)
-			info->num_needed_small_buffers++;
-		else
-			info->num_needed_large_buffers++;
 	}
 
-	/* Return four credits after every fourth packet. */
-	if (--qup->__receive_credit_remaining == 0) {
-		u32 interval = qup->__receive_credit_interval;
-		qup->__receive_credit_remaining = interval;
-		__netio_fastio_return_credits(qup->__fastio_index, interval);
-	}
+	/* ISSUE: It would be nice to defer this until the packet has */
+	/* actually been processed. */
+	tile_net_return_credit(info);
 
 	/* Consume this packet. */
 	qup->__packet_receive_read = index2;
@@ -1543,7 +1550,7 @@ static int tile_net_drain_lipp_buffers(struct tile_net_priv *priv)
 
 	/* Drain all the LIPP buffers. */
 	while (true) {
-		int buffer;
+		unsigned int buffer;
 
 		/* NOTE: This should never fail. */
 		if (hv_dev_pread(priv->hv_devhdl, 0, (HV_VirtAddr)&buffer,
@@ -1707,7 +1714,7 @@ static unsigned int tile_net_tx_frags(lepp_frag_t *frags,
 		if (!hash_default) {
 			void *va = pfn_to_kaddr(pfn) + f->page_offset;
 			BUG_ON(PageHighMem(skb_frag_page(f)));
-			finv_buffer_remote(va, f->size, 0);
+			finv_buffer_remote(va, skb_frag_size(f), 0);
 		}
 
 		cpa = ((phys_addr_t)pfn << PAGE_SHIFT) + f->page_offset;
@@ -1735,8 +1742,8 @@ static unsigned int tile_net_tx_frags(lepp_frag_t *frags,
  * Sometimes, if "sendfile()" requires copying, we will be called with
  * "data" containing the header and payload, with "frags" being empty.
  *
- * In theory, "sh->nr_frags" could be 3, but in practice, it seems
- * that this will never actually happen.
+ * Sometimes, for example when using NFS over TCP, a single segment can
+ * span 3 fragments, which must be handled carefully in LEPP.
  *
  * See "emulate_large_send_offload()" for some reference code, which
  * does not handle checksumming.
@@ -1844,10 +1851,8 @@ static int tile_net_tx_tso(struct sk_buff *skb, struct net_device *dev)
 
 	spin_lock_irqsave(&priv->eq_lock, irqflags);
 
-	/*
-	 * Handle completions if needed to make room.
-	 * HACK: Spin until there is sufficient room.
-	 */
+	/* Handle completions if needed to make room. */
+	/* NOTE: Return NETDEV_TX_BUSY if there is still no room. */
 	if (lepp_num_free_comp_slots(eq) == 0) {
 		nolds = tile_net_lepp_grab_comps(eq, olds, wanted, 0);
 		if (nolds == 0) {
@@ -1861,6 +1866,7 @@ busy:
 	cmd_tail = eq->cmd_tail;
 
 	/* Prepare to advance, detecting full queue. */
+	/* NOTE: Return NETDEV_TX_BUSY if the queue is full. */
 	cmd_next = cmd_tail + cmd_size;
 	if (cmd_tail < cmd_head && cmd_next >= cmd_head)
 		goto busy;
@@ -2023,10 +2029,8 @@ static int tile_net_tx(struct sk_buff *skb, struct net_device *dev)
 
 	spin_lock_irqsave(&priv->eq_lock, irqflags);
 
-	/*
-	 * Handle completions if needed to make room.
-	 * HACK: Spin until there is sufficient room.
-	 */
+	/* Handle completions if needed to make room. */
+	/* NOTE: Return NETDEV_TX_BUSY if there is still no room. */
 	if (lepp_num_free_comp_slots(eq) == 0) {
 		nolds = tile_net_lepp_grab_comps(eq, olds, wanted, 0);
 		if (nolds == 0) {
@@ -2040,6 +2044,7 @@ busy:
 	cmd_tail = eq->cmd_tail;
 
 	/* Copy the commands, or fail. */
+	/* NOTE: Return NETDEV_TX_BUSY if the queue is full. */
 	for (i = 0; i < num_frags; i++) {
 
 		/* Prepare to advance, detecting full queue. */
@@ -2260,6 +2265,23 @@ static int tile_net_get_mac(struct net_device *dev)
 	return 0;
 }
 
+
+#ifdef CONFIG_NET_POLL_CONTROLLER
+/*
+ * Polling 'interrupt' - used by things like netconsole to send skbs
+ * without having to re-enable interrupts. It's not called while
+ * the interrupt routine is executing.
+ */
+static void tile_net_netpoll(struct net_device *dev)
+{
+	struct tile_net_priv *priv = netdev_priv(dev);
+	disable_percpu_irq(priv->intr_id);
+	tile_net_handle_ingress_interrupt(priv->intr_id, dev);
+	enable_percpu_irq(priv->intr_id, 0);
+}
+#endif
+
+
 static const struct net_device_ops tile_net_ops = {
 	.ndo_open = tile_net_open,
 	.ndo_stop = tile_net_stop,
@@ -2268,7 +2290,10 @@ static const struct net_device_ops tile_net_ops = {
 	.ndo_get_stats = tile_net_get_stats,
 	.ndo_change_mtu = tile_net_change_mtu,
 	.ndo_tx_timeout = tile_net_tx_timeout,
-	.ndo_set_mac_address = tile_net_set_mac_address
+	.ndo_set_mac_address = tile_net_set_mac_address,
+#ifdef CONFIG_NET_POLL_CONTROLLER
+	.ndo_poll_controller = tile_net_netpoll,
+#endif
 };
 
 
@@ -2408,7 +2433,7 @@ static void tile_net_cleanup(void)
  */
 static int tile_net_init_module(void)
 {
-	pr_info("Tilera IPP Net Driver\n");
+	pr_info("Tilera Network Driver\n");
 
 	tile_net_devs[0] = tile_net_dev_init("xgbe0");
 	tile_net_devs[1] = tile_net_dev_init("xgbe1");
-- 
1.6.5.2

Re: [PATCH] tilepro ethernet driver: fix a few minor issues

From: David Miller <davem@davemloft.net>
Date: 2012-04-03 22:23:43

From: Chris Metcalf <redacted>
Date: Fri, 30 Mar 2012 19:23:35 -0400
This commit fixes a number of issues seen with the driver:

- Improve handling of return credits to the hardware shim
- Use skb_frag_size() appropriately
- Add netpoll support to run console over UDP

Signed-off-by: Chris Metcalf <redacted>
Please do not mix pure bug fixes and new features.

Submit the pure bug fixes separately for 'net' tree submission
and then submit feature additions like netpoll support in
a patch targetting the 'net-next' tree.
+	/* Handle completions if needed to make room. */
+	/* NOTE: Return NETDEV_TX_BUSY if there is still no room. */
Do not format comments like this, use something like this instead:

	/* Handle completions if needed to make room.
	 * NOTE: Return NETDEV_TX_BUSY if there is still no room.
	 */
 	/* Prepare to advance, detecting full queue. */
+	/* NOTE: Return NETDEV_TX_BUSY if the queue is full. */
Similarly, combine them up into a single comment.
+	/* Handle completions if needed to make room. */
+	/* NOTE: Return NETDEV_TX_BUSY if there is still no room. */
Same here.
 	/* Copy the commands, or fail. */
+	/* NOTE: Return NETDEV_TX_BUSY if the queue is full. */
Again.
+#ifdef CONFIG_NET_POLL_CONTROLLER
+/*
+ * Polling 'interrupt' - used by things like netconsole to send skbs
Format the comment:

	/* Like
	 * this.
	 */

not:

	/*
	 * Like
	 * this.
	 */

Re: [PATCH] tilepro ethernet driver: fix a few minor issues

From: Stephen Rothwell <hidden>
Date: 2012-04-03 22:37:50

Hi Dave,

On Tue, 03 Apr 2012 18:23:35 -0400 (EDT) David Miller [off-list ref] wrote:
Format the comment:

	/* Like
	 * this.
	 */

not:

	/*
	 * Like
	 * this.
	 */
People may be confused because Documentation/CodingStyle says to use the
latter ...

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

Re: [PATCH] tilepro ethernet driver: fix a few minor issues

From: David Miller <davem@davemloft.net>
Date: 2012-04-03 22:42:15

From: Stephen Rothwell <redacted>
Date: Wed, 4 Apr 2012 08:37:38 +1000
Hi Dave,

On Tue, 03 Apr 2012 18:23:35 -0400 (EDT) David Miller [off-list ref] wrote:
quoted
Format the comment:

	/* Like
	 * this.
	 */

not:

	/*
	 * Like
	 * this.
	 */
People may be confused because Documentation/CodingStyle says to use the
latter ...
I refuse to be dragged into this conversation, I'm completely aware
of the exact situation and I've stated my peace on this time and time
again.

Re: [PATCH] tilepro ethernet driver: fix a few minor issues

From: Joe Perches <joe@perches.com>
Date: 2012-04-03 23:01:31

On Tue, 2012-04-03 at 18:40 -0400, David Miller wrote:
From: Stephen Rothwell <redacted>
Date: Wed, 4 Apr 2012 08:37:38 +1000
quoted
On Tue, 03 Apr 2012 18:23:35 -0400 (EDT) David Miller [off-list ref] wrote:
quoted
Format the comment:
	/* Like
	 * this.
	 */
not:
	/*
	 * Like
	 * this.
	 */
People may be confused because Documentation/CodingStyle says to use the
latter ...
I refuse to be dragged into this conversation, I'm completely aware
of the exact situation and I've stated my peace on this time and time
again.
I think you'd have less overall upset in yourself and in
other people if you would do more than repeat yourself
on patch submissions via mailing lists.

To my knowledge when the comment block style was added to
CodingStyle in 2006, you didn't mention anything about it.

https://lkml.org/lkml/2006/12/7/41

and I don't see any other mentions from you.

When I suggested adding something specifically for net
and drivers/net, you were at least dispraising.

http://lists.openwall.net/netdev/2012/02/04/59

I think it'd be useful to have this preference you have
for block comment style written out somewhere other than
the email archives.

cheers, Joe

Re: [PATCH] tilepro ethernet driver: fix a few minor issues

From: Chris Metcalf <hidden>
Date: 2012-04-03 23:32:24

On 4/3/2012 7:01 PM, Joe Perches wrote:
On Tue, 2012-04-03 at 18:40 -0400, David Miller wrote:
quoted
From: Stephen Rothwell <redacted>
Date: Wed, 4 Apr 2012 08:37:38 +1000
quoted
On Tue, 03 Apr 2012 18:23:35 -0400 (EDT) David Miller [off-list ref] wrote:
quoted
Format the comment:
	/* Like
	 * this.
	 */
not:
	/*
	 * Like
	 * this.
	 */
People may be confused because Documentation/CodingStyle says to use the
latter ...
I refuse to be dragged into this conversation, I'm completely aware
of the exact situation and I've stated my peace on this time and time
again.
I think you'd have less overall upset in yourself and in
other people if you would do more than repeat yourself
on patch submissions via mailing lists.

To my knowledge when the comment block style was added to
CodingStyle in 2006, you didn't mention anything about it.

https://lkml.org/lkml/2006/12/7/41

and I don't see any other mentions from you.

When I suggested adding something specifically for net
and drivers/net, you were at least dispraising.

http://lists.openwall.net/netdev/2012/02/04/59

I think it'd be useful to have this preference you have
for block comment style written out somewhere other than
the email archives.
Interesting back-story.  Obviously it's the various sub-maintainers'
perquisite to request specific comment style, and if Linus doesn't push
back on the sub-maintainers, that's how it is.

David, I concur that it would be useful to make this canonical somewhere,
since folks who are not aware of your preferences will obviously follow the
CodingStyle document until corrected by you, which seems like a pain for
you.  I suspect a modified version of Joe's original CodingStyle change,
along with a comparable change in checkpatch, is the most "scalable" solution.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] tilepro ethernet driver: fix a few minor issues

From: David Miller <davem@davemloft.net>
Date: 2012-04-03 23:51:08

From: Chris Metcalf <redacted>
Date: Tue, 3 Apr 2012 19:32:20 -0400
David, I concur that it would be useful to make this canonical somewhere,
since folks who are not aware of your preferences will obviously follow the
CodingStyle document until corrected by you, which seems like a pain for
you.  I suspect a modified version of Joe's original CodingStyle change,
along with a comparable change in checkpatch, is the most "scalable" solution.
I'll try to find time to address this.

Re: [PATCH] tilepro ethernet driver: fix a few minor issues

From: Chris Metcalf <hidden>
Date: 2012-04-04 12:18:12

On 4/3/2012 6:23 PM, David Miller wrote:
From: Chris Metcalf <redacted>
Date: Fri, 30 Mar 2012 19:23:35 -0400
quoted
This commit fixes a number of issues seen with the driver:

- Improve handling of return credits to the hardware shim
- Use skb_frag_size() appropriately
- Add netpoll support to run console over UDP

Signed-off-by: Chris Metcalf <redacted>
Please do not mix pure bug fixes and new features.
Fair point.  The (trivial) netpoll change went in in response to an
internal bug saying "hey, console over UDP is broken with the tile driver"
so it seemed like a bug. :-)
Submit the pure bug fixes separately for 'net' tree submission
and then submit feature additions like netpoll support in
a patch targetting the 'net-next' tree.
To use management-speak, there seem to be some "dotted-line reporting"
issues for this code.  It didn't occur to me that you would push it through
the net tree; I assumed I would push it through the tile tree.  (In fact,
shortly before I got your email I had asked Linus to pull it.)  I'm happy
to do it either way; would you prefer to take this stuff for the net trees
going forward?
quoted
+	/* Handle completions if needed to make room. */
+	/* NOTE: Return NETDEV_TX_BUSY if there is still no room. */
Do not format comments like this, use something like this instead:
I will do a follow-up patch to fix up the comment style throughout the
whole driver.

Thanks!

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] tilepro ethernet driver: fix a few minor issues

From: David Miller <davem@davemloft.net>
Date: 2012-04-04 21:44:58

From: Chris Metcalf <redacted>
Date: Wed, 4 Apr 2012 08:18:08 -0400
It didn't occur to me that you would push it through the net tree; I
assumed I would push it through the tile tree.
If it's purely changes inside the networking driver, best to push
it through the networking tree(s).
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help