Re: [patch 26/32] xen: Add Xen virtual block device driver.

[Jeremy Fitzhardinge <jeremy@goop.org>]

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.
>   

Yup.

>> +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.
>   

OK.

> 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.
>   

OK.

>> +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.
>   

OK.

>> +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.
>   

OK.

[...]

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.

Thanks,
    J