Skip to contentSkip to conversation

ATA over Ethernet driver for 2.6.10-rc3-bk11

PATCH In review Ed L Cashin 5 participants 9 messages Dec 17, 2004 – Dec 22, 2004

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ATA over Ethernet driver for 2.6.10-rc3-bk11

I've implemented the second round of changes suggested here for the AoE driver. Here's a list of changes since December 13 when the revised 2.6.9 patch was submitted.

Greg KH suggestions:
* remove stat char device and put status info into sysfs
* split ctl char device into discover and interfaces
* use MODULE_VERSION
* remove unused macros from aoe.h
* make struct tags lowercase

Jens Axboe suggestions:
* use GFP_NOIO in make_request_fn on mempool alloc

Provide support for ATA over Ethernet devices

Documentation/aoe/aoe.txt +75 -0
diff -urNp linux-2.6.10-rc3-bk11/Documentation/aoe/aoe.txt linux-2.6.10-rc3-bk11-aoe/Documentation/aoe/aoe.txt
@@ -0,0 +1,75 @@
+The EtherDrive (R) HOWTO for users of 2.6 kernels is found at ...
+
+  http://www.coraid.com/support/linux/EtherDrive-2.6-HOWTO.html
+
+  It has many tips and hints!
+
+CREATING DEVICE NODES
+
+  Users of udev should find device nodes created automatically.  Two
+  scripts are provided in Documentation/aoe as examples of static
+  device node creation for using the aoe driver.
+
+    rm -rf /dev/etherd
+    sh Documentation/aoe/mkdevs.sh /dev/etherd
+
+  ... or to make just one shelf's worth of block device nodes ...
+
+    sh Documentation/aoe/mkshelf.sh /dev/etherd 0
+
+  There is also an autoload script that shows how to edit
+  /etc/modprobe.conf to ensure that the aoe module is loaded when
+  necessary.
+
+USING DEVICE NODES
+
+  "cat /dev/etherd/err" blocks, waiting for error diagnostic output,
+  like any retransmitted packets.
+
+  "echo eth2 eth4 > /dev/etherd/interfaces" tells the aoe driver to
+  limit ATA over Ethernet traffic to eth2 and eth4.  AoE traffic from
+  untrusted networks should be ignored as a matter of security.
+
+  "echo > /dev/etherd/discover" tells the driver to find out what AoE
+  devices are available.
+
+  The block devices are named like this:
+
+	e{shelf}.{slot}
+	e{shelf}.{slot}p{part}
+
+  ... so that "e0.2" is the third blade from the left (slot 2) in the
+  first shelf (shelf address zero).  That's the whole disk.  The first
+  partition on that disk would be "e0.2p1".
+
+USING SYSFS
+
+  Each aoe block device in /sys/block has the extra attributes of
+  state, mac, and netif.  The state attribute is "up" when the device
+  is ready for I/O and "down" if detected but unusable.  The
+  "down,closewait" state shows that the device is still open and
+  cannot come up again until it has been closed.
+
+  The mac attribute is the ethernet address of the remote AoE device.
+  The netif attribute is the network interface on the localhost
+  through which we are communicating with the remote AoE device.
+
+  There is a script in this directory that formats this information
+  in a convenient way.
+
+  root@makki linux# sh Documentation/aoe/status.sh 
+    device                 mac       netif           state
+      e6.0        0010040010c6        eth0              up
+      e6.1        001004001067        eth0              up
+      e6.2        001004001068        eth0              up
+      e6.3        001004001065        eth0              up
+      e6.4        001004001066        eth0              up
+      e6.5        0010040010c7        eth0              up
+      e6.6        0010040010c8        eth0              up
+      e6.7        0010040010c9        eth0              up
+      e6.8        0010040010ca        eth0              up
+      e6.9        0010040010cb        eth0              up
+      e9.0        001004000020        eth1              up
+      e9.5        001004000025        eth1              up
+      e9.9        001004000029        eth1              up
+
Documentation/aoe/autoload.sh +17 -0
diff -urNp linux-2.6.10-rc3-bk11/Documentation/aoe/autoload.sh linux-2.6.10-rc3-bk11-aoe/Documentation/aoe/autoload.sh
@@ -0,0 +1,17 @@
+#!/bin/sh
+# set aoe to autoload by installing the
+# aliases in /etc/modprobe.conf
+
+f=/etc/modprobe.conf
+
+if test ! -r $f || test ! -w $f; then
+	echo "cannot configure $f for module autoloading" 1>&2
+	exit 1
+fi
+
+grep major-152 $f >/dev/null
+if [ $? = 1 ]; then
+	echo alias block-major-152 aoe >> $f
+	echo alias char-major-152 aoe >> $f
+fi
+
Documentation/aoe/mkdevs.sh +33 -0
diff -urNp linux-2.6.10-rc3-bk11/Documentation/aoe/mkdevs.sh linux-2.6.10-rc3-bk11-aoe/Documentation/aoe/mkdevs.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+n_shelves=10
+
+if test "$#" != "1"; then
+	echo "Usage: sh mkdevs.sh {dir}" 1>&2
+	exit 1
+fi
+dir=$1
+
+MAJOR=152
+
+echo "Creating AoE devnode files in $dir ..."
+
+set -e
+
+mkdir -p $dir
+
+# (Status info is in sysfs.  See status.sh.)
+# rm -f $dir/stat
+# mknod -m 0400 $dir/stat c $MAJOR 1
+rm -f $dir/err
+mknod -m 0400 $dir/err c $MAJOR 2
+rm -f $dir/discover
+mknod -m 0200 $dir/discover c $MAJOR 3
+rm -f $dir/interfaces
+mknod -m 0200 $dir/interfaces c $MAJOR 4
+
+i=0
+while test $i -lt $n_shelves; do
+	sh -xc "sh `dirname $0`/mkshelf.sh $dir $i"
+	i=`expr $i + 1`
+done
Documentation/aoe/mkshelf.sh +23 -0
diff -urNp linux-2.6.10-rc3-bk11/Documentation/aoe/mkshelf.sh linux-2.6.10-rc3-bk11-aoe/Documentation/aoe/mkshelf.sh
@@ -0,0 +1,23 @@
+#! /bin/sh
+
+if test "$#" != "2"; then
+	echo "Usage: sh mkshelf.sh {dir} {shelfaddress}" 1>&2
+	exit 1
+fi
+dir=$1
+shelf=$2
+MAJOR=152
+
+set -e
+
+minor=`echo 10 \* $shelf \* 16 | bc`
+for slot in `seq 0 9`; do
+	for part in `seq 0 15`; do
+		name=e$shelf.$slot
+		test "$part" != "0" && name=${name}p$part
+		rm -f $dir/$name
+		mknod -m 0660 $dir/$name b $MAJOR $minor
+
+		minor=`expr $minor + 1`
+	done
+done
Documentation/aoe/status.sh +15 -0
diff -urNp linux-2.6.10-rc3-bk11/Documentation/aoe/status.sh linux-2.6.10-rc3-bk11-aoe/Documentation/aoe/status.sh
@@ -0,0 +1,15 @@
+# collate and present sysfs information about AoE storage
+
+set -e
+format="%8s\t%12s\t%8s\t%8s\n"
+
+printf "$format" device mac netif state
+
+for d in `ls -d /sys/block/etherd* | grep -v p`; do
+	dev=`echo "$d" | sed 's/.*!//'`
+	printf "$format" \
+		"$dev" \
+		"`cat \"$d/mac\"`" \
+		"`cat \"$d/netif\"`" \
+		"`cat \"$d/state\"`"
+done | sort
MAINTAINERS +6 -0
diff -urNp linux-2.6.10-rc3-bk11/MAINTAINERS linux-2.6.10-rc3-bk11-aoe/MAINTAINERS
@@ -328,6 +328,12 @@ W:	http://sourceforge.net/projects/acpi4
 W:	http://julien.lerouge.free.fr
 S:	Maintained
 
+ATA OVER ETHERNET DRIVER
+P:	Ed L. Cashin
+M:	[email protected]
+W:	http://www.coraid.com/support/linux
+S:	Supported
+
 ATM
 P:	Chas Williams
 M:	[email protected]
drivers/Makefile +1 -0
diff -urNp linux-2.6.10-rc3-bk11/drivers/Makefile linux-2.6.10-rc3-bk11-aoe/drivers/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_DIO)		+= dio/
 obj-$(CONFIG_SBUS)		+= sbus/
 obj-$(CONFIG_ZORRO)		+= zorro/
 obj-$(CONFIG_MAC)		+= macintosh/
+obj-$(CONFIG_ATA_OVER_ETH)	+= block/aoe/
 obj-$(CONFIG_PARIDE) 		+= block/paride/
 obj-$(CONFIG_TC)		+= tc/
 obj-$(CONFIG_USB)		+= usb/
drivers/block/Kconfig +8 -0
diff -urNp linux-2.6.10-rc3-bk11/drivers/block/Kconfig linux-2.6.10-rc3-bk11-aoe/drivers/block/Kconfig
@@ -429,4 +429,12 @@ source "drivers/s390/block/Kconfig"
 
 source "drivers/block/Kconfig.iosched"
 
+config ATA_OVER_ETH
+	tristate "ATA over Ethernet support"
+	depends on NET
+	default m
+	help
+	This driver provides Support for ATA over Ethernet block
+	devices like the Coraid EtherDrive (R) Storage Blade.
+
 endmenu
drivers/block/aoe/Makefile +6 -0
diff -urNp linux-2.6.10-rc3-bk11/drivers/block/aoe/Makefile linux-2.6.10-rc3-bk11-aoe/drivers/block/aoe/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for ATA over Ethernet
+#
+
+obj-$(CONFIG_ATA_OVER_ETH)	+= aoe.o
+aoe-objs := aoeblk.o aoechr.o aoecmd.o aoedev.o aoemain.o aoenet.o
drivers/block/aoe/aoe.h +163 -0

163 lines not on this page · raw message

drivers/block/aoe/aoeblk.c +251 -0

251 lines not on this page · raw message

drivers/block/aoe/aoechr.c +295 -0

295 lines not on this page · raw message

drivers/block/aoe/aoecmd.c +627 -0

627 lines not on this page · raw message

drivers/block/aoe/aoedev.c +194 -0

194 lines not on this page · raw message

drivers/block/aoe/aoemain.c +93 -0

93 lines not on this page · raw message

drivers/block/aoe/aoenet.c +173 -0

173 lines not on this page · raw message

Signature · 2 lines
-- 
  Ed L Cashin <[email protected]>
#1 lore raw
Quoted 11 lines · Ed L Cashin

I've implemented the second round of changes suggested here for the AoE driver. Here's a list of changes since December 13 when the revised 2.6.9 patch was submitted.

Greg KH suggestions:
* remove stat char device and put status info into sysfs
* split ctl char device into discover and interfaces
* use MODULE_VERSION
* remove unused macros from aoe.h
* make struct tags lowercase

Quoted 5 lines

Jens Axboe suggestions:
* use GFP_NOIO in make_request_fn on mempool alloc

Provide support for ATA over Ethernet devices

Looks good. I've added this to my driver bk tree and it will show up in the next -mm release, and I'll forward it on to Linus after 2.6.10 is out.

Oh, I applied the following patch on top of yours, to get rid of the sparse warnings, and get rid of a kmalloc that could have been potentially pretty nasty in your write() function. It saved a bit of code too :)

thanks,

greg k-h

----------

AOE: fix up sparse warnings and get rid of a kmalloc in the aoe driver.

drivers/block/aoe/aoe.h +1 -1
diff -Nru a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
@@ -158,6 +158,6 @@
 void aoenet_exit(void);
 void aoenet_xmit(struct sk_buff *);
 int is_aoe_netif(struct net_device *ifp);
-int set_aoe_iflist(char *str);
+int set_aoe_iflist(const char __user *str, size_t size);
 
 u64 mac_addr(char addr[6]);
drivers/block/aoe/aoeblk.c +2 -2
diff -Nru a/drivers/block/aoe/aoeblk.c b/drivers/block/aoe/aoeblk.c
@@ -170,7 +170,7 @@
 
 	if (cmd == HDIO_GETGEO) {
 		d->geo.start = get_start_sect(inode->i_bdev);
-		if (!copy_to_user((void *) arg, &d->geo, sizeof d->geo))
+		if (!copy_to_user((void __user *) arg, &d->geo, sizeof d->geo))
 			return 0;
 		return -EFAULT;
 	}
@@ -227,7 +227,7 @@
 	
 	printk(KERN_INFO "aoe: %012llx e%lu.%lu v%04x has %llu "
 		"sectors\n", mac_addr(d->addr), d->aoemajor, d->aoeminor,
-		d->fw_ver, d->ssize);
+		d->fw_ver, (long long)d->ssize);
 }
 
 void __exit
drivers/block/aoe/aoechr.c +6 -22
diff -Nru a/drivers/block/aoe/aoechr.c b/drivers/block/aoe/aoechr.c
@@ -51,9 +51,9 @@
 }
 
 static int
-interfaces(char *str)
+interfaces(const char __user *str, size_t size)
 {
-	if (set_aoe_iflist(str)) {
+	if (set_aoe_iflist(str, size)) {
 		printk(KERN_CRIT
 		       "%s: could not set interface list: %s\n",
 		       __FUNCTION__, "too many interfaces");
@@ -135,24 +135,10 @@
 }
 
 static ssize_t
-aoechr_write(struct file *filp, const char *buf, size_t cnt, loff_t *offp)
+aoechr_write(struct file *filp, const char __user *buf, size_t cnt, loff_t *offp)
 {
-	char *str = kcalloc(1, cnt+1, GFP_KERNEL);
-	int ret;
+	int ret = -EINVAL;
 
-	if (!str) {
-		printk(KERN_CRIT "aoe: aoechr_write: cannot allocate memory\n");
-		return -ENOMEM;
-	}
-
-	ret = -EFAULT;
-	if (copy_from_user(str, buf, cnt)) {
-		printk(KERN_INFO "aoe: aoechr_write: copy from user failed\n");
-		goto out;
-	}
-
-	str[cnt] = '\0';
-	ret = -EINVAL;
 	switch ((unsigned long) filp->private_data) {
 	default:
 		printk(KERN_INFO "aoe: aoechr_write: can't write to that file.\n");
@@ -161,13 +147,11 @@
 		ret = discover();
 		break;
 	case MINOR_INTERFACES:
-		ret = interfaces(str);
+		ret = interfaces(buf, cnt);
 		break;
 	}
 	if (ret == 0)
 		ret = cnt;
- out:
-	kfree(str);
 	return ret;
 }
 
@@ -192,7 +176,7 @@
 }
 
 static ssize_t
-aoechr_read(struct file *filp, char *buf, size_t cnt, loff_t *off)
+aoechr_read(struct file *filp, char __user *buf, size_t cnt, loff_t *off)
 {
 	int n;
 	char *mp;
drivers/block/aoe/aoenet.c +7 -5
diff -Nru a/drivers/block/aoe/aoenet.c b/drivers/block/aoe/aoenet.c
@@ -53,14 +53,16 @@
 }
 
 int
-set_aoe_iflist(char *str)
+set_aoe_iflist(const char __user *user_str, size_t size)
 {
-	int len = strlen(str);
-
-	if (len >= IFLISTSZ)
+	if (size >= IFLISTSZ)
 		return -EINVAL;
 
-	strcpy(aoe_iflist, str);
+	if (copy_from_user(aoe_iflist, user_str, size)) {
+		printk(KERN_INFO "aoe: %s: copy from user failed\n", __FUNCTION__);
+		return -EFAULT;
+	}
+	aoe_iflist[size] = 0x00;
 	return 0;
 }
 
#2 lore raw
In reply to Ed L Cashin
Quoted 1 line · Ed L Cashin

+ ETH_P_AOE = 0x88a2,

include/linux/if_ether.h already defines this as ETH_P_EDP2=0x88A2; use that.

drivers/block/aoe/aoecmd.c line 93

Quoted 4 lines
drivers/block/aoe/aoecmd.c +4 -0
+static int
+aoehdr_atainit(struct aoedev *d, struct aoe_hdr *h)
+{
+       u16 type = __constant_cpu_to_be16(ETH_P_AOE);

How about __constant_htons()?
> +aoecmd_cfg(ushort aoemajor, unsigned char aoeminor)

drivers/block/aoe/aoecmd.c line 487

Quoted 6 lines
drivers/block/aoe/aoecmd.c +6 -0
+{
+       struct aoe_hdr *h;
+       struct aoe_cfghdr *ch;
+       struct sk_buff *skb, *sl;
+       struct net_device *ifp;
+       u16 aoe_type = __constant_cpu_to_be16(ETH_P_AOE);

Ditto

Quoted 11 lines
drivers/block/aoe/aoenet.c +11 -0
+enum {
+       IFLISTSZ = 1024,
+};
+
+static char aoe_iflist[IFLISTSZ];
+
+int
+is_aoe_netif(struct net_device *ifp)
+{
+       register char *p, *q;
+       register int len;

drivers/block/aoe/aoenet.c

Quoted 10 lines
drivers/block/aoe/aoenet.c +10 -0
+
+       if (aoe_iflist[0] == '\0')
+               return 1;
+
+       for (p = aoe_iflist; *p; p = q + strspn(q, WHITESPACE)) {
+               q = p + strcspn(p, WHITESPACE);
+               if (q != p)
+                       len = q - p;
+               else
+                       len = strlen(p); /* last token in aoe_iflist

*/

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+               if (strlen(ifp->name) == len && !strncmp(ifp->name, p,

len))

drivers/block/aoe/aoenet.c line 53

Quoted 7 lines
drivers/block/aoe/aoenet.c +7 -0
+                       return 1;
+               if (q == p)
+                       break;
+       }
+
+       return 0;
+}

This is getting called for every skb received. Do you really want to walk a string array looking for "eth0" or "eth1" for every receive packet? Maybe making the locals "register" helps speed things up. :-)

Seriously, can you achieve the same by registering the packet type handler for each dev and let netif_receive_skb() do the check for skb->dev == packet_type->dev?

drivers/block/aoe/aoenet.c line 86

Quoted 8 lines
drivers/block/aoe/aoenet.c +8 -0
+skb_check(struct sk_buff *skb)
+{
+       if (skb_is_nonlinear(skb))
+       if ((skb = skb_share_check(skb, GFP_ATOMIC)))
+       if (skb_linearize(skb, GFP_ATOMIC) < 0) {
+               dev_kfree_skb(skb);
+               return NULL;
+       }

indentation of if's

Quoted 2 lines

+ * (1) i have no idea if this is redundant, but i can't figure why
+ * the ifp is passed in if it is.

This isn't necessary - see deliver_skb()

Quoted 1 line

+ skb->len += ETH_HLEN; /* (2) */

You want to do a skb_push(skb, ETH_HLEN) here instead to keep skb->len and skb->data corresponding (basically to undo the skb->pull() in eth_type_trans()). skb->mac.raw will still be correct.

Quoted 1 line

+ n = __be32_to_cpu(*((u32 *) h->tag));

Use ntohl() variant since this is network header field.

drivers/block/aoe/aoenet.c

Quoted 5 lines
drivers/block/aoe/aoenet.c +5 -0
+       if (h->verfl & AOEFL_ERR) {
+               n = h->err;
+               if (n > NECODES)
+                       n = 0;
+               printk(KERN_CRIT "aoe: aoenet_rcv: error packet from

%d.%d; "

drivers/block/aoe/aoenet.c line 139

Quoted 5 lines
drivers/block/aoe/aoenet.c +5 -0
+                       "ecode=%d '%s'\n",
+                      __be16_to_cpu(*((u16 *) h->major)), h->minor,
+                       h->err, aoe_errlist[n]);
+               goto exit;
+       }

Is there a better way to handle errors than flooding the log?

Quoted 1 line

+ dev_kfree_skb(skb);

kfree_skb() should be enough.

-scott

#3 lore raw
Quoted 6 lines · Scott Feldman
Quoted 1 line · Ed L Cashin

+ ETH_P_AOE = 0x88a2,

include/linux/if_ether.h already defines this as ETH_P_EDP2=0x88A2; use that.

If my aging memory serves me, EDP2 == AOE.

Jeff

#4 lore raw

PATCH 1/1 ETH_P_AOE (was Re: [PATCH] ATA over Ethernet driver for 2.6.10-rc3-bk11)

In reply to Scott Feldman
Quoted 6 lines · Scott Feldman
Quoted 1 line · Ed L Cashin

+ ETH_P_AOE = 0x88a2,

include/linux/if_ether.h already defines this as ETH_P_EDP2=0x88A2; use that.

Ah, that's old. It's not just "EtherDrive protocol" but the ATA over Ethernet protocol.

Here's a patch for 2.6.10-rc3-bk11 that goes on top of the aoe patch in this thread.

drivers/block/aoe/aoe.h +0 -1
diff -urNp linux-2.6.10-rc3-bk11-aoe/drivers/block/aoe/aoe.h linux-2.6.10-rc3-bk11-aoe-2/drivers/block/aoe/aoe.h
@@ -26,7 +26,6 @@ enum {
 	AOECCMD_FSET,
 
 	AOE_HVER = 0x10,
-	ETH_P_AOE = 0x88a2,
 };
 
 struct aoe_hdr {
include/linux/if_ether.h +1 -1
diff -urNp linux-2.6.10-rc3-bk11-aoe/include/linux/if_ether.h linux-2.6.10-rc3-bk11-aoe-2/include/linux/if_ether.h
@@ -69,7 +69,7 @@
 #define ETH_P_ATMFATE	0x8884		/* Frame-based ATM Transport
 					 * over Ethernet
 					 */
-#define ETH_P_EDP2	0x88A2		/* Coraid EDP2			*/
+#define ETH_P_AOE	0x88A2		/* ATA over Ethernet		*/
 
 /*
  *	Non DIX types. Won't clash for 1500 types.
Signature · 2 lines
-- 
  Ed L Cashin <[email protected]>
Applied
#5 lore raw

Re: [PATCH] ETH_P_AOE (was Re: [PATCH] ATA over Ethernet driver for 2.6.10-rc3-bk11)

Quoted 6 lines · Ed L Cashin
Quoted 4 lines · Scott Feldman
Quoted 1 line · Ed L Cashin

+ ETH_P_AOE = 0x88a2,

Quoted 8 lines
Quoted 2 lines

include/linux/if_ether.h already defines this as ETH_P_EDP2=0x88A2; use that.

Ah, that's old. It's not just "EtherDrive protocol" but the ATA over Ethernet protocol.

Here's a patch for 2.6.10-rc3-bk11 that goes on top of the aoe patch in this thread.

Applied to my trees, thanks.

greg k-h

#6 lore raw
In reply to Scott Feldman
Quoted 7 lines · Scott Feldman
Quoted 1 line · Ed L Cashin

+ ETH_P_AOE = 0x88a2,

include/linux/if_ether.h already defines this as ETH_P_EDP2=0x88A2; use that.

Quoted 6 lines

drivers/block/aoe/aoecmd.c line 93

Quoted 4 lines
drivers/block/aoe/aoecmd.c +4 -0
+static int
+aoehdr_atainit(struct aoedev *d, struct aoe_hdr *h)
+{
+       u16 type = __constant_cpu_to_be16(ETH_P_AOE);

How about __constant_htons()?

Hi. Andi Kleene and you both ask why we're using the __cpu_to_be16 kind of byte-swappers.

I think it comes down to a semantics thing, and it's probably controversial. We are using fixed-size integers, so we specify the size of the integers. A short happens to be 16-bits wide on most architectures, but it needn't be, esp. in the future, so it's more clear to say what we mean.

Signature · 1 line
  Ed L Cashin <[email protected]>
#7 lore raw
In reply to Scott Feldman

Scott Feldman <[email protected]> writes:

...

Quoted 33 lines
Quoted 7 lines
drivers/block/aoe/aoenet.c +7 -0
+static char aoe_iflist[IFLISTSZ];
+
+int
+is_aoe_netif(struct net_device *ifp)
+{
+       register char *p, *q;
+       register int len;

drivers/block/aoe/aoenet.c

Quoted 10 lines
drivers/block/aoe/aoenet.c +10 -0
+
+       if (aoe_iflist[0] == '\0')
+               return 1;
+
+       for (p = aoe_iflist; *p; p = q + strspn(q, WHITESPACE)) {
+               q = p + strcspn(p, WHITESPACE);
+               if (q != p)
+                       len = q - p;
+               else
+                       len = strlen(p); /* last token in aoe_iflist

*/

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+               if (strlen(ifp->name) == len && !strncmp(ifp->name, p,

len))

drivers/block/aoe/aoenet.c line 53

Quoted 7 lines
drivers/block/aoe/aoenet.c +7 -0
+                       return 1;
+               if (q == p)
+                       break;
+       }
+
+       return 0;
+}

This is getting called for every skb received. Do you really want to walk a string array looking for "eth0" or "eth1" for every receive packet? Maybe making the locals "register" helps speed things up. :-)

Quoted 3 lines

Seriously, can you achieve the same by registering the packet type handler for each dev and let netif_receive_skb() do the check for skb->dev == packet_type->dev?

We'd like to keep state in the driver in order to be able to control both outgoing and incoming traffic. I'm not sure this check will become a performance problem, but anyone with who can measure it is welcome to speak up. I suspect that optimizing this now would be a case of premature optimization.

Quoted 4 lines
Quoted 2 lines

+ * (1) i have no idea if this is redundant, but i can't figure why
+ * the ifp is passed in if it is.

This isn't necessary - see deliver_skb()

Thanks. That looks like the only place our packet handler is called.

Quoted 5 lines
Quoted 1 line

+ skb->len += ETH_HLEN; /* (2) */

You want to do a skb_push(skb, ETH_HLEN) here instead to keep skb->len and skb->data corresponding (basically to undo the skb->pull() in eth_type_trans()). skb->mac.raw will still be correct.

Thanks.

...

Quoted 5 lines

drivers/block/aoe/aoenet.c

Quoted 5 lines
drivers/block/aoe/aoenet.c +5 -0
+       if (h->verfl & AOEFL_ERR) {
+               n = h->err;
+               if (n > NECODES)
+                       n = 0;
+               printk(KERN_CRIT "aoe: aoenet_rcv: error packet from
Quoted 8 lines

%d.%d; "

drivers/block/aoe/aoenet.c line 139

Quoted 5 lines
drivers/block/aoe/aoenet.c +5 -0
+                       "ecode=%d '%s'\n",
+                      __be16_to_cpu(*((u16 *) h->major)), h->minor,
+                       h->err, aoe_errlist[n]);
+               goto exit;
+       }

Is there a better way to handle errors than flooding the log?

These errors haven't happened much, but if they did, I'd want everybody to know. Would you suggest a per-device counter to limit how many times this message gets printed?

Quoted 3 lines
Quoted 1 line

+ dev_kfree_skb(skb);

kfree_skb() should be enough.

Hmm. It's just a macro for kfree_skb, but I thought that dev_kfree_skb is the more correct one for us to call. I mean, if dev_kfree_skb isn't supposed to be called here, then what's it for?

Signature · 1 line
  Ed L Cashin <[email protected]>
#8 lore raw
Quoted 6 lines · Ed L Cashin
Quoted 1 line · Scott Feldman

Is there a better way to handle errors than flooding the log?

These errors haven't happened much, but if they did, I'd want everybody to know. Would you suggest a per-device counter to limit how many times this message gets printed?

And a sensible KERN_ level like KERN_WARNING. KERN_CRIT is used when something catastrophic just occurred and the box is as good as defunct. Take a look at "net_ratelimit()"

Alan