[parisc-linux-cvs] [PATCH] ccio-dma cleanups and kernel documentation

[Ryan Bradetich]

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
> 

--