Re: cpqarray patches for 2.6 [4 of 5]

[Jeff Garzik]

mikem@xxxxxxxxxxxxxxxxxxxxxxx wrote:
> This is patch 4 of 5 for cpqarray. Please apply in order.
>
> Thanks,
> mikem
> -------------------------------------------------------------------------------
>    - Change to use pci APIs (change from 2.4.18 to 2.4.19)
>      PS: This also includes eisa detection fix during initialization
>      which was missing from 2.4.19 but fixed in 2.4.25


While I like this patch a lot, there is one tiny bug I spotted, and one 
question:


> +	/* sendcmd will turn off interrupt, and send the flush...
> +	 * To write all data in the battery backed cache to disks
> +	 * no data returned, but don't want to send NULL to sendcmd */
> +	if( sendcmd(FLUSH_CACHE, i, buff, 4, 0, 0, 0))
> +	{
> +		printk(KERN_WARNING "Unable to flush cache on controller %d\n",
> +				i);
> +	}
> +	free_irq(hba[i]->intr, hba[i]);
> +	iounmap(hba[i]->vaddr);
> +	unregister_blkdev(COMPAQ_SMART2_MAJOR+i, hba[i]->devname);
> +	del_timer(&hba[i]->timer);

should this be del_timer_sync()?


> +	remove_proc_entry(hba[i]->devname, proc_array);
> +	pci_free_consistent(hba[i]->pci_dev,
> +			NR_CMDS * sizeof(cmdlist_t), (hba[i]->cmd_pool),
>  			hba[i]->cmd_pool_dhandle);
> -		kfree(hba[i]->cmd_pool_bits);
> +	kfree(hba[i]->cmd_pool_bits);
> +	for(j = 0; j < NWD; j++) {
> +		if (ida_gendisk[i][j]->flags & GENHD_FL_UP)
> +			del_gendisk(ida_gendisk[i][j]);
> +		devfs_remove("ida/c%dd%d",i,j);
> +		put_disk(ida_gendisk[i][j]);
> +	}
> +	blk_cleanup_queue(hba[i]->queue);
> +	release_io_mem(hba[i]);
> +	free_hba(i);
> +}
>  

> +	hba[i]->access.set_intr_mask(hba[i], 0);
> +	if (request_irq(hba[i]->intr, do_ida_intr,
> +		SA_INTERRUPT|SA_SHIRQ, hba[i]->devname, hba[i]))
> +	{
> +		printk(KERN_ERR "cpqarray: Unable to get irq %d for %s\n", 
>  				hba[i]->intr, hba[i]->devname);

Even though this line of code existed in the driver prior to your patch, 
 this highlights a bug:  SA_SHIRQ should really only be passed to 
request_irq() when the device is a PCI device.  If it's an EISA device, 
that really shouldn't be present.

I am also curious why SA_INTERRUPT is needed.  EISA requirement?

This patch gets my ACK otherwise...  I'm glad somebody did this.

	Jeff



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