Re: [PATCH v3 2/3] dma-mapping: replace zone_dma_bits by zone_dma_limit

Re: [PATCH v3 2/3] dma-mapping: replace zone_dma_bits by zone_dma_limit

[kernel test robot]

Hi Baruch,

kernel test robot noticed the following build warnings:

[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on powerpc/next powerpc/fixes s390/features linus/master v6.11-rc1 next-20240729]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Baruch-Siach/dma-mapping-improve-DMA-zone-selection/20240729-211018
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
patch link:    https://lore.kernel.org/r/053fa4806a2c63efcde80caca473a8b670a2701c.1722249878.git.baruch%40tkos.co.il
patch subject: [PATCH v3 2/3] dma-mapping: replace zone_dma_bits by zone_dma_limit
config: arm-allnoconfig (https://download.01.org/0day-ci/archive/20240730/202407300338.oaUo6jtB-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project ccae7b461be339e717d02f99ac857cf0bc7d17fc)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240730/202407300338.oaUo6jtB-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407300338.oaUo6jtB-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from kernel/dma/direct.c:7:
   In file included from include/linux/memblock.h:12:
   In file included from include/linux/mm.h:2253:
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> kernel/dma/direct.c:23:46: warning: implicit conversion from 'unsigned long long' to 'phys_addr_t' (aka 'unsigned int') changes value from 18446744073709551615 to 4294967295 [-Wconstant-conversion]
      23 | phys_addr_t zone_dma_limit __ro_after_init = DMA_BIT_MASK(24);
         |             ~~~~~~~~~~~~~~                   ^~~~~~~~~~~~~~~~
   include/linux/dma-mapping.h:77:40: note: expanded from macro 'DMA_BIT_MASK'
      77 | #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
         |                                        ^~~~~
   2 warnings generated.


vim +23 kernel/dma/direct.c

   > 7	#include <linux/memblock.h>
     8	#include <linux/export.h>
     9	#include <linux/mm.h>
    10	#include <linux/dma-map-ops.h>
    11	#include <linux/scatterlist.h>
    12	#include <linux/pfn.h>
    13	#include <linux/vmalloc.h>
    14	#include <linux/set_memory.h>
    15	#include <linux/slab.h>
    16	#include "direct.h"
    17	
    18	/*
    19	 * Most architectures use ZONE_DMA for the first 16 Megabytes, but some use
    20	 * it for entirely different regions. In that case the arch code needs to
    21	 * override the variable below for dma-direct to work properly.
    22	 */
  > 23	phys_addr_t zone_dma_limit __ro_after_init = DMA_BIT_MASK(24);
    24	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 2/3] dma-mapping: replace zone_dma_bits by zone_dma_limit

[Nathan Chancellor]

On Tue, Jul 30, 2024 at 04:20:51AM +0800, kernel test robot wrote:
> Hi Baruch,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on arm64/for-next/core]
> [also build test WARNING on powerpc/next powerpc/fixes s390/features linus/master v6.11-rc1 next-20240729]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Baruch-Siach/dma-mapping-improve-DMA-zone-selection/20240729-211018
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
> patch link:    https://lore.kernel.org/r/053fa4806a2c63efcde80caca473a8b670a2701c.1722249878.git.baruch%40tkos.co.il
> patch subject: [PATCH v3 2/3] dma-mapping: replace zone_dma_bits by zone_dma_limit
> config: arm-allnoconfig (https://download.01.org/0day-ci/archive/20240730/202407300338.oaUo6jtB-lkp@intel.com/config)
> compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project ccae7b461be339e717d02f99ac857cf0bc7d17fc)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240730/202407300338.oaUo6jtB-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202407300338.oaUo6jtB-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
>    In file included from kernel/dma/direct.c:7:
>    In file included from include/linux/memblock.h:12:
>    In file included from include/linux/mm.h:2253:
>    include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
>      514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
>          |                               ~~~~~~~~~~~ ^ ~~~
> >> kernel/dma/direct.c:23:46: warning: implicit conversion from 'unsigned long long' to 'phys_addr_t' (aka 'unsigned int') changes value from 18446744073709551615 to 4294967295 [-Wconstant-conversion]
>       23 | phys_addr_t zone_dma_limit __ro_after_init = DMA_BIT_MASK(24);
>          |             ~~~~~~~~~~~~~~                   ^~~~~~~~~~~~~~~~
>    include/linux/dma-mapping.h:77:40: note: expanded from macro 'DMA_BIT_MASK'
>       77 | #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
>          |                                        ^~~~~
>    2 warnings generated.

FWIW, this is likely a false positive due to an issue in Clang with the
control flow graph for global variables:

https://github.com/ClangBuiltLinux/linux/issues/92

DMA_BIT_MASK() has been the biggest offender :/ If there is any way to
refactor this code to avoid this, that would be great (as that has been
one of our longest outstanding issues and getting it fixed in the
compiler does not seem super easy at this point).

Cheers,
Nathan

> vim +23 kernel/dma/direct.c
> 
>    > 7	#include <linux/memblock.h>
>      8	#include <linux/export.h>
>      9	#include <linux/mm.h>
>     10	#include <linux/dma-map-ops.h>
>     11	#include <linux/scatterlist.h>
>     12	#include <linux/pfn.h>
>     13	#include <linux/vmalloc.h>
>     14	#include <linux/set_memory.h>
>     15	#include <linux/slab.h>
>     16	#include "direct.h"
>     17	
>     18	/*
>     19	 * Most architectures use ZONE_DMA for the first 16 Megabytes, but some use
>     20	 * it for entirely different regions. In that case the arch code needs to
>     21	 * override the variable below for dma-direct to work properly.
>     22	 */
>   > 23	phys_addr_t zone_dma_limit __ro_after_init = DMA_BIT_MASK(24);
>     24	
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
> 

Re: [PATCH v3 2/3] dma-mapping: replace zone_dma_bits by zone_dma_limit

[Christoph Hellwig]

On Mon, Jul 29, 2024 at 07:12:08PM -0700, Nathan Chancellor wrote:
> >          |             ~~~~~~~~~~~~~~                   ^~~~~~~~~~~~~~~~
> >    include/linux/dma-mapping.h:77:40: note: expanded from macro 'DMA_BIT_MASK'
> >       77 | #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
> >          |                                        ^~~~~
> >    2 warnings generated.
> 
> FWIW, this is likely a false positive due to an issue in Clang with the
> control flow graph for global variables:
> 
> https://github.com/ClangBuiltLinux/linux/issues/92
> 
> DMA_BIT_MASK() has been the biggest offender :/ If there is any way to
> refactor this code to avoid this, that would be great (as that has been
> one of our longest outstanding issues and getting it fixed in the
> compiler does not seem super easy at this point).

I have no idea what you'd want changed here, but I'll happily take
patches.

Re: [PATCH v3 2/3] dma-mapping: replace zone_dma_bits by zone_dma_limit

[Nathan Chancellor]

On Tue, Jul 30, 2024 at 05:34:50PM +0200, Christoph Hellwig wrote:
> On Mon, Jul 29, 2024 at 07:12:08PM -0700, Nathan Chancellor wrote:
> > >          |             ~~~~~~~~~~~~~~                   ^~~~~~~~~~~~~~~~
> > >    include/linux/dma-mapping.h:77:40: note: expanded from macro 'DMA_BIT_MASK'
> > >       77 | #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
> > >          |                                        ^~~~~
> > >    2 warnings generated.
> > 
> > FWIW, this is likely a false positive due to an issue in Clang with the
> > control flow graph for global variables:
> > 
> > https://github.com/ClangBuiltLinux/linux/issues/92
> > 
> > DMA_BIT_MASK() has been the biggest offender :/ If there is any way to
> > refactor this code to avoid this, that would be great (as that has been
> > one of our longest outstanding issues and getting it fixed in the
> > compiler does not seem super easy at this point).
> 
> I have no idea what you'd want changed here, but I'll happily take
> patches.

Unfortunately, I am not sure either... I do not see anything obviously,
so perhaps it could just be avoided with the __diag() infrastructure?

diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 3dbc0b89d6fb..b58e7eb9c8f1 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -20,7 +20,12 @@
  * it for entirely different regions. In that case the arch code needs to
  * override the variable below for dma-direct to work properly.
  */
+__diag_push();
+__diag_ignore(clang, 13, "-Wconstant-conversion",
+	      "Clang incorrectly thinks the n == 64 case in DMA_BIT_MASK() can happen here,"
+	      "which would truncate with a 32-bit phys_addr_t");
 phys_addr_t zone_dma_limit __ro_after_init = DMA_BIT_MASK(24);
+__diag_pop();
 
 static inline dma_addr_t phys_to_dma_direct(struct device *dev,
 		phys_addr_t phys)

Re: [PATCH v3 2/3] dma-mapping: replace zone_dma_bits by zone_dma_limit

[Christoph Hellwig]

On Wed, Jul 31, 2024 at 06:24:24PM -0700, Nathan Chancellor wrote:
> Unfortunately, I am not sure either... I do not see anything obviously,
> so perhaps it could just be avoided with the __diag() infrastructure?
> 
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 3dbc0b89d6fb..b58e7eb9c8f1 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -20,7 +20,12 @@
>   * it for entirely different regions. In that case the arch code needs to
>   * override the variable below for dma-direct to work properly.
>   */
> +__diag_push();
> +__diag_ignore(clang, 13, "-Wconstant-conversion",
> +	      "Clang incorrectly thinks the n == 64 case in DMA_BIT_MASK() can happen here,"
> +	      "which would truncate with a 32-bit phys_addr_t");
>  phys_addr_t zone_dma_limit __ro_after_init = DMA_BIT_MASK(24);

So..  The code above is clearly wrong, as DMA_BIT_MASK always returns a
u64, and phys_addr_t can be smaller than that.  So at least in this case
the warning seems perfectly valid and the code has issues because it is
mixing different concepts.

Where do you see warnings like this upstream?

Re: [PATCH v3 2/3] dma-mapping: replace zone_dma_bits by zone_dma_limit

[Nathan Chancellor]

On Thu, Aug 01, 2024 at 03:44:54PM +0200, Christoph Hellwig wrote:
> On Wed, Jul 31, 2024 at 06:24:24PM -0700, Nathan Chancellor wrote:
> > Unfortunately, I am not sure either... I do not see anything obviously,
> > so perhaps it could just be avoided with the __diag() infrastructure?
> > 
> > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> > index 3dbc0b89d6fb..b58e7eb9c8f1 100644
> > --- a/kernel/dma/direct.c
> > +++ b/kernel/dma/direct.c
> > @@ -20,7 +20,12 @@
> >   * it for entirely different regions. In that case the arch code needs to
> >   * override the variable below for dma-direct to work properly.
> >   */
> > +__diag_push();
> > +__diag_ignore(clang, 13, "-Wconstant-conversion",
> > +	      "Clang incorrectly thinks the n == 64 case in DMA_BIT_MASK() can happen here,"
> > +	      "which would truncate with a 32-bit phys_addr_t");
> >  phys_addr_t zone_dma_limit __ro_after_init = DMA_BIT_MASK(24);
> 
> So..  The code above is clearly wrong, as DMA_BIT_MASK always returns a
> u64, and phys_addr_t can be smaller than that.  So at least in this case
> the warning seems perfectly valid and the code has issues because it is
> mixing different concepts.

Sure, that seems like a reasonable way to look at things even if the
warning itself is a false positive.

> Where do you see warnings like this upstream?

I don't see this upstream, this is from patch 2 of this series:

https://lore.kernel.org/053fa4806a2c63efcde80caca473a8b670a2701c.1722249878.git.baruch@tkos.co.il/

Cheers,
Nathan