On Fri, Feb 09, 2007 at 10:55:39AM -0500, James Bottomley wrote:
> On Thu, 2007-02-08 at 19:40 +0000, Christoph Hellwig wrote:
> > The logic here seems odd to me. Shouldn't we first check
> > dma_get_required_mask and the do dma_set_mask?
>
> Yes ... I picked up the mask problem, but not that. And actually
> there's a missing zero check on one of the dma_set_mask()s.
>
> Does this look like the right patch then?
This looks functionally correct to me, but there's some cosmetic issues
left:
> Index: scsi-misc-2.6/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
> ===================================================================
> --- scsi-misc-2.6.orig/drivers/scsi/aic7xxx/aic79xx_osm_pci.c 2007-02-09 09:12:55.000000000 -0500
> +++ scsi-misc-2.6/drivers/scsi/aic7xxx/aic79xx_osm_pci.c 2007-02-09 09:21:32.000000000 -0500
> @@ -132,6 +132,7 @@ ahd_linux_pci_dev_probe(struct pci_dev *
> struct ahd_pci_identity *entry;
> char *name;
> int error;
> + struct device *dev = &pdev->dev;
>
> pci = pdev;
> entry = ahd_find_pci_device(pci);
> @@ -161,20 +162,16 @@ ahd_linux_pci_dev_probe(struct pci_dev *
> pci_set_master(pdev);
>
> if (sizeof(dma_addr_t) > 4) {
> - uint64_t memsize;
> - const uint64_t mask_39bit = 0x7FFFFFFFFFULL;
> -
> - memsize = ahd_linux_get_memsize();
> -
> - if (memsize >= 0x8000000000ULL
> - && pci_set_dma_mask(pdev, DMA_64BIT_MASK) == 0) {
> + if (dma_get_required_mask(dev) > DMA_39BIT_MASK
> + && dma_set_mask(dev, DMA_64BIT_MASK) == 0)
> ahd->flags |= AHD_64BIT_ADDRESSING;
> - } else if (memsize > 0x80000000
> - && pci_set_dma_mask(pdev, mask_39bit) == 0) {
> + else if (dma_get_required_mask(dev) > DMA_32BIT_MASK
> + && dma_set_mask(dev, DMA_39BIT_MASK) == 0)
> ahd->flags |= AHD_39BIT_ADDRESSING;
> - }
> + else
> + dma_set_mask(dev, DMA_32BIT_MASK);
> } else {
I'd rather do the dma_get_required_mask only once, and we we want
&& at the end of the first instead of at the beginning of the second
line. So this block should look like:
u64 required_mask = dma_get_required_mask(dev);
if (required_mask > DMA_39BIT_MASK &&
dma_set_mask(dev, DMA_64BIT_MASK) == 0)
ahd->flags |= AHD_64BIT_ADDRESSING;
else if (required_mask > DMA_32BIT_MASK &&
dma_set_mask(dev, DMA_39BIT_MASK) == 0)
ahd->flags |= AHD_39BIT_ADDRESSING;
else
dma_set_mask(dev, DMA_32BIT_MASK);
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
