[PATCH 2/4] x86-64: Calgary IOMMU - move valid_dma_direction into the callers

[PATCH 2/4] x86-64: Calgary IOMMU - move valid_dma_direction into the callers

[Jon Mason]

>From Andi Kleen's comments on the original Calgary patch, move
valid_dma_direction into the calling functions.

Signed-off-by: Muli Ben-Yehuda <muli@il.ibm.com>
Signed-off-by: Jon Mason <jdmason@us.ibm.com>

diff -r 460b603caa42 include/asm-x86_64/dma-mapping.h
--- a/include/asm-x86_64/dma-mapping.h	Wed May 24 10:58:12 2006 -0500
+++ b/include/asm-x86_64/dma-mapping.h	Wed May 24 11:01:01 2006 -0500
@@ -56,6 +56,13 @@ extern struct dma_mapping_ops* dma_ops;
 extern struct dma_mapping_ops* dma_ops;
 extern int iommu_merge;
 
+static inline int valid_dma_direction(int dma_direction)
+{
+	return ((dma_direction == DMA_BIDIRECTIONAL) ||
+		(dma_direction == DMA_TO_DEVICE) ||
+		(dma_direction == DMA_FROM_DEVICE));
+}
+
 static inline int dma_mapping_error(dma_addr_t dma_addr)
 {
 	if (dma_ops->mapping_error)
@@ -73,6 +80,7 @@ dma_map_single(struct device *hwdev, voi
 dma_map_single(struct device *hwdev, void *ptr, size_t size,
 	       int direction)
 {
+	BUG_ON(!valid_dma_direction(direction));
 	return dma_ops->map_single(hwdev, ptr, size, direction);
 }
 
@@ -80,6 +88,7 @@ dma_unmap_single(struct device *dev, dma
 dma_unmap_single(struct device *dev, dma_addr_t addr,size_t size,
 		 int direction)
 {
+	BUG_ON(!valid_dma_direction(direction));
 	dma_ops->unmap_single(dev, addr, size, direction);
 }
 
@@ -92,6 +101,7 @@ dma_sync_single_for_cpu(struct device *h
 dma_sync_single_for_cpu(struct device *hwdev, dma_addr_t dma_handle,
 			size_t size, int direction)
 {
+	BUG_ON(!valid_dma_direction(direction));
 	if (dma_ops->sync_single_for_cpu)
 		dma_ops->sync_single_for_cpu(hwdev, dma_handle, size,
 					     direction);
@@ -102,6 +112,7 @@ dma_sync_single_for_device(struct device
 dma_sync_single_for_device(struct device *hwdev, dma_addr_t dma_handle,
 			   size_t size, int direction)
 {
+	BUG_ON(!valid_dma_direction(direction));
 	if (dma_ops->sync_single_for_device)
 		dma_ops->sync_single_for_device(hwdev, dma_handle, size,
 						direction);
@@ -112,6 +123,7 @@ dma_sync_single_range_for_cpu(struct dev
 dma_sync_single_range_for_cpu(struct device *hwdev, dma_addr_t dma_handle,
 			      unsigned long offset, size_t size, int direction)
 {
+	BUG_ON(!valid_dma_direction(direction));
 	if (dma_ops->sync_single_range_for_cpu) {
 		dma_ops->sync_single_range_for_cpu(hwdev, dma_handle, offset, size, direction);
 	}
@@ -123,6 +135,7 @@ dma_sync_single_range_for_device(struct 
 dma_sync_single_range_for_device(struct device *hwdev, dma_addr_t dma_handle,
 				 unsigned long offset, size_t size, int direction)
 {
+	BUG_ON(!valid_dma_direction(direction));
 	if (dma_ops->sync_single_range_for_device)
 		dma_ops->sync_single_range_for_device(hwdev, dma_handle,
 						      offset, size, direction);
@@ -134,6 +147,7 @@ dma_sync_sg_for_cpu(struct device *hwdev
 dma_sync_sg_for_cpu(struct device *hwdev, struct scatterlist *sg,
 		    int nelems, int direction)
 {
+	BUG_ON(!valid_dma_direction(direction));
 	if (dma_ops->sync_sg_for_cpu)
 		dma_ops->sync_sg_for_cpu(hwdev, sg, nelems, direction);
 	flush_write_buffers();
@@ -143,6 +157,7 @@ dma_sync_sg_for_device(struct device *hw
 dma_sync_sg_for_device(struct device *hwdev, struct scatterlist *sg,
 		       int nelems, int direction)
 {
+	BUG_ON(!valid_dma_direction(direction));
 	if (dma_ops->sync_sg_for_device) {
 		dma_ops->sync_sg_for_device(hwdev, sg, nelems, direction);
 	}
@@ -153,6 +168,7 @@ static inline int
 static inline int
 dma_map_sg(struct device *hwdev, struct scatterlist *sg, int nents, int direction)
 {
+	BUG_ON(!valid_dma_direction(direction));
 	return dma_ops->map_sg(hwdev, sg, nents, direction);
 }
 
@@ -160,6 +176,7 @@ dma_unmap_sg(struct device *hwdev, struc
 dma_unmap_sg(struct device *hwdev, struct scatterlist *sg, int nents,
 	     int direction)
 {
+	BUG_ON(!valid_dma_direction(direction));
 	dma_ops->unmap_sg(hwdev, sg, nents, direction);
 }
 

Re: [PATCH 2/4] x86-64: Calgary IOMMU - move valid_dma_direction into the callers

[Jeff Garzik]

Jon Mason wrote:
>>From Andi Kleen's comments on the original Calgary patch, move
> valid_dma_direction into the calling functions.
> 
> Signed-off-by: Muli Ben-Yehuda <muli@il.ibm.com>
> Signed-off-by: Jon Mason <jdmason@us.ibm.com>

Even though BUG_ON() includes unlikely(), this introduces additional 
tests in very hot paths.

_Why_ do we need this at all?

I would prefer to NAK the patch, and fix the odd user that gets it 
wrong.  It becomes REALLY obvious that a driver has gotten this wrong, 
REALLY quickly.

I see no need to burden critical hot paths with dumb checks like this.

	Jeff

Re: [PATCH 2/4] x86-64: Calgary IOMMU - move valid_dma_direction into the callers

[Muli Ben-Yehuda]

On Thu, May 25, 2006 at 12:35:07AM -0400, Jeff Garzik wrote:
> Jon Mason wrote:
> >>From Andi Kleen's comments on the original Calgary patch, move
> >valid_dma_direction into the calling functions.
> >
> >Signed-off-by: Muli Ben-Yehuda <muli@il.ibm.com>
> >Signed-off-by: Jon Mason <jdmason@us.ibm.com>
> 
> Even though BUG_ON() includes unlikely(), this introduces additional 
> tests in very hot paths.

Are they really very hot? I mean if you're calling the DMA API, you're
about to frob the hardware or have already frobbed it - does this
check really matter?

> _Why_ do we need this at all?

It was helpful for us during the dma-ops work and Calgary bringup and
Andi requested that we move it from Calgary to common code. I think
we're fine with dropping it if that's the consensus, but it did catch
a few bugs early on and the cost is tiny.

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

Re: [PATCH 2/4] x86-64: Calgary IOMMU - move valid_dma_direction into the callers

[Jeff Garzik]

Muli Ben-Yehuda wrote:
> On Thu, May 25, 2006 at 12:35:07AM -0400, Jeff Garzik wrote:
>> Jon Mason wrote:
>>> >From Andi Kleen's comments on the original Calgary patch, move
>>> valid_dma_direction into the calling functions.
>>>
>>> Signed-off-by: Muli Ben-Yehuda <muli@il.ibm.com>
>>> Signed-off-by: Jon Mason <jdmason@us.ibm.com>
>> Even though BUG_ON() includes unlikely(), this introduces additional 
>> tests in very hot paths.
> 
> Are they really very hot? I mean if you're calling the DMA API, you're
> about to frob the hardware or have already frobbed it - does this
> check really matter?

When you are adding a check that will _never_ be hit in production, to 
the _hottest_ paths in the kernel, you can be assured it matters...


>> _Why_ do we need this at all?
> 
> It was helpful for us during the dma-ops work and Calgary bringup and
> Andi requested that we move it from Calgary to common code. I think
> we're fine with dropping it if that's the consensus, but it did catch
> a few bugs early on and the cost is tiny.

Key phrase:  "early on"

These checks will only be useful during _early_ development of a new DMA 
platform.  For _100%_ of the real world users, these checks are useless. 
  Not 99%, 100%.

	Jeff

Re: [discuss] Re: [PATCH 2/4] x86-64: Calgary IOMMU - move valid_dma_direction into the callers

[Andi Kleen]

On Thursday 25 May 2006 11:58, Jeff Garzik wrote:
> Muli Ben-Yehuda wrote:
> > On Thu, May 25, 2006 at 12:35:07AM -0400, Jeff Garzik wrote:
> >> Jon Mason wrote:
> >>> >From Andi Kleen's comments on the original Calgary patch, move
> >>> valid_dma_direction into the calling functions.
> >>>
> >>> Signed-off-by: Muli Ben-Yehuda <muli@il.ibm.com>
> >>> Signed-off-by: Jon Mason <jdmason@us.ibm.com>
> >> Even though BUG_ON() includes unlikely(), this introduces additional 
> >> tests in very hot paths.
> > 
> > Are they really very hot? I mean if you're calling the DMA API, you're
> > about to frob the hardware or have already frobbed it - does this
> > check really matter?
> 
> When you are adding a check that will _never_ be hit in production, to 
> the _hottest_ paths in the kernel, you can be assured it matters...

pci_dma_* shouldn't be that hot. Or at least IO usually has so much
overhead that some more bugging shouldn't matter.

On the other hand if the problem of passing wrong parameters here
isn't common I would be ok with dropping them.

-Andi

Re: [discuss] Re: [PATCH 2/4] x86-64: Calgary IOMMU - move valid_dma_direction into the callers

[Jeff Garzik]

Andi Kleen wrote:
> On Thursday 25 May 2006 11:58, Jeff Garzik wrote:
>> Muli Ben-Yehuda wrote:
>>> On Thu, May 25, 2006 at 12:35:07AM -0400, Jeff Garzik wrote:
>>>> Jon Mason wrote:
>>>>> >From Andi Kleen's comments on the original Calgary patch, move
>>>>> valid_dma_direction into the calling functions.
>>>>>
>>>>> Signed-off-by: Muli Ben-Yehuda <muli@il.ibm.com>
>>>>> Signed-off-by: Jon Mason <jdmason@us.ibm.com>
>>>> Even though BUG_ON() includes unlikely(), this introduces additional 
>>>> tests in very hot paths.
>>> Are they really very hot? I mean if you're calling the DMA API, you're
>>> about to frob the hardware or have already frobbed it - does this
>>> check really matter?
>> When you are adding a check that will _never_ be hit in production, to 
>> the _hottest_ paths in the kernel, you can be assured it matters...
> 
> pci_dma_* shouldn't be that hot. Or at least IO usually has so much
> overhead that some more bugging shouldn't matter.

I respectfully disagree with that logic.  If its a key hot path -- which 
it is, every modern network or disk I/O runs through these paths -- then 
it deserves at least _some_ consideration before adding more CPU cycles.


> On the other hand if the problem of passing wrong parameters here
> isn't common I would be ok with dropping them.

As the author noted, it was only used in early platform bring-up.  And 
simply reviewing the patch... it is clear that screwing up the 
parameters would cause massive, noticeable problems immediately -- such 
as on EM64T with swiotlb.

	Jeff

Re: [discuss] Re: [PATCH 2/4] x86-64: Calgary IOMMU - move valid_dma_direction into the callers

[Andi Kleen]

> > On the other hand if the problem of passing wrong parameters here
> > isn't common I would be ok with dropping them.
> 
> As the author noted, it was only used in early platform bring-up.  And 
> simply reviewing the patch... it is clear that screwing up the 
> parameters would cause massive, noticeable problems immediately -- such 
> as on EM64T with swiotlb.

Well the point is that even if you don't run on swiotlb
(which most people even on EM64T don't use because they don't have more than
3GB of RAM) you won't screw them up.

On the other hand correct IOMMU operation needs much more than passing
these parameters so only checking for this might not be very useful.

BTW we've had these checks forever - Jon just moved them to a different place.
But they indeed might have outlived their usefulness.

-Andi