| Message ID | 201203302325.q2UNPm8j012403@farm-0012.internal.tilera.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Chris Metcalf <cmetcalf@tilera.com> 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 <cmetcalf@tilera.com> 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. */ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dave, On Tue, 03 Apr 2012 18:23:35 -0400 (EDT) David Miller <davem@davemloft.net> wrote: > > Format the comment: > > /* Like > * this. > */ > > not: > > /* > * Like > * this. > */ People may be confused because Documentation/CodingStyle says to use the latter ...
From: Stephen Rothwell <sfr@canb.auug.org.au> Date: Wed, 4 Apr 2012 08:37:38 +1000 > Hi Dave, > > On Tue, 03 Apr 2012 18:23:35 -0400 (EDT) David Miller <davem@davemloft.net> wrote: >> >> 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2012-04-03 at 18:40 -0400, David Miller wrote: > From: Stephen Rothwell <sfr@canb.auug.org.au> > Date: Wed, 4 Apr 2012 08:37:38 +1000 > > On Tue, 03 Apr 2012 18:23:35 -0400 (EDT) David Miller <davem@davemloft.net> wrote: > >> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/3/2012 7:01 PM, Joe Perches wrote: > On Tue, 2012-04-03 at 18:40 -0400, David Miller wrote: >> From: Stephen Rothwell <sfr@canb.auug.org.au> >> Date: Wed, 4 Apr 2012 08:37:38 +1000 >>> On Tue, 03 Apr 2012 18:23:35 -0400 (EDT) David Miller <davem@davemloft.net> wrote: >>>> 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.
From: Chris Metcalf <cmetcalf@tilera.com> 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/3/2012 6:23 PM, David Miller wrote: > From: Chris Metcalf <cmetcalf@tilera.com> > 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 <cmetcalf@tilera.com> > 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? >> + /* 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!
From: Chris Metcalf <cmetcalf@tilera.com> 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). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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");
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 <cmetcalf@tilera.com> --- drivers/net/ethernet/tile/tilepro.c | 75 +++++++++++++++++++++++------------ 1 files changed, 50 insertions(+), 25 deletions(-)