[parisc-linux-cvs] [PATCH] ccio-dma cleanups and kernel documentation
Ryan Bradetich
rbrad@beavis.ybsoft.com
Wed, 29 Aug 2001 20:13:14 -0600
On Wed, Aug 29, 2001 at 03:00:00PM -0600, Grant Grundler wrote:
>
> Ryan Bradetich wrote:
> > static int
> > -ccio_alloc_range(struct ioc *ioc, size_t size)
> > +ccio_alloc_range(struct ioc *ioc, unsigned long pages_needed)
>
> This change has me nervous.
Grant,
I made this change because I felt it made the code more clear. Let
me explain my reasoning, and if you're still nervous lets discuss it
further :)
Currently, the ccio_alloc_range (and the sba_alloc_range) makes the
implicit assumption that size will be aligned on a page boundry. The
following ASSERT at the top of the ccio_alloc_range/sba_alloc_range
function enforces the page alignment:
ASSERT(0 == (size & ~IOVP_MASK));
When I was auditing the code, I thought we had missed this corner case,
until I examined the function calls closely and realized the implicit
assumption. After doing this research, I decided to change the function
argument to be more descriptive of the implicit assumption.
> > - idx = ccio_alloc_range(ioc, size);
> > + idx = ccio_alloc_range(ioc, (size >> IOVP_SHIFT));
>
> I didn't walk through the code but it seems like we lost a corner case
> of when an address starts on something other than a page boundary
> and crosses the next page boundary.
Approx 4-5 lines above this call, we round the size upto the nearest
page boundry with the following macro:
/* round up to nearest IOVP_SIZE */
size = ROUNDUP(size + offset, IOVP_SIZE);
The same with the ccio_alloc_range() call shown below:
dma_len = ROUNDUP(dma_len + dma_offset, IOVP_SIZE);
> > - | (ccio_alloc_range(ioc, dma_len) << IOVP_SHIFT)
> > + | (ccio_alloc_range(ioc, (dma_len >> IOVP_SHIFT)) << IO
> > VP_SHIFT)
>
> This should just be a mask operation (drop lower bits).
hmm... you sure? I added the >> IOVP_SHIFT to convert the number of bytes to
the number of pages_needed. The << IOVP_SHIFT for the entire ccio_alloc_range()
was not changed.
This is from ccio-dma.c revision 1.37
sg_dma_address(dma_sg) =
PIDE_FLAG
| (ccio_alloc_range(ioc, dma_len) << IOVP_SHIFT)
| dma_offset;
This is from ccio-dma.c revision 1.38 (currently the latest)
sg_dma_address(dma_sg) =
PIDE_FLAG
| (ccio_alloc_range(ioc, (dma_len >> IOVP_SHIFT)) << IOVP_SHIFT)
| dma_offset;
I do not see the error, but I've been wrong before ... and another set of eyes is
always a good thing! Thanks for taking time to review the patch!
- Ryan
> grant
>
> Grant Grundler
> parisc-linux {PCI|IOMMU|SMP} hacker
> +1.408.447.7253
>
--