Re: [PATCH 3/3] 2.6.0 aacraid driver update

[Jeff Garzik <jgarzik@pobox.com>]

Mark Haverkamp wrote:
> @@ -194,8 +208,17 @@
>  			if (pci_enable_device(dev))
>  				continue;
>  			pci_set_master(dev);
> -			pci_set_dma_mask(dev, 0xFFFFFFFFULL);
>  
> +			if (aac_drivers[index].quirks & AAC_QUIRK_31BIT)
> +				ret = pci_set_dma_mask(dev, 0x7FFFFFFFULL);
> +			else
> +				ret = pci_set_dma_mask(dev, 0xFFFFFFFFULL);
> +
> +			if (ret) {
> +				printk(KERN_WARNING "aacraid: Can't set DMA mask.\n");
> +				continue;
> +			}
> +				
>  			if((dev->subsystem_vendor != aac_drivers[index].subsystem_vendor) || 
>  			   (dev->subsystem_device != aac_drivers[index].subsystem_device))
>  					continue;

This probe loop is fairly yucky, and this last 'if' test shows it's 
unnecessary.

Use the last field in struct pci_device_id to provide an index into a 
pci-id-specific info table.

And overall, let the PCI API do this sort of looping for you.



> +static irqreturn_t aac_rkt_intr(int irq, void *dev_id, struct pt_regs *regs)
> +{
> +	struct aac_dev *dev = dev_id;
> +	unsigned long bellbits;
> +	u8 intstat, mask;
> +	intstat = rkt_readb(dev, MUnit.OISR);
> +	/*
> +	 *	Read mask and invert because drawbridge is reversed.
> +	 *	This allows us to only service interrupts that have 
> +	 *	been enabled.
> +	 */
> +	mask = ~(rkt_readb(dev, MUnit.OIMR));

Don't keep reading this infrequently-changing interrupt mask from 
hardware...  Save an IO and cache that.  Is that possible?



> +	/*
> +	 *	Wait up to 30 seconds
> +	 */
> +	while (time_before(jiffies, start+30*HZ)) 
> +	{
> +		udelay(5);	/* Delay 5 microseconds to let Mon960 get info. */
> +		/*
> +		 *	Mon960 will set doorbell0 bit when it has completed the command.
> +		 */
> +		if (rkt_readl(dev, OutboundDoorbellReg) & OUTBOUNDDOORBELL_0) {
> +			/*
> +			 *	Clear the doorbell.
> +			 */
> +			rkt_writel(dev, OutboundDoorbellReg, OUTBOUNDDOORBELL_0);
> +			ok = 1;
> +			break;
> +		}
> +		/*
> +		 *	Yield the processor in case we are slow 
> +		 */
> +		set_current_state(TASK_UNINTERRUPTIBLE);
> +		schedule_timeout(1);

hmmm... why not simply call yield() here instead?  I think yield() is 
closer to the intent you wish to achieve...

Also note that you udelay() _before_ your first readb().  This could be 
a PCI posting bug.



> +	/*
> +	 *	Map in the registers from the adapter.
> +	 */
> +	if((dev->regs.rkt = (struct rkt_registers *)ioremap((unsigned long)dev->scsi_host_ptr->base, 8192))==NULL)
> +	{	
> +		printk(KERN_WARNING "aacraid: unable to map i960.\n" );
> +		return -1;
> +	}

Make the line length of this 'if' test shorter, and in the process make 
the code more readable, by splitting this operation into two lines:  the 
ioremap(), and the 'if' test for NULL.

The above is an excellent example of why people should _not_ combine 
assignment and 'if' tests... it just obfuscates the code.

	Jeff