Re: [PATCH 2.6.0] megaraid 64bit fix/cleanup (AMD64)

[Jeff Garzik]

Brad House wrote:
> Hmm, I don't think this driver is as complex as others may
> be to port.  But then again, I'm probably wrong b/c I'm mainly
> a userland guy, not a kernel guy :/
> But, nonetheless, I've made some changes:

The only thing that matters is the hardware s/g list capabilities. 
Driver complexity is irrelevant.


> > > 			/* Calculate Scatter-Gather info */
> > > 			mbox->m_out.numsgelements = mega_build_sglist(adapter, scb,
> > > -					(u32 *)&mbox->m_out.xferaddr, (u32 *)&seg);
> > > +					(dma_addr_t *)&mbox->m_out.xferaddr, (u32 *)&seg);
> >
> > Casting just hides a bug.
>
>
> Well, the  xferaddr is actually a dma_addr_t now, so that cast really
> does nothing, the only reason it's there is because they previously
> casted it as (u32 *).  I removed the cast just so it couldn't obscure
> warnings, and it didn't.
>
> Also, I use  dma_addr_t even though it may have nothing to do with dma
> where it's used. I'm more familiar with userland stuff, so I wasn't sure
> what to use. In userland I'd use  uintptr_t.
>
>
> > The real fix is to pass a full 64-bit address
>
>
> I did find a few instances where they recast the addresses,
> which was improper, but it does appear that the original address
> in the original driver was coming in as 64bit (dma_addr_t as originally
> written), but were passing it around and casting it as a u32, so I
> think the interfaces allowed for it to work, they just wrote it
> unware of 64bit systems.
>
> Also, they tried to stuff the address returned here :
> ext_inq = pci_alloc_consistent(adapter->dev,
>                                 sizeof(mraid_ext_inquiry), &dma_handle);
> (the dma_handle) into a u32 which I just fixed.
>
>
> > into the s/g list, if it supports 64-bit addresses.  if it doesn't, you
> > need to make sure the driver doesn't set highmem_io, make sure the
> > driver doesn't set a 64-bit DMA mask, and make sure the driver does set
> > a 32-bit DMA mask.
>
>
> The driver already does this it appears, without me needing to do it,
> Part of which is covered by this:
>  /* Set the Mode of addressing to 64 bit if we can */
>                 if((adapter->flag & BOARD_64BIT)&&(sizeof(dma_addr_t) ==
> 8)) {
>                         pci_set_dma_mask(pdev, 0xffffffffffffffffULL);
>                         adapter->has_64bit_addr = 1;
>                 }
>                 else  {
>                         pci_set_dma_mask(pdev, 0xffffffff);
>                         adapter->has_64bit_addr = 0;
>                 }

This code is completely bogus and wrong (not your fault, but still). 
Take a look at tg3.c for the right way to do it.

The driver continues to have obvious 64-bit issues that your patch 
doesn't address.  Your main test platform should really be a 32-bit 
system with PAE, and >4GB of RAM.

	Jeff



-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/