arcmsr: upper 32 of dma address lost

PATCH Superseded Dan Carpenter 2 participants 6 messages Feb 7, 2014 – Feb 11, 2014

Subsystems (2)

SCSI Kernel janitors

lore

Share & tools

lore

Dan Carpenter

author

2014-02-07 13:00 UTC #0 lore raw

PATCH 1/1 arcmsr: upper 32 of dma address lost

The original code always set the upper 32 bits to zero because it was doing a shift of the wrong variable.

Fixes 1a4f550a09f8 ('[SCSI] arcmsr: 1.20.00.15: add SATA RAID plus other fixes')

Signed-off-by Dan Carpenter <[email protected]>

drivers/scsi/arcmsr/arcmsr_hba.c +3 -4

index 4f6a30b8e5f9..9cfd399c47c0 100644
@@ -2500,16 +2500,15 @@ static int arcmsr_polling_ccbdone(struct AdapterControlBlock *acb,
 static int arcmsr_iop_confirm(struct AdapterControlBlock *acb)
 {
 	uint32_t cdb_phyaddr, cdb_phyaddr_hi32;
-	dma_addr_t dma_coherent_handle;
+
 	/*
 	********************************************************************
 	** here we need to tell iop 331 our freeccb.HighPart
 	** if freeccb.HighPart is not zero
 	********************************************************************
 	*/
-	dma_coherent_handle = acb->dma_coherent_handle;
-	cdb_phyaddr = (uint32_t)(dma_coherent_handle);
-	cdb_phyaddr_hi32 = (uint32_t)((cdb_phyaddr >> 16) >> 16);
+	cdb_phyaddr = (uint32_t)(acb->dma_coherent_handle);
+	cdb_phyaddr_hi32 = (uint32_t)(acb->dma_coherent_handle >> 32);
 	acb->cdb_phyaddr_hi32 = cdb_phyaddr_hi32;
 	/*
 	***********************************************************************

Dan Carpenter

author

2014-02-07 13:07 UTC #1 lore raw

Quoted 3 lines · Dan Carpenter

The original code always set the upper 32 bits to zero because it was doing a shift of the wrong variable.

Actually let me redo this. I want to add a cast to prevent a static checker warning on 32 bit systems. Sorry, for the noise.

regards,
dan carpenter

Quoted 27 lines

Fixes 1a4f550a09f8 ('[SCSI] arcmsr: 1.20.00.15: add SATA RAID plus other fixes')

Signed-off-by Dan Carpenter <[email protected]>

drivers/scsi/arcmsr/arcmsr_hba.c +3 -4

index 4f6a30b8e5f9..9cfd399c47c0 100644
@@ -2500,16 +2500,15 @@ static int arcmsr_polling_ccbdone(struct AdapterControlBlock *acb,
 static int arcmsr_iop_confirm(struct AdapterControlBlock *acb)
 {
 	uint32_t cdb_phyaddr, cdb_phyaddr_hi32;
-	dma_addr_t dma_coherent_handle;
+
 	/*
 	********************************************************************
 	** here we need to tell iop 331 our freeccb.HighPart
 	** if freeccb.HighPart is not zero
 	********************************************************************
 	*/
-	dma_coherent_handle = acb->dma_coherent_handle;
-	cdb_phyaddr = (uint32_t)(dma_coherent_handle);
-	cdb_phyaddr_hi32 = (uint32_t)((cdb_phyaddr >> 16) >> 16);
+	cdb_phyaddr = (uint32_t)(acb->dma_coherent_handle);
+	cdb_phyaddr_hi32 = (uint32_t)(acb->dma_coherent_handle >> 32);
 	acb->cdb_phyaddr_hi32 = cdb_phyaddr_hi32;
 	/*
 	***********************************************************************

Dan Carpenter

author

2014-02-10 15:18 UTC #2 lore raw

PATCH v2 1/1 arcmsr: upper 32 of dma address lost

In reply to Dan Carpenter

The original code always set the upper 32 bits to zero because it was doing a shift of the wrong variable.

Fixes 1a4f550a09f8 ('[SCSI] arcmsr: 1.20.00.15: add SATA RAID plus other fixes')

Signed-off-by Dan Carpenter <[email protected]>

---
v2: Add a cast to u64 to avoid a future static checker warning

drivers/scsi/arcmsr/arcmsr_hba.c +3 -4

index 4f6a30b8e5f9..9cfd399c47c0 100644
@@ -2500,16 +2500,15 @@ static int arcmsr_polling_ccbdone(struct AdapterControlBlock *acb,
 static int arcmsr_iop_confirm(struct AdapterControlBlock *acb)
 {
 	uint32_t cdb_phyaddr, cdb_phyaddr_hi32;
-	dma_addr_t dma_coherent_handle;
+
 	/*
 	********************************************************************
 	** here we need to tell iop 331 our freeccb.HighPart
 	** if freeccb.HighPart is not zero
 	********************************************************************
 	*/
-	dma_coherent_handle = acb->dma_coherent_handle;
-	cdb_phyaddr = (uint32_t)(dma_coherent_handle);
-	cdb_phyaddr_hi32 = (uint32_t)((cdb_phyaddr >> 16) >> 16);
+	cdb_phyaddr = (uint32_t)(acb->dma_coherent_handle);
+	cdb_phyaddr_hi32 = (uint32_t)((u64)acb->dma_coherent_handle >> 32);
 	acb->cdb_phyaddr_hi32 = cdb_phyaddr_hi32;
 	/*
 	***********************************************************************

James Bottomley

maintainer

2014-02-10 15:22 UTC #3 lore raw

Quoted 19 lines · Dan Carpenter

The original code always set the upper 32 bits to zero because it was doing a shift of the wrong variable.
Fixes 1a4f550a09f8 ('[SCSI] arcmsr: 1.20.00.15: add SATA RAID plus other fixes')
Signed-off-by Dan Carpenter <[email protected]>
---
v2: Add a cast to u64 to avoid a future static checker warning
drivers/scsi/arcmsr/arcmsr_hba.c +1 -1
index 4f6a30b8e5f9..9cfd399c47c0 100644
@@ -2500,16 +2500,15 @@ static int arcmsr_polling_ccbdone(struct AdapterControlBlock *acb,
 static int arcmsr_iop_confirm(struct AdapterControlBlock *acb)
 {
 	uint32_t cdb_phyaddr, cdb_phyaddr_hi32;
-	dma_addr_t dma_coherent_handle;
+
 	/*

drivers/scsi/arcmsr/arcmsr_hba.c static int arcmsr_polling_ccbdone(struct AdapterControlBlock *acb, · line 2511

Quoted 10 lines

drivers/scsi/arcmsr/arcmsr_hba.c +2 -3

 	********************************************************************
 	** here we need to tell iop 331 our freeccb.HighPart
 	** if freeccb.HighPart is not zero
 	********************************************************************
 	*/
-	dma_coherent_handle = acb->dma_coherent_handle;
-	cdb_phyaddr = (uint32_t)(dma_coherent_handle);
-	cdb_phyaddr_hi32 = (uint32_t)((cdb_phyaddr >> 16) >> 16);
+	cdb_phyaddr = (uint32_t)(acb->dma_coherent_handle);
+	cdb_phyaddr_hi32 = (uint32_t)((u64)acb->dma_coherent_handle >> 32);

The original 16 >> 16 is better because there's no requirement to cast to u64 and take an expensive 64 bit build out on 32 bits just to avoid the warning.

There's actually a macro in kernel.h (upper_32_bits) that does this.

James

Quoted 8 lines

drivers/scsi/arcmsr/arcmsr_hba.c +0 -1
 	acb->cdb_phyaddr_hi32 = cdb_phyaddr_hi32;
 	/*
 	***********************************************************************
 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dan Carpenter

author

2014-02-10 15:29 UTC #4 lore raw

Quoted 35 lines · James Bottomley

Quoted 19 lines · Dan Carpenter
The original code always set the upper 32 bits to zero because it was doing a shift of the wrong variable.
Fixes 1a4f550a09f8 ('[SCSI] arcmsr: 1.20.00.15: add SATA RAID plus other fixes')
Signed-off-by Dan Carpenter <[email protected]>
---
v2: Add a cast to u64 to avoid a future static checker warning
drivers/scsi/arcmsr/arcmsr_hba.c +1 -1
index 4f6a30b8e5f9..9cfd399c47c0 100644
@@ -2500,16 +2500,15 @@ static int arcmsr_polling_ccbdone(struct AdapterControlBlock *acb,
 static int arcmsr_iop_confirm(struct AdapterControlBlock *acb)
 {
 	uint32_t cdb_phyaddr, cdb_phyaddr_hi32;
-	dma_addr_t dma_coherent_handle;
+
 	/*
drivers/scsi/arcmsr/arcmsr_hba.c static int arcmsr_polling_ccbdone(struct AdapterControlBlock *acb, · line 2511
Quoted 10 lines
drivers/scsi/arcmsr/arcmsr_hba.c +2 -3
 	********************************************************************
 	** here we need to tell iop 331 our freeccb.HighPart
 	** if freeccb.HighPart is not zero
 	********************************************************************
 	*/
-	dma_coherent_handle = acb->dma_coherent_handle;
-	cdb_phyaddr = (uint32_t)(dma_coherent_handle);
-	cdb_phyaddr_hi32 = (uint32_t)((cdb_phyaddr >> 16) >> 16);
+	cdb_phyaddr = (uint32_t)(acb->dma_coherent_handle);
+	cdb_phyaddr_hi32 = (uint32_t)((u64)acb->dma_coherent_handle >> 32);
The original 16 >> 16 is better because there's no requirement to cast to u64 and take an expensive 64 bit build out on 32 bits just to avoid the warning.

Quoted 2 lines

There's actually a macro in kernel.h (upper_32_bits) that does this.

Ah. I was wondering what that was. Thanks. I will resend.

regards,
dan carpenter

Dan Carpenter

author

2014-02-11 16:06 UTC #5 lore raw

PATCH v3 1/1 arcmsr: upper 32 of dma address lost

In reply to Dan Carpenter

The original code always set the upper 32 bits to zero because it was doing a shift of the wrong variable.

Fixes 1a4f550a09f8 ('[SCSI] arcmsr: 1.20.00.15: add SATA RAID plus other fixes')

Signed-off-by Dan Carpenter <[email protected]>

---
v3: In v2 I introduced an expensive cast to solve a static checker
warning but James noticed and pointed me to the upper_32_bits() macro.

drivers/scsi/arcmsr/arcmsr_hba.c +3 -4

index 4f6a30b8e5f9..9cfd399c47c0 100644
@@ -2500,16 +2500,15 @@ static int arcmsr_polling_ccbdone(struct AdapterControlBlock *acb,
 static int arcmsr_iop_confirm(struct AdapterControlBlock *acb)
 {
 	uint32_t cdb_phyaddr, cdb_phyaddr_hi32;
-	dma_addr_t dma_coherent_handle;
+
 	/*
 	********************************************************************
 	** here we need to tell iop 331 our freeccb.HighPart
 	** if freeccb.HighPart is not zero
 	********************************************************************
 	*/
-	dma_coherent_handle = acb->dma_coherent_handle;
-	cdb_phyaddr = (uint32_t)(dma_coherent_handle);
-	cdb_phyaddr_hi32 = (uint32_t)((cdb_phyaddr >> 16) >> 16);
+	cdb_phyaddr = lower_32_bits(acb->dma_coherent_handle);
+	cdb_phyaddr_hi32 = upper_32_bits(acb->dma_coherent_handle);
 	acb->cdb_phyaddr_hi32 = cdb_phyaddr_hi32;
 	/*
 	***********************************************************************