Re: [patch 26/32] xen: Add Xen virtual block device driver.
From: Jeremy Fitzhardinge
Date: Mon Apr 30 2007 - 14:52:36 EST
Christoph Hellwig wrote:
>> source "drivers/s390/block/Kconfig"
>> +source "drivers/block/xen/Kconfig"
> Please don't add a new subdirectory for a tiny new driver that
> really should be only a single .c file.
>> +config XEN_BLKDEV_FRONTEND
>> + tristate "Block device frontend driver"
>> + depends on XEN
>> + default y
> Please kill the default statement. We really should have script that
> autorejects every new driver that wants to be default y..
I guess. But if you've already decided to enable Xen, it seems to me
that the default option should reflect the common case. But I don't
feel strongly about it either way.
>> +#define BLKIF_STATE_DISCONNECTED 0
>> +#define BLKIF_STATE_CONNECTED 1
>> +#define BLKIF_STATE_SUSPENDED 2
> enum, please.
> Please get rid of all the forward-declarations by putting the code
> into proper order.
Yep. I'll de-generic the names too.
>> +static inline int GET_ID_FROM_FREELIST(
>> + struct blkfront_info *info)
>> + unsigned long free = info->shadow_free;
>> + BUG_ON(free > BLK_RING_SIZE);
>> + info->shadow_free = info->shadow[free].req.id;
>> + info->shadow[free].req.id = 0x0fffffee; /* debug */
>> + return free;
>> +static inline void ADD_ID_TO_FREELIST(
>> + struct blkfront_info *info, unsigned long id)
>> + info->shadow[id].req.id = info->shadow_free;
>> + info->shadow[id].request = 0;
>> + info->shadow_free = id;
> Please give these proper lowercased names, and while you're at it
> please kill all the inline statements you have and let the compiler
> do it's work.
>> +static irqreturn_t blkif_int(int irq, void *dev_id)
> Please call interrupt handlers foo_interrupt, that makes peopl see what's
> going on much more easily.
>> +static void blkif_recover(struct blkfront_info *info)
>> + int i;
>> + struct blkif_request *req;
>> + struct blk_shadow *copy;
>> + int j;
>> + /* Stage 1: Make a safe copy of the shadow state. */
>> + copy = kmalloc(sizeof(info->shadow), GFP_KERNEL | __GFP_NOFAIL);
> Please don't use __GFP_NOFAIL in any new code.
The rest looks sound. I'll go through it in detail, but a lot of your
points things I've been meaning to get to anyway.
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/