Re: [PATCH] ATA over Ethernet driver for 2.6.9

[Arjan van de Ven]

> +#include "u.h"

can you give this header a more human friendly name ?

> +#include "if_aoe.h"
> +
> +#define VER 2
> +#define nil NULL

please just use NULL

> +#define nelem(A) (sizeof (A) / sizeof (A)[0])
> +#define AOE_MAJOR 152
> +#define ROOT_PATH "/dev/etherd/"

ehhhh device drivers really should be agnostic to device node paths!

> +typedef struct Bufq Bufq;
> +struct Bufq {
> +	Buf *head, *tail;
> +};

is there a good reason to not use list.h lists ??

> +struct Aoedev {
> +	Aoedev *next;
> +	uchar addr[6];		/* remote mac addr */
> +	ushort flags;
> +	ulong sysminor;
> +	ulong aoemajor;
> +	ulong aoeminor;

sounds like the wrong type, why not use dev_t ?

> +	ulong nopen;		/* user count */

why do you need this ?

> +static int
> +aoeblk_release(struct inode *inode, struct file *filp)
> +{
> +	Aoedev *d;
> +	ulong flags;
> +
> +	d = (Aoedev *) inode->i_bdev->bd_disk->private_data;
> +
> +	spin_lock_irqsave(&d->lock, flags);
> +	if (--d->nopen == 0)

eh why not just a ->release function instead that uses the blocklayer
refcounting instead of doing your own ?


> +int
> +aoeblk_make_request(request_queue_t *q, struct bio *bio)
> +{
> +	Aoedev *d;
> +	Buf *buf;
> +	struct sk_buff *sl;
> +	ulong flags;
> +
> +	blk_queue_bounce(q, &bio);
> +
> +	buf = kallocz(sizeof *buf, GFP_KERNEL);

this is deadlocky; you HAVE to use a mempool for allocations here!


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