lkml.org 
[lkml]   [2004]   [Dec]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] ATA over Ethernet driver for 2.6.9
Hi Ed,

Few comments below.

On Mon, 06 Dec 2004 10:51:46 -0500, Ed L Cashin <ecashin@coraid.com> wrote:
> diff -urpN linux-2.6.9/drivers/block/aoe/all.h linux-2.6.9-aoe/drivers/block/aoe/all.h
> --- linux-2.6.9/drivers/block/aoe/all.h 1969-12-31 19:00:00.000000000 -0500
> +++ linux-2.6.9-aoe/drivers/block/aoe/all.h 2004-12-06 10:40:00.000000000 -0500

How about calling this aoe.h or similar?

> @@ -0,0 +1,156 @@
> +#include <linux/ctype.h>
> +#include <linux/string.h>
> +#include <linux/hdreg.h>
> +#include <linux/blkdev.h>
> +#include <linux/types.h>
> +#include <linux/fs.h>
> +#include <linux/module.h>
> +#include <linux/blkpg.h>
> +#include <linux/slab.h>
> +#include <linux/skbuff.h>
> +#include <linux/ioctl.h>
> +#include <linux/if.h>
> +#include <linux/netdevice.h>
> +#include <linux/kdev_t.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/poll.h>
> +#include <linux/timer.h>
> +#include <linux/genhd.h>
> +#include <asm/uaccess.h>
> +#include <asm/namei.h>
> +#include <asm/semaphore.h>

This looks wrong. I don't see you using, for example, semaphores
everywhere you include this header. Please just move these includes to
the appropriate .c files.

> +enum {
> + DEVFL_UP = 1, /* device is installed in system and ready for AoE->ATA commands */
> + DEVFL_TKILL = (1<<1), /* flag for timer to know when to kill self */
> + DEVFL_EXT = (1<<2), /* device accepts lba48 commands */
> + DEVFL_CLOSEWAIT = (1<<3), /* device is waiting for all closes to revalidate */
> + DEVFL_WC_UPDATE = (1<<4), /* this device needs to update write cache status */
> + DEVFL_WORKON = (1<<4),
> +
> + BUFFL_FAIL = 1,
> +
> + MAXATADATA = 1024,
> + NPERSHELF = 10,
> + FREETAG = -1,
> +};

Perhaps it would be cleaner if you kept DEVFL flags in separate enum
block as they're related and have nothing to do with the rest?

> +typedef struct Buf Buf;
> +struct Buf {

Please don't introduce typedefs for structs. Furthermore, please make
the struct name start with a lower case letter like the rest of the
kernel.

> diff -urpN linux-2.6.9/drivers/block/aoe/aoeblk.c linux-2.6.9-aoe/drivers/block/aoe/aoeblk.c
> --- linux-2.6.9/drivers/block/aoe/aoeblk.c 1969-12-31 19:00:00.000000000 -0500
> +++ linux-2.6.9-aoe/drivers/block/aoe/aoeblk.c 2004-12-06 10:40:00.000000000 -0500
> +static int
> +aoeblk_open(struct inode *inode, struct file *filp)
> +{
> + Aoedev *d;
> + ulong flags;
> +
> + d = (Aoedev *) inode->i_bdev->bd_disk->private_data;

Please remove the redundant casts when converting from a void pointer.
The conversion is guaranteed by the C standard.

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

\
 
 \ /
  Last update: 2005-03-22 14:08    [from the cache]
©2003-2011 Jasper Spaans