RE: [PATCH 1/3] 2.6.0 aacraid driver update

[Mark Haverkamp <markh@osdl.org>]

On Wed, 2003-10-08 at 06:11, Salyzyn, Mark wrote:
> The scsicmd->SCp.ptr is `stealing' an unused member of the command to hold
> on to the physical address of an additional allocation that is part of the
> request so that when the command completes the memory can be `freed'.
> 
> My recommendation is to allocate an additional structure to hold on to all
> classes of `theft' like this in the driver (my guess is there is more) and
> use the SCp.ptr to point to this allocation rather than try to `steal' more
> to make up the full length.
> 
> Any wisdom of the list with regards to this?
> 
> Sincerely -- Mark Salyzyn
> 

I looked at the scsi_pointer struct yesterday and found that it already
has a dma_handle element that is the right type.  I updated the code to
use SCp.dma_handle and removed all the casting. Does that sound OK? 
This way we don't have to alloc another struct.

Mark.

> -----Original Message-----
> From: Mark Haverkamp [mailto:markh@osdl.org]
> Sent: Tuesday, October 07, 2003 5:51 PM
> To: Matthew Wilcox
> Cc: Jeff Garzik; linux-scsi; James Bottomley; Salyzyn, Mark
> Subject: Re: [PATCH 1/3] 2.6.0 aacraid driver update
> 
> 
> On Tue, 2003-10-07 at 14:39, Matthew Wilcox wrote:
> > On Tue, Oct 07, 2003 at 02:18:12PM -0700, Mark Haverkamp wrote:
> > > > > -		pci_unmap_single(dev->pdev,
> (dma_addr_t)(ulong)scsicmd->SCp.ptr,
> > > > > +		pci_unmap_single(dev->pdev, (dma_addr_t)(unsigned
> long)scsicmd->SCp.ptr,
> > > > >  				 scsicmd->request_bufflen,
> > > > >
> scsi_to_pci_dma_dir(scsicmd->sc_data_direction));
> > 
> > > I checked out this casting today.  The function takes a dma_addr_t as
> > > the argument.  You can't just pass the pointer in as is because:
> > > 
> > > drivers/scsi/aacraid/aachba.c:690: warning: passing arg 2 of
> `pci_unmap_single'
> > > makes integer from pointer without a cast
> > > 
> > > If you just cast the pointer to dma_addr_t you get:
> > > 
> > > drivers/scsi/aacraid/aachba.c:688: warning: cast from pointer to integer
> of different size
> > > 
> > > If you just cast to unsigned long, as in the code below, there aren't
> any warnings, but
> > > the wrong type is being passed to the function.  So, I think that both
> the casts are
> > > needed.
> > 
> > Oops, that's not true.  Try it out yourself.
> > 
> > int foo(short bar);
> > 
> > int baz(void)
> > {
> > 	return foo(0x12345678);
> > }
> > 
> > will truncate the argument to foo to 0x5678 before calling it by
> > implicitly casting to a short.  So we can do just fine with:
> > 
> > -		pci_unmap_single(dev->pdev,
> (dma_addr_t)(ulong)scsicmd->SCp.ptr,
> > +		pci_unmap_single(dev->pdev, (unsigned long)scsicmd->SCp.ptr,
> >  				 scsicmd->request_bufflen,
> >
> scsi_to_pci_dma_dir(scsicmd->sc_data_direction));
> > 
> > Now that all that casting is out of the way, we can see this driver isn't
> > highmem-safe as it's storing a dma_addr_t in a pointer ... yuck.
> 
> Ok, I see now.
> 
> Thanks,
> Mark.
-- 
Mark Haverkamp <markh@osdl.org>