[PATCH] fix gcc warning on 64 bit compile of gdth

[PATCH] fix gcc warning on 64 bit compile of gdth

[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

can be fixed with the following patch.

James

===== drivers/scsi/gdth.c 1.47 vs edited =====
--- 1.47/drivers/scsi/gdth.c	2005-01-03 10:53:29 -06:00
+++ edited/drivers/scsi/gdth.c	2005-01-03 11:09:32 -06:00
@@ -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);
         }
 

Re: [PATCH] fix gcc warning on 64 bit compile of gdth

[Al Viro]

On Mon, Jan 03, 2005 at 03:45:04PM -0600, James Bottomley wrote:
> 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.

*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?

Re: [PATCH] fix gcc warning on 64 bit compile of gdth

[James Bottomley]

On Mon, 2005-01-03 at 22:43 +0000, Al Viro wrote:
> *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

Re: [PATCH] fix gcc warning on 64 bit compile of gdth

[Al Viro]

On Mon, Jan 03, 2005 at 04:49:39PM -0600, James Bottomley wrote:
> On Mon, 2005-01-03 at 22:43 +0000, Al Viro wrote:
> > *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).

Re: [PATCH] fix gcc warning on 64 bit compile of gdth

[James Bottomley]

On Mon, 2005-01-03 at 22:54 +0000, Al Viro wrote:
> ... 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 =====
--- 1.47/drivers/scsi/gdth.c	2005-01-03 10:53:29 -06:00
+++ edited/drivers/scsi/gdth.c	2005-01-03 17:47:54 -06:00
@@ -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

Re: [PATCH] fix gcc warning on 64 bit compile of gdth

[Al Viro]

On Mon, Jan 03, 2005 at 05:53:38PM -0600, James Bottomley wrote:
> On Mon, 2005-01-03 at 22:54 +0000, Al Viro wrote:
> > ... 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?

Re: [PATCH] fix gcc warning on 64 bit compile of gdth

[Christoph Hellwig]

On Mon, Jan 03, 2005 at 10:54:01PM +0000, Al Viro wrote:
> ... 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.

RE: [PATCH] fix gcc warning on 64 bit compile of gdth

[Leubner, Achim]

You are right, in gdth_fill_raw_command() the driver is doing two
different dma mappings, one for the data buffer stored in
SCp.dma_handle, and the second for the sense buffer stored in SCp.buffer
and host_scribble. So reuse of dma_handle does not work here.
And, you are right, this is ugly and brittle, but there is no remaining
space in the SCp scratch area to save the second dma mapping.  
The other patches look fine for me, thanks. But what's the reason to
remove the support of 2.2.x and 2.4.x without the full dma API?

Thanks,
Achim

----------------------------------------------
Achim Leubner
Research & Development
ICP vortex Computersysteme GmbH
Gostritzer Str. 61-63
D-01217 Dresden, Germany
Phone: +49-351-871-8291
Fax: +49-351-871-8448
Email: achim_leubner@adaptec.com
Web: www.icp-vortex.com



> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Dienstag, 4. Januar 2005 09:27
> To: Al Viro
> Cc: James Bottomley; Leubner, Achim; SCSI Mailing List
> Subject: Re: [PATCH] fix gcc warning on 64 bit compile of gdth
> 
> On Mon, Jan 03, 2005 at 10:54:01PM +0000, Al Viro wrote:
> > ... 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.

RE: [PATCH] fix gcc warning on 64 bit compile of gdth

[Arjan van de Ven]

On Tue, 2005-01-04 at 10:29 +0100, Leubner, Achim wrote:
> You are right, in gdth_fill_raw_command() the driver is doing two
> different dma mappings, one for the data buffer stored in
> SCp.dma_handle, and the second for the sense buffer stored in SCp.buffer
> and host_scribble. So reuse of dma_handle does not work here.

Is there a way the driver can get rid of looking at the sense buffer at
all and let the midlayer do this ?

> And, you are right, this is ugly and brittle, but there is no remaining
> space in the SCp scratch area to save the second dma mapping.  
> The other patches look fine for me, thanks. But what's the reason to
> remove the support of 2.2.x and 2.4.x without the full dma API?

ifdefs like that for really old kernels significantly clutter up the
driver and make fixing other things in it really really hard.

Re: [PATCH] fix gcc warning on 64 bit compile of gdth

[Al Viro]

On Tue, Jan 04, 2005 at 10:32:41AM +0100, Arjan van de Ven wrote:
> On Tue, 2005-01-04 at 10:29 +0100, Leubner, Achim wrote:
> > You are right, in gdth_fill_raw_command() the driver is doing two
> > different dma mappings, one for the data buffer stored in
> > SCp.dma_handle, and the second for the sense buffer stored in SCp.buffer
> > and host_scribble. So reuse of dma_handle does not work here.
> 
> Is there a way the driver can get rid of looking at the sense buffer at
> all and let the midlayer do this ?

AFAICS, hardware wants dma_address of that puppy passed to it.  Which might
be a solution of this problem, if that address is not clobbered and can
be read back from where we'd stored it.  But that's a question for hardware
folks - does the value of cmdp->u.{raw,raw64}.sense_data survive until we
get to unmapping stuff in gdth_sync_event()?

In any case, we really should check for failure of pci_map_...() and be
more careful with checking if two areas need to be unmapped...

RE: [PATCH] fix gcc warning on 64 bit compile of gdth

[Leubner, Achim]

Yes, that's the problem. I have to pass a physical address of
scp->sense_buffer to our controller firmware that the firmware can fill
that space with DMA after I/O. But cmdp->u.{raw,raw64}.sense_data does
not survive until we get to unmapping stuff in gdth_sync_event() because
there is only one cmdp structure per controller and the controller can
handle multiple commands. So I have to save the physical address
anywhere in the Scsi_Cmnd structure.
You are right, some pci_map...() checking stuff should be added.

> -----Original Message-----
> From: Al Viro [mailto:viro@www.linux.org.uk] On Behalf Of Al Viro
> Sent: Dienstag, 4. Januar 2005 11:30
> To: Arjan van de Ven
> Cc: Leubner, Achim; Christoph Hellwig; James Bottomley; SCSI Mailing
List
> Subject: Re: [PATCH] fix gcc warning on 64 bit compile of gdth
> 
> On Tue, Jan 04, 2005 at 10:32:41AM +0100, Arjan van de Ven wrote:
> > On Tue, 2005-01-04 at 10:29 +0100, Leubner, Achim wrote:
> > > You are right, in gdth_fill_raw_command() the driver is doing two
> > > different dma mappings, one for the data buffer stored in
> > > SCp.dma_handle, and the second for the sense buffer stored in
SCp.buffer
> > > and host_scribble. So reuse of dma_handle does not work here.
> >
> > Is there a way the driver can get rid of looking at the sense buffer
at
> > all and let the midlayer do this ?
> 
> AFAICS, hardware wants dma_address of that puppy passed to it.  Which
might
> be a solution of this problem, if that address is not clobbered and
can
> be read back from where we'd stored it.  But that's a question for
hardware
> folks - does the value of cmdp->u.{raw,raw64}.sense_data survive until
we
> get to unmapping stuff in gdth_sync_event()?
> 
> In any case, we really should check for failure of pci_map_...() and
be
> more careful with checking if two areas need to be unmapped...

Re: [PATCH] fix gcc warning on 64 bit compile of gdth

[Al Viro]

On Tue, Jan 04, 2005 at 11:57:33AM +0100, Leubner, Achim wrote:
> Yes, that's the problem. I have to pass a physical address of
> scp->sense_buffer to our controller firmware that the firmware can fill
> that space with DMA after I/O. But cmdp->u.{raw,raw64}.sense_data does
> not survive until we get to unmapping stuff in gdth_sync_event() because
> there is only one cmdp structure per controller and the controller can
> handle multiple commands. So I have to save the physical address
> anywhere in the Scsi_Cmnd structure.

Umm...   What about adding a new field to
    struct {
        Scsi_Cmnd       *cmnd;                  /* pending request */
        ushort          service;                /* service */
    } cmd_tab[GDTH_MAXCMDS];                    /* table of pend. reques
in gdth_ha_str?  It's not that we couldn't change size of that animal and
AFAICS anything that might be passed to gdth_sync_event() would have to
be extracted from ha->cmd_tab[] anyway...

BTW, what uses values stored in gdth_ctr_vtab[]?  It's static and gdth.[ch]
never read its elements, only set them...

RE: [PATCH] fix gcc warning on 64 bit compile of gdth

[Leubner, Achim]

Agreed. Adding a new field in cmd_tab[] is a good possibility to solve
the problem.

gdth_ctr_vtab[] is used in kernel 2.4.x in gdth_proc_info()
(gdth_proc.c) to find the right ha and bus number. The background of
gdth_ctr_vtab[] is a problem of older cdrecord versions with controllers
with more than one bus. In this case the driver gave the possibility to
split the controller into multiple controllers with one bus each
(command line switch "virt_ctr:Y").


> -----Original Message-----
> From: Al Viro [mailto:viro@www.linux.org.uk] On Behalf Of Al Viro
> Sent: Dienstag, 4. Januar 2005 12:53
> To: Leubner, Achim
> Cc: Arjan van de Ven; Christoph Hellwig; James Bottomley; SCSI Mailing
List
> Subject: Re: [PATCH] fix gcc warning on 64 bit compile of gdth
> 
> On Tue, Jan 04, 2005 at 11:57:33AM +0100, Leubner, Achim wrote:
> > Yes, that's the problem. I have to pass a physical address of
> > scp->sense_buffer to our controller firmware that the firmware can
fill
> > that space with DMA after I/O. But cmdp->u.{raw,raw64}.sense_data
does
> > not survive until we get to unmapping stuff in gdth_sync_event()
because
> > there is only one cmdp structure per controller and the controller
can
> > handle multiple commands. So I have to save the physical address
> > anywhere in the Scsi_Cmnd structure.
> 
> Umm...   What about adding a new field to
>     struct {
>         Scsi_Cmnd       *cmnd;                  /* pending request */
>         ushort          service;                /* service */
>     } cmd_tab[GDTH_MAXCMDS];                    /* table of pend.
reques
> in gdth_ha_str?  It's not that we couldn't change size of that animal
and
> AFAICS anything that might be passed to gdth_sync_event() would have
to
> be extracted from ha->cmd_tab[] anyway...
> 
> BTW, what uses values stored in gdth_ctr_vtab[]?  It's static and
gdth.[ch]
> never read its elements, only set them...

RE: [PATCH] fix gcc warning on 64 bit compile of gdth

[James Bottomley]

On Tue, 2005-01-04 at 10:29 +0100, Leubner, Achim wrote:
> You are right, in gdth_fill_raw_command() the driver is doing two
> different dma mappings, one for the data buffer stored in
> SCp.dma_handle, and the second for the sense buffer stored in SCp.buffer
> and host_scribble. So reuse of dma_handle does not work here.
> And, you are right, this is ugly and brittle, but there is no remaining
> space in the SCp scratch area to save the second dma mapping.  
> The other patches look fine for me, thanks. But what's the reason to
> remove the support of 2.2.x and 2.4.x without the full dma API?

Well, most other drivers don't have this problem.

The sense buffer is only used to communicate sense if the command fails
with a check condition.  The point being that you only have either the
command or the request sense active, so you can do the mappings
sequentially.  It's a bit of a waste of dma mapping space to map both
the request buffer and the sense buffer for every command that comes in.

Is it not possible to modify gdth to do the mappings sequentially and
only if the initial command gets the check condition?

James

Re: [PATCH] fix gcc warning on 64 bit compile of gdth

[Christoph Hellwig]

On Tue, Jan 04, 2005 at 10:29:15AM +0100, Leubner, Achim wrote:
> You are right, in gdth_fill_raw_command() the driver is doing two
> different dma mappings, one for the data buffer stored in
> SCp.dma_handle, and the second for the sense buffer stored in SCp.buffer
> and host_scribble. So reuse of dma_handle does not work here.
> And, you are right, this is ugly and brittle, but there is no remaining
> space in the SCp scratch area to save the second dma mapping.  
> The other patches look fine for me, thanks. But what's the reason to
> remove the support of 2.2.x and 2.4.x without the full dma API?

Too much noise in the driver for totally obsolete kernels.

Re: [PATCH] fix gcc warning on 64 bit compile of gdth

[Christoph Hellwig]

On Tue, Jan 04, 2005 at 10:32:41AM +0100, Arjan van de Ven wrote:
> On Tue, 2005-01-04 at 10:29 +0100, Leubner, Achim wrote:
> > You are right, in gdth_fill_raw_command() the driver is doing two
> > different dma mappings, one for the data buffer stored in
> > SCp.dma_handle, and the second for the sense buffer stored in SCp.buffer
> > and host_scribble. So reuse of dma_handle does not work here.
> 
> Is there a way the driver can get rid of looking at the sense buffer at
> all and let the midlayer do this ?

The driver needs to fill the sense buffer ;-)