Re: [PATCH v3] dma-debug: New interfaces to debug dma mapping errors

[Konrad Rzeszutek Wilk]

On Wed, Oct 03, 2012 at 02:45:11PM -0700, Andrew Morton wrote:
> On Wed, 03 Oct 2012 08:55:59 -0600
> Shuah Khan <shuah.khan@xxxxxx> wrote:
> 
> > A recent dma mapping error analysis effort showed that a large percentage
> > of dma_map_single() and dma_map_page() returns are not checked for mapping
> > errors.
> > 
> > Reference:
> > http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis
> > 
> > Adding support for tracking dma mapping and unmapping errors to help assess
> > the following:
> > 
> > When do dma mapping errors get detected?
> > How often do these errors occur?
> > Why don't we see failures related to missing dma mapping error checks?
> > Are they silent failures?
> 
> This seems to be a strange way of addressing kernel programming errors.
> Instead of fixing them up, we generate lots of statistics about how
> often they happen!

And by using this we can fix the drivers.
> 
> Would it not be better to find and fix the buggy code sites?  A
> coccinelle script wold probably help here.

That is the end goal (fixing the buggy code sites). Shuah has
identified the bad culprits.

> 
> And let's also look at *why* we keep doing this.  Partly it's because
> these things are self-propagating - people copy-n-paste bad code so we
> get more bad code.


And this patch will now tell the poor engineers that write new code that
they pasted bad code.

> 
> 
> Another reason surely is the poor documentation.  Suppose our diligent
> programmer goes to the dma_map_single() definition site:
> 
> #define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, NULL)
> 
> No documentation at all.  Because it's a stupid macro it doesn't even
> give the types and names of the arguments or the type of the return
> value.
> 
> So he goes to dma_map_single_attrs() and finds that is altogether
> undocmented.
> 
> So he goes into Documentation/DMA-API-HOWTO.txt, searches for
> "dma_map_single" and finds
> 
> : To map a single region, you do:
> : 
> : 	struct device *dev = &my_dev->dev;
> : 	dma_addr_t dma_handle;
> : 	void *addr = buffer->ptr;
> : 	size_t size = buffer->len;
> : 
> : 	dma_handle = dma_map_single(dev, addr, size, direction);
> : 
> : and to unmap it:
> : 
> : 	dma_unmap_single(dev, dma_handle, size, direction);
> 
> 
> So it is hardly surprising that we keep screwing this up!

Right, so that should be also modified (Thank you for looking at that)!

> --
> 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/
> 
--
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/