Messages in this thread | | | Date | Wed, 8 Dec 2004 11:53:09 +0200 | From | Pekka Enberg <> | Subject | Re: [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/
| |