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

Jeff Garzik
2003-10-06T21:55:49Z

Mark Haverkamp wrote:

> +static void aac_io_done(Scsi_Cmnd * scsicmd)
> +{
> + unsigned long cpu_flags;
> + struct Scsi_Host *host = scsicmd->device->host;
> + spin_lock_irqsave(host->host_lock, cpu_flags);
> + scsicmd->scsi_done(scsicmd);
> + spin_unlock_irqrestore(host->host_lock, cpu_flags);
> +}

Do you really need the host lock to end an IO?

> +static void __aac_io_done(Scsi_Cmnd * scsicmd)
> +{
> + scsicmd->scsi_done(scsicmd);
> +}

This stuff should be in scsi_lib.c or a header in include/scsi, don't
you think?

> @@ -523,9 +632,27 @@
> dev->pae_support = (paemode!=0);
> }
> if(dev->pae_support != 0) {
> - printk(KERN_INFO"%s%d: 64 Bit PAE enabled\n", dev->name, dev->id);
> - pci_set_dma_mask(dev->pdev, (dma_addr_t)0xFFFFFFFFFFFFFFFFULL);
> + if (pci_set_dma_mask(dev->pdev,
> + (dma_addr_t)0xFFFFFFFFFFFFFFFFULL)) {

you may (and should) remove the cast.

> + printk(KERN_INFO"%s%d: DMA mask set failed, 64 Bit PAE disabled\n",
> + dev->name, dev->id);
> + dev->pae_support = 0;
> + } else {
> + printk(KERN_INFO"%s%d: 64 Bit PAE enabled\n",
> + dev->name, dev->id);

"64-bit DAC mode" may be a little bit more precise.

PAE is processor, and x86-specific at that. DAC is PCI bus.

> + } else {
> + /*
> + * Reset if Quirk 31 was used, since data
> + * transfers are ok.
> + */
> + if (pci_set_dma_mask(dev->pdev, (dma_addr_t)0xFFFFFFFFULL)) {
> + printk(KERN_INFO"aacraid: Can't reset DMA mask.\n");

remove the cast

> @@ -559,7 +686,7 @@
> scsicmd->use_sg,
> scsi_to_pci_dma_dir(scsicmd->sc_data_direction));
> else if(scsicmd->request_bufflen)
> - pci_unmap_single(dev->pdev, (dma_addr_t)(ulong)scsicmd->SCp.ptr,
> + pci_unmap_single(dev->pdev, (dma_addr_t)(unsigned long)scsicmd->SCp.ptr,
> scsicmd->request_bufflen,
> scsi_to_pci_dma_dir(scsicmd->sc_data_direction));
> readreply = (struct aac_read_reply *)fib_data(fibptr);

do you really need all that casting? :)

> @@ -603,7 +734,7 @@
> scsicmd->use_sg,
> scsi_to_pci_dma_dir(scsicmd->sc_data_direction));
> else if(scsicmd->request_bufflen)
> - pci_unmap_single(dev->pdev, (dma_addr_t)(ulong)scsicmd->SCp.ptr,
> + pci_unmap_single(dev->pdev, (dma_addr_t)(unsigned long)scsicmd->SCp.ptr,
> scsicmd->request_bufflen,
> scsi_to_pci_dma_dir(scsicmd->sc_data_direction));
>

likewise

> @@ -1235,7 +1511,7 @@
> scsicmd->use_sg,
> scsi_to_pci_dma_dir(scsicmd->sc_data_direction));
> else if(scsicmd->request_bufflen)
> - pci_unmap_single(dev->pdev, (ulong)scsicmd->SCp.ptr, scsicmd->request_bufflen,
> + pci_unmap_single(dev->pdev, (unsigned long)scsicmd->SCp.ptr, scsicmd->request_bufflen,
> scsi_to_pci_dma_dir(scsicmd->sc_data_direction));
>

ah-hah, you choose another casting style, differing from the above
examples I pointed out...