Re: [PATCH 3/8] cciss: new disk register/deregister routines

[Andrew Morton <akpm@osdl.org>]

mike.miller@hp.com wrote:
>
> Patch 3 of 8
> This patch removes a couple of functions dealing  with configuration and
> replaces them with new functions. This implementation fixes some bugs
> associated with the ACUXE. It also allows a logical volume to be removed
> from the middle without deleting all volumes behind it.
> If a user has 5 logical volumes and decides he wants to reconfigure volume
> number 3, he can now do that without removing volumes 4 & 5 first. This
> code has been tested in our labs against all application software.
> Please consider this for inclusion.
> 
> ...
>
> +static void cciss_update_drive_info(int ctlr, int drv_index)
> +  {
> +	ctlr_info_t *h = hba[ctlr];
> +	struct gendisk *disk;
> +	ReadCapdata_struct *size_buff = NULL;
> +	InquiryData_struct *inq_buff = NULL;
> +	unsigned int block_size;
> +	unsigned int total_size;
> +	unsigned long flags = 0;
> +	int ret = 0;
> +
> +	/* if the disk already exists then deregister it before proceeding*/
> +	if (h->drv[drv_index].raid_level != -1){
> +		spin_lock_irqsave(CCISS_LOCK(h->ctlr), flags);
> +		h->drv[drv_index].busy_configuring = 1;
> +		spin_unlock_irqrestore(CCISS_LOCK(h->ctlr), flags);

I always get worried when I see a spinlock around a simple assignment like
this - it often doesn't make a lot of sense and can indicate that
something's wrong.

> ...
> +
> +	/* Get information about the disk and modify the driver sturcture */
> +	size_buff = kmalloc(sizeof( ReadCapdata_struct), GFP_KERNEL);
> +        if (size_buff == NULL)
> +		goto mem_msg;
> +	inq_buff = kmalloc(sizeof( InquiryData_struct), GFP_KERNEL);
> +       	if (inq_buff == NULL)
> +		goto mem_msg;

Indenting went wrong.

urgh, maybe it just looks wrong because someone's being inserting spaces
instead of tabs.

> +		ld_buff = kmalloc(sizeof(ReportLunData_struct), GFP_KERNEL);
> +		if (ld_buff == NULL)
> +			goto mem_msg;
> +		memset(ld_buff, 0, sizeof(ReportLunData_struct));

We have kzalloc() now.

>  	/* make sure logical volume is NOT is use */
> -	if( drv->usage_count > 1) {
> -		spin_unlock_irqrestore(CCISS_LOCK(ctlr), flags);
> +	if(clear_all || (h->gendisk[0] == disk)) {
> +                if (drv->usage_count > 1)
>                  return -EBUSY;

Again, it looks wrong but is probably OK due to broken tab key.

Do you want the code to go in like this or do you want to clean it up?