Skip to contentSkip to conversation

fix gcc warning on 64 bit compile of gdth

PATCH In review James Bottomley 3 participants 7 messages Jan 3, 2005 – Jan 4, 2005 SCSI

lore
Share & tools
lore

James Bottomley

author
#0 lore raw

PATCH 1/1 fix gcc warning on 64 bit compile of gdth

This warning:

drivers/scsi/gdth.c: In function `gdth_fill_raw_cmd':
drivers/scsi/gdth.c:2948: warning: cast to pointer from integer of different size
drivers/scsi/gdth.c:2950: warning: cast to pointer from integer of different size
drivers/scsi/gdth.c: In function `gdth_sync_event':
drivers/scsi/gdth.c:3644: warning: cast from pointer to integer of different size
drivers/scsi/gdth.c:3646: warning: cast from pointer to integer of different size

can be fixed with the following patch.

James

===== drivers/scsi/gdth.c 1.47 vs edited =====

drivers/scsi/gdth.c +4 -4
@@ -2945,9 +2945,9 @@
         offset = (ulong)scp->sense_buffer & ~PAGE_MASK;
         sense_paddr = pci_map_page(ha->pdev,page,offset,
                                    16,PCI_DMA_FROMDEVICE);
-        scp->SCp.buffer = (struct scatterlist *)((ulong32)sense_paddr);
+        scp->SCp.buffer = (struct scatterlist *)((unsigned long)sense_paddr);
         /* high part, if 64bit */
-        scp->host_scribble = (char *)(ulong32)((ulong64)sense_paddr >> 32);
+        scp->host_scribble = (char *)(unsigned long)((ulong64)sense_paddr >> 32);
         cmdp->OpCode           = GDT_WRITE;             /* always */
         cmdp->BoardNode        = LOCALBOARD;
         if (mode64) { 

@@ -3641,9 +3641,9 @@
                            scp->request_bufflen,scp->SCp.Message);
         if (scp->SCp.buffer) {
             dma_addr_t addr;
-            addr = (dma_addr_t)(ulong32)scp->SCp.buffer;
+            addr = (dma_addr_t)(unsigned long)scp->SCp.buffer;
             if (scp->host_scribble)
-                addr += (dma_addr_t)((ulong64)(ulong32)scp->host_scribble << 32);               
+                addr += (dma_addr_t)((ulong64)(unsigned long)scp->host_scribble << 32);               
             pci_unmap_page(ha->pdev,addr,16,PCI_DMA_FROMDEVICE);
         }
#1 lore raw
Quoted 13 lines · James Bottomley

This warning:

drivers/scsi/gdth.c: In function `gdth_fill_raw_cmd':
drivers/scsi/gdth.c:2948: warning: cast to pointer from integer of different size
drivers/scsi/gdth.c:2950: warning: cast to pointer from integer of different size
drivers/scsi/gdth.c: In function `gdth_sync_event':
drivers/scsi/gdth.c:3644: warning: cast from pointer to integer of different size
drivers/scsi/gdth.c:3646: warning: cast from pointer to integer of different size

Quoted 1 line

can be fixed with the following patch.

*UGH*

Why do you play with shoving dma_addr_t into a pair of pointers and then decoding it when you have ->SCp.dma_handle?

author
#2 lore raw
Quoted 4 lines · Al Viro

*UGH*

Why do you play with shoving dma_addr_t into a pair of pointers and then decoding it when you have ->SCp.dma_handle?

Well, I don't since it's not my driver.

However, I think the nutcase it's trying to crack is storing dma_addr_t on x86 PAE. There dma_addr_t is wider than a void *.

James

#3 lore raw
Quoted 10 lines · James Bottomley
Quoted 4 lines · Al Viro

*UGH*

Why do you play with shoving dma_addr_t into a pair of pointers and then decoding it when you have ->SCp.dma_handle?

Well, I don't since it's not my driver.

However, I think the nutcase it's trying to crack is storing dma_addr_t on x86 PAE. There dma_addr_t is wider than a void *.

... and dma_handle *is* dma_addr_t and gets used in exact same driver to store the results of the same function (grep for pci_map_page in there).

author
#4 lore raw
Quoted 2 lines · Al Viro

... and dma_handle *is* dma_addr_t and gets used in exact same driver to store the results of the same function (grep for pci_map_page in there).

Fine. But I usually observe the rule of tamper minimally ... drivers have a nasty habit of sticking to you if you do more than that.

Indeed, now I look at the cleaned up patch, the think will leak DMA mappings if sense_paddr is zero (which is quite possible, since zero is a valid bus physical address). However, I'm declaring this too minimal to bother with ... unless you want to fix it?

James

===== drivers/scsi/gdth.c 1.47 vs edited =====

drivers/scsi/gdth.c +6 -11
@@ -2945,9 +2945,8 @@
         offset = (ulong)scp->sense_buffer & ~PAGE_MASK;
         sense_paddr = pci_map_page(ha->pdev,page,offset,
                                    16,PCI_DMA_FROMDEVICE);
-        scp->SCp.buffer = (struct scatterlist *)((ulong32)sense_paddr);
-        /* high part, if 64bit */
-        scp->host_scribble = (char *)(ulong32)((ulong64)sense_paddr >> 32);
+        scp->SCp.dma_handle = sense_paddr;
+
         cmdp->OpCode           = GDT_WRITE;             /* always */
         cmdp->BoardNode        = LOCALBOARD;
         if (mode64) { 

@@ -3639,13 +3638,9 @@
         else if (scp->SCp.Status == GDTH_MAP_SINGLE) 
             pci_unmap_page(ha->pdev,scp->SCp.dma_handle,
                            scp->request_bufflen,scp->SCp.Message);
-        if (scp->SCp.buffer) {
-            dma_addr_t addr;
-            addr = (dma_addr_t)(ulong32)scp->SCp.buffer;
-            if (scp->host_scribble)
-                addr += (dma_addr_t)((ulong64)(ulong32)scp->host_scribble << 32);               
-            pci_unmap_page(ha->pdev,addr,16,PCI_DMA_FROMDEVICE);
-        }
+        if (scp->SCp.dma_handle)
+            pci_unmap_page(ha->pdev,scp->SCp.dma_handle,16,PCI_DMA_FROMDEVICE);
+
 
         if (ha->status == S_OK) {
             scp->SCp.Status = S_OK;

@@ -4830,7 +4825,7 @@
     scp->SCp.phase = -1;
     scp->SCp.sent_command = -1;
     scp->SCp.Status = GDTH_MAP_NONE;
-    scp->SCp.buffer = (struct scatterlist *)NULL;
+    scp->SCp.dma_handle = 0;
 
     hanum = NUMDATA(scp->device->host)->hanum;
 #ifdef GDTH_STATISTICS
#5 lore raw
Quoted 11 lines · James Bottomley
Quoted 2 lines · Al Viro

... and dma_handle *is* dma_addr_t and gets used in exact same driver to store the results of the same function (grep for pci_map_page in there).

Fine. But I usually observe the rule of tamper minimally ... drivers have a nasty habit of sticking to you if you do more than that.

Indeed, now I look at the cleaned up patch, the think will leak DMA mappings if sense_paddr is zero (which is quite possible, since zero is a valid bus physical address). However, I'm declaring this too minimal to bother with ... unless you want to fix it?

After looking at the source some more... NAK. Turns out that there was a reason to avoid reuse of ->dma_handle here. What happens is that we might map *two* things in gdth_fill_raw_command() - scp->sense_data and possibly scp->request_buffer. So yes, there is a reason for not reusing ->dma_handle - it's used for the latter.

So ->dma_handle is not a solution. However, "shove it into ->Buffer and ->host_scribble" is not a solution either, AFAICS.
a) the problem you've found with code that unmaps stuff. Probably
should be handled by looking at ->SCp.Status instead of ->SCp.Buffer and setting a bit in .Status in the same place where we are currently setting .Buffer.
b) AFAICS, driver blindly assumes that pci_map_page() and friends
never fail. Probably should be checked in gdth_fill_..._cmd() so we could bail out before we feed fsck knows what to the hardware...
c) this "shove dma_address_t into two pointers" is, indeed, fscking
ugly and brittle (note e.g. the bug with checking lower 32 bits first and then adding upper 32 from ->host_scribble only if they were not all 0). What do other drivers do when they want more than one mapping?

Comments?

#6 lore raw
In reply to Al Viro
Quoted 2 lines · Al Viro

... and dma_handle *is* dma_addr_t and gets used in exact same driver to store the results of the same function (grep for pci_map_page in there).

But as it looks do me the driver is doing two different dma mappings in here.