From: Arnd Bergmann <arnd@arndb.de>
To: Chris Metcalf <cmetcalf@tilera.com>
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH 6/6] tilegx network driver: initial support
Date: Mon, 9 Apr 2012 13:49:54 +0000 [thread overview]
Message-ID: <201204091349.54484.arnd@arndb.de> (raw)
In-Reply-To: <201204062101.q36L1uCi011406@farm-0027.internal.tilera.com>
On Friday 06 April 2012, Chris Metcalf wrote:
> This change adds support for the tilegx network driver based on the
> GXIO IORPC support in the tilegx software stack, using the on-chip
> mPIPE packet processing engine.
>
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
> ---
> drivers/net/ethernet/tile/Kconfig | 1 +
> drivers/net/ethernet/tile/Makefile | 4 +-
> drivers/net/ethernet/tile/tilegx.c | 2045 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 2048 insertions(+), 2 deletions(-)
> create mode 100644 drivers/net/ethernet/tile/tilegx.c
I think the directory name should be the company, not the architecture here, so make
it drivers/net/ethernet/tilera/tilegx.c instead.
> +
> +MODULE_AUTHOR("Tilera");
> +MODULE_LICENSE("GPL");
> +
MODULE_AUTHOR is normally a real person with an email address.
> +/* Statistics counters for a specific cpu and device. */
> +struct tile_net_stats_t {
> + u32 rx_packets;
> + u32 rx_bytes;
> + u32 tx_packets;
> + u32 tx_bytes;
> +};
I think you need to drop the _t postfix here, which presumably comes
from converting it from a typedef.
> +
> +/* The actual devices. */
> +static struct net_device *tile_net_devs[TILE_NET_DEVS];
> +
> +/* The device for a given channel. HACK: We use "32", not
> + * TILE_NET_CHANNELS, because it is fairly subtle that the 5 bit
> + * "idesc.channel" field never exceeds TILE_NET_CHANNELS.
> + */
> +static struct net_device *tile_net_devs_for_channel[32];
When you need to keep a list or array of device structures in a driver, you're
usually doing something very wrong. The convention is to just pass the pointer
around to where you need it.
> +
> +/* Convert a "buffer ptr" into a "buffer cpa". */
> +static inline void *buf_to_cpa(void *buf)
> +{
> + return (void *)__pa(buf);
> +}
> +
> +
> +/* Convert a "buffer cpa" into a "buffer ptr". */
> +static inline void *cpa_to_buf(void *cpa)
> +{
> + return (void *)__va(cpa);
> +}
This is almost certainly wrong: The type returned by __pa is a phys_addr_t,
which cannot be dereferenced like a pointer. On normal drivers, you would
use dma_map_single()/dma_unmap_single() to get a token that can get
passed into a dma engine. From what I can tell, this device is directly mapped,
while your PCI uses an IOMMU, so that would require two different
implementations of dma mapping operations.
> +/* Allocate and push a buffer. */
> +static bool tile_net_provide_buffer(bool small)
> +{
> + int stack = small ? small_buffer_stack : large_buffer_stack;
> +
> + /* Buffers must be aligned. */
> + const unsigned long align = 128;
> +
> + /* Note that "dev_alloc_skb()" adds NET_SKB_PAD more bytes,
> + * and also "reserves" that many bytes.
> + */
> + int len = sizeof(struct sk_buff **) + align + (small ? 128 : 1664);
> +
> + /* Allocate (or fail). */
> + struct sk_buff *skb = dev_alloc_skb(len);
> + if (skb == NULL)
> + return false;
> +
> + /* Make room for a back-pointer to 'skb'. */
> + skb_reserve(skb, sizeof(struct sk_buff **));
> +
> + /* Make sure we are aligned. */
> + skb_reserve(skb, -(long)skb->data & (align - 1));
> +
> + /* Save a back-pointer to 'skb'. */
> + *(struct sk_buff **)(skb->data - sizeof(struct sk_buff **)) = skb;
This looks very wrong: why would you put the pointer to the skb into the
skb itself?
> + /* Make sure "skb" and the back-pointer have been flushed. */
> + __insn_mf();
Try to use archicture independent names for flush operations like this
to make it more readable. I assume this should be smp_wmb()?
> +
> + /* Compute the "ip checksum". */
> + jsum = isum_hack + htons(s_len - eh_len) + htons(id);
> + jsum = __insn_v2sadu(jsum, 0);
> + jsum = __insn_v2sadu(jsum, 0);
> + jsum = (0xFFFF ^ jsum);
> + jh->check = jsum;
> +
> + /* Update the tcp "seq". */
> + uh->seq = htonl(seq);
> +
> + /* Update some flags. */
> + if (!final)
> + uh->fin = uh->psh = 0;
> +
> + /* Compute the tcp pseudo-header checksum. */
> + usum = tsum_hack + htons(s_len);
> + usum = __insn_v2sadu(usum, 0);
> + usum = __insn_v2sadu(usum, 0);
> + uh->check = usum;
Why to you open-code the ip checksum functions here? Normally the stack takes
care of this by calling the functions you already provide in
arch/tile/lib/checksum.c
Arnd
next prev parent reply other threads:[~2012-04-09 13:50 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-04-04 20:39 [PATCH 0/6] arch/tile: provide tilegx networking support Chris Metcalf
2012-04-04 20:39 ` [PATCH 1/6] arch/tile: introduce GXIO IORPC framework for tilegx Chris Metcalf
2012-04-04 20:58 ` [PATCH 4/6] arch/tile: common DMA code for the GXIO IORPC subsystem Chris Metcalf
2012-04-06 17:41 ` [PATCH 2/6] arch/tile: fix set_pte() to properly handle kernel MMIO mappings Chris Metcalf
2012-04-06 17:52 ` [PATCH 3/6] arch/tile: support MMIO-based readb/writeb etc Chris Metcalf
2012-04-09 13:24 ` Arnd Bergmann
2012-04-09 20:53 ` Chris Metcalf
2012-04-06 20:38 ` [PATCH 5/6] arch/tile: provide kernel support for the tilegx mPIPE shim Chris Metcalf
2012-04-09 13:34 ` Arnd Bergmann
2012-04-09 21:04 ` Chris Metcalf
2012-04-06 20:42 ` [PATCH 6/6] tilegx network driver: initial support Chris Metcalf
2012-04-09 13:49 ` Arnd Bergmann [this message]
2012-04-09 21:30 ` Chris Metcalf
2012-04-10 10:42 ` Arnd Bergmann
2012-04-12 23:23 ` Chris Metcalf
2012-04-13 10:34 ` Arnd Bergmann
2012-04-28 22:07 ` Chris Metcalf
2012-04-04 20:39 ` [PATCH v2 0/6] arch/tile: networking support for tilegx Chris Metcalf
2012-04-04 20:39 ` [PATCH v2 1/6] arch/tile: introduce GXIO IORPC framework " Chris Metcalf
2012-04-04 20:58 ` [PATCH v2 3/6] arch/tile: common DMA code for the GXIO IORPC subsystem Chris Metcalf
2012-04-06 17:52 ` [PATCH v2 2/6] arch/tile: support MMIO-based readb/writeb etc Chris Metcalf
2012-04-06 20:38 ` [PATCH v2 4/6] arch/tile: provide kernel support for the tilegx mPIPE shim Chris Metcalf
2012-04-06 20:42 ` [PATCH v2 6/6] tilegx network driver: initial support Chris Metcalf
2012-04-30 14:35 ` Arnd Bergmann
2001-09-17 4:00 ` [PATCH v3] " Chris Metcalf
2012-05-03 5:41 ` David Miller
2012-05-03 15:45 ` Chris Metcalf
2012-05-03 17:07 ` David Miller
2012-05-03 17:25 ` Chris Metcalf
2012-05-03 16:41 ` [PATCH v4] " Chris Metcalf
2012-05-04 6:42 ` David Miller
2012-05-09 10:42 ` [PATCH v5] " Chris Metcalf
2012-05-11 13:54 ` Ben Hutchings
2012-05-20 4:42 ` [PATCH v6] " Chris Metcalf
2012-05-20 20:55 ` David Miller
2012-05-23 20:42 ` [PATCH v7] " Chris Metcalf
2012-05-24 4:31 ` David Miller
2012-05-25 14:42 ` [PATCH v8] " Chris Metcalf
2012-06-04 20:12 ` [PATCH v9] " Chris Metcalf
2012-06-06 16:41 ` David Miller
2012-06-06 17:31 ` Eric Dumazet
2012-06-06 17:40 ` Eric Dumazet
2012-06-06 18:36 ` Chris Metcalf
2012-06-06 18:54 ` David Miller
2001-09-17 4:00 ` [PATCH v10] " Chris Metcalf
2012-04-06 20:42 ` Chris Metcalf
2012-06-07 20:39 ` David Miller
2012-06-07 20:44 ` Chris Metcalf
2012-06-07 20:47 ` Chris Metcalf
2012-06-07 20:50 ` Ben Hutchings
2012-06-07 20:52 ` Joe Perches
2012-06-07 20:45 ` Chris Metcalf
2012-06-12 0:03 ` David Miller
2012-06-12 13:14 ` Chris Metcalf
2012-06-06 18:10 ` [PATCH v9] " Eric Dumazet
2012-06-06 18:17 ` David Miller
2012-06-06 18:19 ` Ben Hutchings
2012-05-20 16:35 ` [PATCH v5] " Chris Metcalf
2012-04-28 19:41 ` [PATCH v2 5/6] arch/tile: break out the "csum a long" function to <asm/checksum.h> Chris Metcalf
2012-04-29 11:15 ` [PATCH 6/6] tilegx network driver: initial support Arnd Bergmann
2012-04-15 23:06 ` Chris Metcalf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201204091349.54484.arnd@arndb.de \
--to=arnd@arndb.de \
--cc=cmetcalf@tilera.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).