[PATCH] lib/dma-debug: fix bucket_find_contain

[PATCH] lib/dma-debug: fix bucket_find_contain

[Sebastian Ott]

bucket_find_contain will search the bucket list for a dma_debug_entry. When
the entry isn't found it needs to search other buckets too, since only the
start address of a dma range is hashed (which might be in a different bucket).

A copy of the dma_debug_entry is used to get the previous hash bucket but when
its list is searched the original dma_debug_entry is to be used not its
modified copy.

This fixes false "device driver tries to sync DMA memory it has not allocated"
warnings.

Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
---
 lib/dma-debug.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -361,7 +361,7 @@ static struct dma_debug_entry *bucket_fi
 	unsigned int range = 0;
 
 	while (range <= max_range) {
-		entry = __hash_bucket_find(*bucket, &index, containing_match);
+		entry = __hash_bucket_find(*bucket, ref, containing_match);
 
 		if (entry)
 			return entry;

Re: [PATCH] lib/dma-debug: fix bucket_find_contain

[Andrew Morton]

On Mon, 16 Mar 2015 19:24:29 +0100 (CET) Sebastian Ott <sebott@linux.vnet.ibm.com> wrote:

> bucket_find_contain will search the bucket list for a dma_debug_entry. When
> the entry isn't found it needs to search other buckets too, since only the
> start address of a dma range is hashed (which might be in a different bucket).
> 
> A copy of the dma_debug_entry is used to get the previous hash bucket but when
> its list is searched the original dma_debug_entry is to be used not its
> modified copy.
> 
> This fixes false "device driver tries to sync DMA memory it has not allocated"
> warnings.

The changelog doesn't help me understand how serious this is.  A 4.0
thing?  -stable?

> --- a/lib/dma-debug.c
> +++ b/lib/dma-debug.c
> @@ -361,7 +361,7 @@ static struct dma_debug_entry *bucket_fi
>  	unsigned int range = 0;
>  
>  	while (range <= max_range) {
> -		entry = __hash_bucket_find(*bucket, &index, containing_match);
> +		entry = __hash_bucket_find(*bucket, ref, containing_match);
>  
>  		if (entry)
>  			return entry;

This code just got more confusing :(  Are you sure we aren't supposed to
use `ref' in bucket_find_contain()'s call to get_hash_bucket()?

Re: [PATCH] lib/dma-debug: fix bucket_find_contain

[Sebastian Ott]

On Thu, 19 Mar 2015, Andrew Morton wrote:
> On Mon, 16 Mar 2015 19:24:29 +0100 (CET) Sebastian Ott <sebott@linux.vnet.ibm.com> wrote:
> 
> > bucket_find_contain will search the bucket list for a dma_debug_entry. When
> > the entry isn't found it needs to search other buckets too, since only the
> > start address of a dma range is hashed (which might be in a different bucket).
> > 
> > A copy of the dma_debug_entry is used to get the previous hash bucket but when
> > its list is searched the original dma_debug_entry is to be used not its
> > modified copy.
> > 
> > This fixes false "device driver tries to sync DMA memory it has not allocated"
> > warnings.
> 
> The changelog doesn't help me understand how serious this is.  A 4.0
> thing?  -stable?

Hm, this is broken since quite some time and nobody noticed or cared. I
think it doesn't need to be in stable. Also without the patch you'll just
get some false positive debug_dma warnings.

> 
> > --- a/lib/dma-debug.c
> > +++ b/lib/dma-debug.c
> > @@ -361,7 +361,7 @@ static struct dma_debug_entry *bucket_fi
> >  	unsigned int range = 0;
> >  
> >  	while (range <= max_range) {
> > -		entry = __hash_bucket_find(*bucket, &index, containing_match);
> > +		entry = __hash_bucket_find(*bucket, ref, containing_match);
> >  
> >  		if (entry)
> >  			return entry;
> 
> This code just got more confusing :(  Are you sure we aren't supposed to
> use `ref' in bucket_find_contain()'s call to get_hash_bucket()?

The only caller of bucket_find_contain uses get_hash_bucket(ref).

I suspect that in most cases the dma_handle used for dma_sync will lead to
the same bucket as the handle used for dma_map (and we get a hit in the
first __hash_bucket_find).

But if a large dma mapping is created it could span more than one
bucket. So when dma_sync(X) is called it's possible we didn't find the
entry in the bucket related to X. We use index to get the previous bucket
but we still need to search this bucket for X.

When we would use get_hash_bucket(ref) again we would search the bucket
which we've already searched in the first run.

Regards,
Sebastian