aic79xx: use dma_get_required_mask()

PATCH In review Hannes Reinecke 3 participants 6 messages Feb 7, 2007 – Feb 9, 2007

Subsystems (2)

SCSI AIC7XXX / AIC79XX SCSI driver

lore

Share & tools

lore

Hannes Reinecke

author

2007-02-07 08:47 UTC #0 lore raw

PATCH 1/1 aic79xx: use dma_get_required_mask()

Hi James,

this patch corrects the issue originally noted by Frederic Temporelli:
aic79xx uses the wrong logic to determine the addressing mode. I've converted the original patch to use the dma_get_required_mask() macros as you requested.

Please apply.

Cheers,

Hannes

Signature · 4 lines

Dr. Hannes Reinecke			[email protected]
SuSE Linux Products GmbH		S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de

aic79xx: Use dma_get_required_mask()

As originally noted by Frederic Temporelli, the aic79xx supports 64 bit addressing, but the initialization code of the driver is wrong: it tests the available memory size instead of testing the maximum available memory address.

This patch uses the correct dma_get_required_mask() macros to determine the correct addressing method.

Signed-off-by Hannes Reinecke <[email protected]>

Cc Xavier Bru <[email protected]>

CC Frederic Temporelli <[email protected]>

drivers/scsi/aic7xxx/aic79xx_osm.c +0 -9

index 9bfcca5..c7fe478 100644
@@ -1126,15 +1126,6 @@ ahd_linux_register_host(struct ahd_softc
 	return 0;
 }
 
-uint64_t
-ahd_linux_get_memsize(void)
-{
-	struct sysinfo si;
-
-	si_meminfo(&si);
-	return ((uint64_t)si.totalram << PAGE_SHIFT);
-}
-
 /*
  * Place the SCSI bus into a known state by either resetting it,
  * or forcing transfer negotiations on the next command to any

drivers/scsi/aic7xxx/aic79xx_osm.h +0 -2

index 3a67fc5..147c83c 100644
@@ -496,8 +496,6 @@ ahd_insb(struct ahd_softc * ahd, long po
 int		ahd_linux_register_host(struct ahd_softc *,
 					struct scsi_host_template *);
 
-uint64_t	ahd_linux_get_memsize(void);
-
 /*************************** Pretty Printing **********************************/
 struct info_str {
 	char *buffer;

drivers/scsi/aic7xxx/aic79xx_osm_pci.c +10 ~~-11~~

index 1a3ab6a..22250e6 100644
@@ -127,11 +127,13 @@ static int
 ahd_linux_pci_dev_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	char		 buf[80];
+	const uint64_t	 mask_39bit = 0x7FFFFFFFFFULL;
 	struct		 ahd_softc *ahd;
 	ahd_dev_softc_t	 pci;
 	struct		 ahd_pci_identity *entry;
 	char		*name;
 	int		 error;
+	struct device	*dev = &pdev->dev;
 
 	pci = pdev;
 	entry = ahd_find_pci_device(pci);

@@ -161,20 +163,17 @@ 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_set_mask(dev, DMA_64BIT_MASK)
+		    && dma_get_required_mask(dev) > mask_39bit)
 			ahd->flags |= AHD_64BIT_ADDRESSING;
-		} else if (memsize > 0x80000000
-			&& pci_set_dma_mask(pdev, mask_39bit) == 0) {
+		else if (dma_set_mask(dev, mask_39bit) == 0
+			 && dma_get_required_mask(dev) > DMA_32BIT_MASK)
 			ahd->flags |= AHD_39BIT_ADDRESSING;
-		}
+		else
+			dma_set_mask(dev, DMA_32BIT_MASK);
+			
 	} else {
-		pci_set_dma_mask(pdev, DMA_32BIT_MASK);
+		dma_set_mask(dev, DMA_32BIT_MASK);
 	}
 	ahd->dev_softc = pci;
 	error = ahd_pci_config(ahd, entry);

James Bottomley

2007-02-08 00:28 UTC #1 lore raw

drivers/scsi/aic7xxx/aic79xx_osm_pci.c static int · line 130

Quoted 1 line · Hannes Reinecke

drivers/scsi/aic7xxx/aic79xx_osm_pci.c +1 -0
+	const uint64_t	 mask_39bit = 0x7FFFFFFFFFULL;

Actually there's no need for this ... the mask_39bit references can just become DMA_39BIT_MASK constants. I made this cosmetic change to the patch.

James

Christoph Hellwig

2007-02-08 19:40 UTC #2 lore raw

In reply to Hannes Reinecke

Quoted 29 lines

ahd_linux_pci_dev_probe(struct pci_dev *pdev, const struct pci_device_id *ent) {

drivers/scsi/aic7xxx/aic79xx_osm_pci.c +4 -9

 	char		 buf[80];
+	const uint64_t	 mask_39bit = 0x7FFFFFFFFFULL;
 	struct		 ahd_softc *ahd;
 	ahd_dev_softc_t	 pci;
 	struct		 ahd_pci_identity *entry;
 	char		*name;
 	int		 error;
+	struct device	*dev = &pdev->dev;
 
 	pci = pdev;
 	entry = ahd_find_pci_device(pci);

@@ -161,20 +163,17 @@ 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_set_mask(dev, DMA_64BIT_MASK)
+		    && dma_get_required_mask(dev) > mask_39bit)
 			ahd->flags |= AHD_64BIT_ADDRESSING;
-		} else if (memsize > 0x80000000
-			&& pci_set_dma_mask(pdev, mask_39bit) == 0) {

drivers/scsi/aic7xxx/aic79xx_osm_pci.c ahd_linux_pci_dev_probe(struct pci_dev * · line 176

Quoted 10 lines

drivers/scsi/aic7xxx/aic79xx_osm_pci.c +6 -2

+		else if (dma_set_mask(dev, mask_39bit) == 0
+			 && dma_get_required_mask(dev) > DMA_32BIT_MASK)
 			ahd->flags |= AHD_39BIT_ADDRESSING;
-		}
+		else
+			dma_set_mask(dev, DMA_32BIT_MASK);
+			
 	} else {
-		pci_set_dma_mask(pdev, DMA_32BIT_MASK);
+		dma_set_mask(dev, DMA_32BIT_MASK);

The logic here seems odd to me. Shouldn't we first check dma_get_required_mask and the do dma_set_mask?

James Bottomley

2007-02-09 15:55 UTC #3 lore raw

Quoted 2 lines · Christoph Hellwig

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?

James

Quoted 1 line

From 095ec6c45d80171b31ee04da62618dcad31b8aef Mon Sep 17 00:00:00 2001

From: Hannes Reinecke <[email protected]>
Date: Wed, 7 Feb 2007 09:47:44 +0100
Subject: [SCSI] aic79xx: use dma_get_required_mask()

This patch uses the correct dma_get_required_mask() macros to determine the correct addressing method.

Signed-off-by Hannes Reinecke <[email protected]>

Cc Xavier Bru <[email protected]>

CC Frederic Temporelli <[email protected]>

cosmetic fixes

Signed-off-by James Bottomley <[email protected]>

---
 drivers/scsi/aic7xxx/aic79xx_osm.c     |    9 ---------
 drivers/scsi/aic7xxx/aic79xx_osm.h     |    2 --
 drivers/scsi/aic7xxx/aic79xx_osm_pci.c |   20 +++++++++-----------
 3 files changed, 9 insertions(+), 22 deletions(-)

drivers/scsi/aic7xxx/aic79xx_osm.c +0 -9

Index: scsi-misc-2.6/drivers/scsi/aic7xxx/aic79xx_osm.c
===================================================================
@@ -1126,15 +1126,6 @@ ahd_linux_register_host(struct ahd_softc
 	return 0;
 }
 
-uint64_t
-ahd_linux_get_memsize(void)
-{
-	struct sysinfo si;
-
-	si_meminfo(&si);
-	return ((uint64_t)si.totalram << PAGE_SHIFT);
-}
-
 /*
  * Place the SCSI bus into a known state by either resetting it,
  * or forcing transfer negotiations on the next command to any

drivers/scsi/aic7xxx/aic79xx_osm.h +0 -2

Index: scsi-misc-2.6/drivers/scsi/aic7xxx/aic79xx_osm.h
===================================================================
@@ -496,8 +496,6 @@ ahd_insb(struct ahd_softc * ahd, long po
 int		ahd_linux_register_host(struct ahd_softc *,
 					struct scsi_host_template *);
 
-uint64_t	ahd_linux_get_memsize(void);
-
 /*************************** Pretty Printing **********************************/
 struct info_str {
 	char *buffer;

drivers/scsi/aic7xxx/aic79xx_osm_pci.c +8 ~~-11~~

Index: scsi-misc-2.6/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
===================================================================
@@ -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 {
-		pci_set_dma_mask(pdev, DMA_32BIT_MASK);
+		dma_set_mask(dev, DMA_32BIT_MASK);
 	}
 	ahd->dev_softc = pci;
 	error = ahd_pci_config(ahd, entry);

Christoph Hellwig

2007-02-09 16:10 UTC #4 lore raw

Quoted 8 lines · James Bottomley

Quoted 2 lines · Christoph Hellwig
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:

Quoted 25 lines

Index: scsi-misc-2.6/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
===================================================================

drivers/scsi/aic7xxx/aic79xx_osm_pci.c +3 -7

@@ -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)

drivers/scsi/aic7xxx/aic79xx_osm_pci.c ahd_linux_pci_dev_probe(struct pci_dev * · line 173

Quoted 10 lines

drivers/scsi/aic7xxx/aic79xx_osm_pci.c +4 -3

 			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);

James Bottomley

2007-02-09 16:37 UTC #5 lore raw

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · James Bottomley
Quoted 2 lines · Christoph Hellwig
The logic here seems odd to me. Shouldn't we first check dma_get_required_mask and the do dma_set_mask?

Quoted 7 lines

Quoted 4 lines
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:

OK, try this one.

By the way, when did I suddenly become your patch monkey?

James

Quoted 1 line

From 095ec6c45d80171b31ee04da62618dcad31b8aef Mon Sep 17 00:00:00 2001

From: Hannes Reinecke <[email protected]>
Date: Wed, 7 Feb 2007 09:47:44 +0100
Subject: [SCSI] aic79xx: use dma_get_required_mask()

This patch uses the correct dma_get_required_mask() macros to determine the correct addressing method.

Signed-off-by Hannes Reinecke <[email protected]>

Cc Xavier Bru <[email protected]>

CC Frederic Temporelli <[email protected]>

cosmetic fixes

Signed-off-by James Bottomley <[email protected]>

---
 drivers/scsi/aic7xxx/aic79xx_osm.c     |    9 ---------
 drivers/scsi/aic7xxx/aic79xx_osm.h     |    2 --
 drivers/scsi/aic7xxx/aic79xx_osm_pci.c |   20 +++++++++-----------
 3 files changed, 9 insertions(+), 22 deletions(-)

drivers/scsi/aic7xxx/aic79xx_osm.c +0 -9

Index: scsi-misc-2.6/drivers/scsi/aic7xxx/aic79xx_osm.c
===================================================================
@@ -1126,15 +1126,6 @@ ahd_linux_register_host(struct ahd_softc
 	return 0;
 }
 
-uint64_t
-ahd_linux_get_memsize(void)
-{
-	struct sysinfo si;
-
-	si_meminfo(&si);
-	return ((uint64_t)si.totalram << PAGE_SHIFT);
-}
-
 /*
  * Place the SCSI bus into a known state by either resetting it,
  * or forcing transfer negotiations on the next command to any

drivers/scsi/aic7xxx/aic79xx_osm.h +0 -2

Index: scsi-misc-2.6/drivers/scsi/aic7xxx/aic79xx_osm.h
===================================================================
@@ -496,8 +496,6 @@ ahd_insb(struct ahd_softc * ahd, long po
 int		ahd_linux_register_host(struct ahd_softc *,
 					struct scsi_host_template *);
 
-uint64_t	ahd_linux_get_memsize(void);
-
 /*************************** Pretty Printing **********************************/
 struct info_str {
 	char *buffer;

drivers/scsi/aic7xxx/aic79xx_osm_pci.c +9 ~~-10~~

Index: scsi-misc-2.6/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
===================================================================
@@ -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,18 @@ 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;
+		const u64 required_mask = dma_get_required_mask(dev);
 
-		memsize = ahd_linux_get_memsize();
-
-		if (memsize >= 0x8000000000ULL
-	 	 && pci_set_dma_mask(pdev, DMA_64BIT_MASK) == 0) {
+		if (required_mask > 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 (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);
 	} else {
-		pci_set_dma_mask(pdev, DMA_32BIT_MASK);
+		dma_set_mask(dev, DMA_32BIT_MASK);
 	}
 	ahd->dev_softc = pci;
 	error = ahd_pci_config(ahd, entry);