LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] dma-debug: Check scatterlist segments
@ 2018-04-24 16:12 Robin Murphy
  2018-04-25  4:52 ` kbuild test robot
  2018-04-25  5:58 ` Christoph Hellwig
  0 siblings, 2 replies; 4+ messages in thread
From: Robin Murphy @ 2018-04-24 16:12 UTC (permalink / raw)
  To: hch, m.szyprowski; +Cc: iommu, linux-kernel

Drivers/subsystems creating scatterlists for DMA should be taking care
to respect the scatter-gather limitations of the appropriate device, as
described by dma_parms. A DMA API implementation cannot feasibly split
a scatterlist into *more* entries than originally passed, so it is not
well defined what they should do when given a segment larger than the
limit they are also required to respect.

Conversely, devices which are less limited than the rather conservative
defaults, or indeed have no limitations at all (e.g. GPUs with their own
internal MMU), should be encouraged to set appropriate dma_parms, as
they may get more efficient DMA mapping performance out of it.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 lib/dma-debug.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 7f5cdc1e6b29..9f158941004d 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -1293,6 +1293,30 @@ static void check_sync(struct device *dev,
 	put_hash_bucket(bucket, &flags);
 }
 
+static void check_sg_segment(struct device *dev, struct scatterlist *sg)
+{
+	unsigned int max_seg = dma_get_max_seg_size(dev);
+	dma_addr_t start, end, boundary = dma_get_seg_boundary(dev);
+
+	/*
+	 * Either the driver forgot to set dma_parms appropriately, or
+	 * whoever generated the list forgot to check them.
+	 */
+	if (sg->length > max_seg)
+		err_printk(dev, NULL, "DMA-API: mapping sg segment longer than device claims to support [len=%u] [max=%u]\n",
+			   sg->length, max_seg);
+	/*
+	 * In some cases this could potentially be the DMA API
+	 * implementation's fault, but it would usually imply that
+	 * the scatterlist was built inappropriately to begin with.
+	 */
+	start = sg_dma_address(sg);
+	end = start + sg_dma_len(sg) - 1;
+	if ((start ^ end) & ~boundary)
+		err_printk(dev, NULL, "DMA-API: mapping sg segment across boundary [start=0x%016llx] [end=0x%016llx] [boundary=0x%016llx]\n",
+			   start, end, boundary);
+}
+
 void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
 			size_t size, int direction, dma_addr_t dma_addr,
 			bool map_single)
@@ -1423,6 +1447,8 @@ void debug_dma_map_sg(struct device *dev, struct scatterlist *sg,
 			check_for_illegal_area(dev, sg_virt(s), sg_dma_len(s));
 		}
 
+		check_sg_segment(dev, s);
+
 		add_dma_entry(entry);
 	}
 }
-- 
2.17.0.dirty

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] dma-debug: Check scatterlist segments
  2018-04-24 16:12 [PATCH] dma-debug: Check scatterlist segments Robin Murphy
@ 2018-04-25  4:52 ` kbuild test robot
  2018-04-25  5:58 ` Christoph Hellwig
  1 sibling, 0 replies; 4+ messages in thread
From: kbuild test robot @ 2018-04-25  4:52 UTC (permalink / raw)
  To: Robin Murphy; +Cc: kbuild-all, hch, m.szyprowski, iommu, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 7136 bytes --]

Hi Robin,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v4.17-rc2 next-20180424]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Robin-Murphy/dma-debug-Check-scatterlist-segments/20180425-070135
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sh 

All warnings (new ones prefixed by >>):

   In file included from arch/sh/include/asm/bug.h:112:0,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:12,
                    from include/asm-generic/current.h:5,
                    from ./arch/sh/include/generated/asm/current.h:1,
                    from include/linux/sched.h:12,
                    from include/linux/sched/task_stack.h:9,
                    from lib/dma-debug.c:20:
   lib/dma-debug.c: In function 'check_sg_segment':
>> lib/dma-debug.c:232:12: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 4 has type 'dma_addr_t {aka unsigned int}' [-Wformat=]
       WARN(1, "%s %s: " format,   \
               ^
   include/asm-generic/bug.h:98:50: note: in definition of macro '__WARN_printf'
    #define __WARN_printf(arg...) do { __warn_printk(arg); __WARN(); } while (0)
                                                     ^~~
>> lib/dma-debug.c:232:4: note: in expansion of macro 'WARN'
       WARN(1, "%s %s: " format,   \
       ^~~~
>> lib/dma-debug.c:1316:3: note: in expansion of macro 'err_printk'
      err_printk(dev, NULL, "DMA-API: mapping sg segment across boundary [start=0x%016llx] [end=0x%016llx] [boundary=0x%016llx]\n",
      ^~~~~~~~~~
   lib/dma-debug.c:1316:85: note: format string is defined here
      err_printk(dev, NULL, "DMA-API: mapping sg segment across boundary [start=0x%016llx] [end=0x%016llx] [boundary=0x%016llx]\n",
                                                                                  ~~~~~~^
                                                                                  %016x
   In file included from arch/sh/include/asm/bug.h:112:0,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:12,
                    from include/asm-generic/current.h:5,
                    from ./arch/sh/include/generated/asm/current.h:1,
                    from include/linux/sched.h:12,
                    from include/linux/sched/task_stack.h:9,
                    from lib/dma-debug.c:20:
   lib/dma-debug.c:232:12: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 5 has type 'dma_addr_t {aka unsigned int}' [-Wformat=]
       WARN(1, "%s %s: " format,   \
               ^
   include/asm-generic/bug.h:98:50: note: in definition of macro '__WARN_printf'
    #define __WARN_printf(arg...) do { __warn_printk(arg); __WARN(); } while (0)
                                                     ^~~
>> lib/dma-debug.c:232:4: note: in expansion of macro 'WARN'
       WARN(1, "%s %s: " format,   \
       ^~~~
>> lib/dma-debug.c:1316:3: note: in expansion of macro 'err_printk'
      err_printk(dev, NULL, "DMA-API: mapping sg segment across boundary [start=0x%016llx] [end=0x%016llx] [boundary=0x%016llx]\n",
      ^~~~~~~~~~
   lib/dma-debug.c:1316:101: note: format string is defined here
      err_printk(dev, NULL, "DMA-API: mapping sg segment across boundary [start=0x%016llx] [end=0x%016llx] [boundary=0x%016llx]\n",
                                                                                                  ~~~~~~^
                                                                                                  %016x
   In file included from arch/sh/include/asm/bug.h:112:0,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:12,
                    from include/asm-generic/current.h:5,
                    from ./arch/sh/include/generated/asm/current.h:1,
                    from include/linux/sched.h:12,
                    from include/linux/sched/task_stack.h:9,
                    from lib/dma-debug.c:20:
   lib/dma-debug.c:232:12: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 6 has type 'dma_addr_t {aka unsigned int}' [-Wformat=]
       WARN(1, "%s %s: " format,   \
               ^
   include/asm-generic/bug.h:98:50: note: in definition of macro '__WARN_printf'
    #define __WARN_printf(arg...) do { __warn_printk(arg); __WARN(); } while (0)
                                                     ^~~
>> lib/dma-debug.c:232:4: note: in expansion of macro 'WARN'
       WARN(1, "%s %s: " format,   \
       ^~~~
>> lib/dma-debug.c:1316:3: note: in expansion of macro 'err_printk'
      err_printk(dev, NULL, "DMA-API: mapping sg segment across boundary [start=0x%016llx] [end=0x%016llx] [boundary=0x%016llx]\n",
      ^~~~~~~~~~
   lib/dma-debug.c:1316:122: note: format string is defined here
      err_printk(dev, NULL, "DMA-API: mapping sg segment across boundary [start=0x%016llx] [end=0x%016llx] [boundary=0x%016llx]\n",
                                                                                                                       ~~~~~~^
                                                                                                                       %016x

vim +232 lib/dma-debug.c

2e507d84 Joerg Roedel    2009-05-22  227  
6c132d1b David Woodhouse 2009-01-19  228  #define err_printk(dev, entry, format, arg...) do {			\
2d62ece1 Joerg Roedel    2009-01-09  229  		error_count += 1;					\
2e507d84 Joerg Roedel    2009-05-22  230  		if (driver_filter(dev) &&				\
2e507d84 Joerg Roedel    2009-05-22  231  		    (show_all_errors || show_num_errors > 0)) {		\
2d62ece1 Joerg Roedel    2009-01-09 @232  			WARN(1, "%s %s: " format,			\
ec9c96ef Kyle McMartin   2009-08-19  233  			     dev ? dev_driver_string(dev) : "NULL",	\
ec9c96ef Kyle McMartin   2009-08-19  234  			     dev ? dev_name(dev) : "NULL", ## arg);	\
6c132d1b David Woodhouse 2009-01-19  235  			dump_entry_trace(entry);			\
2d62ece1 Joerg Roedel    2009-01-09  236  		}							\
2d62ece1 Joerg Roedel    2009-01-09  237  		if (!show_all_errors && show_num_errors > 0)		\
2d62ece1 Joerg Roedel    2009-01-09  238  			show_num_errors -= 1;				\
2d62ece1 Joerg Roedel    2009-01-09  239  	} while (0);
2d62ece1 Joerg Roedel    2009-01-09  240  

:::::: The code at line 232 was first introduced by commit
:::::: 2d62ece14fe04168a7d16688ddd2d17ac472268c dma-debug: add core checking functions

:::::: TO: Joerg Roedel <joerg.roedel@amd.com>
:::::: CC: Joerg Roedel <joerg.roedel@amd.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 47757 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] dma-debug: Check scatterlist segments
  2018-04-24 16:12 [PATCH] dma-debug: Check scatterlist segments Robin Murphy
  2018-04-25  4:52 ` kbuild test robot
@ 2018-04-25  5:58 ` Christoph Hellwig
  2018-05-08 10:14   ` Robin Murphy
  1 sibling, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2018-04-25  5:58 UTC (permalink / raw)
  To: Robin Murphy; +Cc: hch, m.szyprowski, iommu, linux-kernel

This looks interesting.  I suspect it is going to blow up in
quite a few places, so maybe at least for now it might make sense
to have a separate config option?

On Tue, Apr 24, 2018 at 05:12:19PM +0100, Robin Murphy wrote:
> Drivers/subsystems creating scatterlists for DMA should be taking care
> to respect the scatter-gather limitations of the appropriate device, as
> described by dma_parms. A DMA API implementation cannot feasibly split
> a scatterlist into *more* entries than originally passed, so it is not
> well defined what they should do when given a segment larger than the
> limit they are also required to respect.
> 
> Conversely, devices which are less limited than the rather conservative
> defaults, or indeed have no limitations at all (e.g. GPUs with their own
> internal MMU), should be encouraged to set appropriate dma_parms, as
> they may get more efficient DMA mapping performance out of it.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  lib/dma-debug.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> index 7f5cdc1e6b29..9f158941004d 100644
> --- a/lib/dma-debug.c
> +++ b/lib/dma-debug.c
> @@ -1293,6 +1293,30 @@ static void check_sync(struct device *dev,
>  	put_hash_bucket(bucket, &flags);
>  }
>  
> +static void check_sg_segment(struct device *dev, struct scatterlist *sg)
> +{
> +	unsigned int max_seg = dma_get_max_seg_size(dev);
> +	dma_addr_t start, end, boundary = dma_get_seg_boundary(dev);
> +
> +	/*
> +	 * Either the driver forgot to set dma_parms appropriately, or
> +	 * whoever generated the list forgot to check them.
> +	 */
> +	if (sg->length > max_seg)
> +		err_printk(dev, NULL, "DMA-API: mapping sg segment longer than device claims to support [len=%u] [max=%u]\n",
> +			   sg->length, max_seg);
> +	/*
> +	 * In some cases this could potentially be the DMA API
> +	 * implementation's fault, but it would usually imply that
> +	 * the scatterlist was built inappropriately to begin with.
> +	 */
> +	start = sg_dma_address(sg);
> +	end = start + sg_dma_len(sg) - 1;
> +	if ((start ^ end) & ~boundary)
> +		err_printk(dev, NULL, "DMA-API: mapping sg segment across boundary [start=0x%016llx] [end=0x%016llx] [boundary=0x%016llx]\n",
> +			   start, end, boundary);
> +}
> +
>  void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
>  			size_t size, int direction, dma_addr_t dma_addr,
>  			bool map_single)
> @@ -1423,6 +1447,8 @@ void debug_dma_map_sg(struct device *dev, struct scatterlist *sg,
>  			check_for_illegal_area(dev, sg_virt(s), sg_dma_len(s));
>  		}
>  
> +		check_sg_segment(dev, s);
> +
>  		add_dma_entry(entry);
>  	}
>  }
> -- 
> 2.17.0.dirty
---end quoted text---

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] dma-debug: Check scatterlist segments
  2018-04-25  5:58 ` Christoph Hellwig
@ 2018-05-08 10:14   ` Robin Murphy
  0 siblings, 0 replies; 4+ messages in thread
From: Robin Murphy @ 2018-05-08 10:14 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: m.szyprowski, iommu, linux-kernel

On 25/04/18 06:58, Christoph Hellwig wrote:
> This looks interesting.  I suspect it is going to blow up in
> quite a few places, so maybe at least for now it might make sense
> to have a separate config option?

True, it's nice to verify this for 'traditional' dma_map_sg() usage, but 
places where it's just used as an intermediate shorthand for "prepare 
all these pages for arbitrary future DMA" are liable to be noisy with 
false-positives for not respecting the default values when they arguably 
don't matter. I'll respin this as an additional config under 
DMA_API_DEBUG, and then we can debate "default y" or not.

Robin.

> On Tue, Apr 24, 2018 at 05:12:19PM +0100, Robin Murphy wrote:
>> Drivers/subsystems creating scatterlists for DMA should be taking care
>> to respect the scatter-gather limitations of the appropriate device, as
>> described by dma_parms. A DMA API implementation cannot feasibly split
>> a scatterlist into *more* entries than originally passed, so it is not
>> well defined what they should do when given a segment larger than the
>> limit they are also required to respect.
>>
>> Conversely, devices which are less limited than the rather conservative
>> defaults, or indeed have no limitations at all (e.g. GPUs with their own
>> internal MMU), should be encouraged to set appropriate dma_parms, as
>> they may get more efficient DMA mapping performance out of it.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   lib/dma-debug.c | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
>> index 7f5cdc1e6b29..9f158941004d 100644
>> --- a/lib/dma-debug.c
>> +++ b/lib/dma-debug.c
>> @@ -1293,6 +1293,30 @@ static void check_sync(struct device *dev,
>>   	put_hash_bucket(bucket, &flags);
>>   }
>>   
>> +static void check_sg_segment(struct device *dev, struct scatterlist *sg)
>> +{
>> +	unsigned int max_seg = dma_get_max_seg_size(dev);
>> +	dma_addr_t start, end, boundary = dma_get_seg_boundary(dev);
>> +
>> +	/*
>> +	 * Either the driver forgot to set dma_parms appropriately, or
>> +	 * whoever generated the list forgot to check them.
>> +	 */
>> +	if (sg->length > max_seg)
>> +		err_printk(dev, NULL, "DMA-API: mapping sg segment longer than device claims to support [len=%u] [max=%u]\n",
>> +			   sg->length, max_seg);
>> +	/*
>> +	 * In some cases this could potentially be the DMA API
>> +	 * implementation's fault, but it would usually imply that
>> +	 * the scatterlist was built inappropriately to begin with.
>> +	 */
>> +	start = sg_dma_address(sg);
>> +	end = start + sg_dma_len(sg) - 1;
>> +	if ((start ^ end) & ~boundary)
>> +		err_printk(dev, NULL, "DMA-API: mapping sg segment across boundary [start=0x%016llx] [end=0x%016llx] [boundary=0x%016llx]\n",
>> +			   start, end, boundary);
>> +}
>> +
>>   void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
>>   			size_t size, int direction, dma_addr_t dma_addr,
>>   			bool map_single)
>> @@ -1423,6 +1447,8 @@ void debug_dma_map_sg(struct device *dev, struct scatterlist *sg,
>>   			check_for_illegal_area(dev, sg_virt(s), sg_dma_len(s));
>>   		}
>>   
>> +		check_sg_segment(dev, s);
>> +
>>   		add_dma_entry(entry);
>>   	}
>>   }
>> -- 
>> 2.17.0.dirty
> ---end quoted text---
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-05-08 10:14 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-24 16:12 [PATCH] dma-debug: Check scatterlist segments Robin Murphy
2018-04-25  4:52 ` kbuild test robot
2018-04-25  5:58 ` Christoph Hellwig
2018-05-08 10:14   ` Robin Murphy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).