[PATCH] include/asm/cmpxchg.h: Remove duplicate header

[PATCH] include/asm/cmpxchg.h: Remove duplicate header

[Brajeswar Ghosh]

Remove asm/xchg.h which is included more than once

Signed-off-by: Brajeswar Ghosh <brajeswar.linux@gmail.com>
---
 arch/alpha/include/asm/cmpxchg.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/alpha/include/asm/cmpxchg.h b/arch/alpha/include/asm/cmpxchg.h
index 6c7c39452471..bcbdac0744f9 100644
--- a/arch/alpha/include/asm/cmpxchg.h
+++ b/arch/alpha/include/asm/cmpxchg.h
@@ -36,7 +36,6 @@
 #undef ____cmpxchg
 #define ____xchg(type, args...)		__xchg ##type(args)
 #define ____cmpxchg(type, args...)	__cmpxchg ##type(args)
-#include <asm/xchg.h>
 
 /*
  * The leading and the trailing memory barriers guarantee that these
-- 
2.17.1

Re: [PATCH] include/asm/cmpxchg.h: Remove duplicate header

[Michael Cree]

On Fri, Nov 02, 2018 at 08:56:37PM +0530, Brajeswar Ghosh wrote:
> Remove asm/xchg.h which is included more than once
> 
> Signed-off-by: Brajeswar Ghosh <brajeswar.linux@gmail.com>
> ---
>  arch/alpha/include/asm/cmpxchg.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/alpha/include/asm/cmpxchg.h b/arch/alpha/include/asm/cmpxchg.h
> index 6c7c39452471..bcbdac0744f9 100644
> --- a/arch/alpha/include/asm/cmpxchg.h
> +++ b/arch/alpha/include/asm/cmpxchg.h
> @@ -36,7 +36,6 @@
>  #undef ____cmpxchg
>  #define ____xchg(type, args...)		__xchg ##type(args)
>  #define ____cmpxchg(type, args...)	__cmpxchg ##type(args)
> -#include <asm/xchg.h>

It's amazing the number of times we get a patch to remove that.

Instead of just automatically removing a second include of a
header file, why don't you take a closer look to see what it
actually does?

Cheers,
Michael.

Re: [PATCH] include/asm/cmpxchg.h: Remove duplicate header

[Souptick Joarder]

Hi Michael,

On Fri, Nov 2, 2018 at 11:59 PM Michael Cree <mcree@orcon.net.nz> wrote:
>
> On Fri, Nov 02, 2018 at 08:56:37PM +0530, Brajeswar Ghosh wrote:
> > Remove asm/xchg.h which is included more than once
> >
> > Signed-off-by: Brajeswar Ghosh <brajeswar.linux@gmail.com>
> > ---
> >  arch/alpha/include/asm/cmpxchg.h | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/arch/alpha/include/asm/cmpxchg.h b/arch/alpha/include/asm/cmpxchg.h
> > index 6c7c39452471..bcbdac0744f9 100644
> > --- a/arch/alpha/include/asm/cmpxchg.h
> > +++ b/arch/alpha/include/asm/cmpxchg.h
> > @@ -36,7 +36,6 @@
> >  #undef ____cmpxchg
> >  #define ____xchg(type, args...)              __xchg ##type(args)
> >  #define ____cmpxchg(type, args...)   __cmpxchg ##type(args)
> > -#include <asm/xchg.h>
>
> It's amazing the number of times we get a patch to remove that.
>
> Instead of just automatically removing a second include of a
> header file, why don't you take a closer look to see what it
> actually does?

We run the static analyser "make includecheck" which list out files where
duplicate headers can be removed and based on that we thought to remove
from this file. Didn't understood about the existence of second include ??

Re: [PATCH] include/asm/cmpxchg.h: Remove duplicate header

[Matt Turner]

On Fri, Nov 2, 2018 at 11:55 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
> We run the static analyser "make includecheck" which list out files where
> duplicate headers can be removed and based on that we thought to remove
> from this file. Didn't understood about the existence of second include ??


#define ____xchg(type, args...)         __xchg ## type ## _local(args)
#define ____cmpxchg(type, args...)      __cmpxchg ## type ## _local(args)
#include <asm/xchg.h>
[snip]
#undef ____xchg
#undef ____cmpxchg
#define ____xchg(type, args...)         __xchg ##type(args)
#define ____cmpxchg(type, args...)      __cmpxchg ##type(args)
#include <asm/xchg.h>

asm/xchg.h has a comment at the top that says

/*
 * xchg/xchg_local and cmpxchg/cmpxchg_local share the same code
 * except that local version do not have the expensive memory barrier.
 * So this file is included twice from asm/cmpxchg.h.
 */

Re: [PATCH] include/asm/cmpxchg.h: Remove duplicate header

[Souptick Joarder]

On Sat, Nov 3, 2018 at 12:31 AM Matt Turner <mattst88@gmail.com> wrote:
>
> On Fri, Nov 2, 2018 at 11:55 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
> > We run the static analyser "make includecheck" which list out files where
> > duplicate headers can be removed and based on that we thought to remove
> > from this file. Didn't understood about the existence of second include ??
>
>
> #define ____xchg(type, args...)         __xchg ## type ## _local(args)
> #define ____cmpxchg(type, args...)      __cmpxchg ## type ## _local(args)
> #include <asm/xchg.h>
> [snip]
> #undef ____xchg
> #undef ____cmpxchg
> #define ____xchg(type, args...)         __xchg ##type(args)
> #define ____cmpxchg(type, args...)      __cmpxchg ##type(args)
> #include <asm/xchg.h>
>
> asm/xchg.h has a comment at the top that says
>
> /*
>  * xchg/xchg_local and cmpxchg/cmpxchg_local share the same code
>  * except that local version do not have the expensive memory barrier.
>  * So this file is included twice from asm/cmpxchg.h.
>  */

Thanks Matt. Sorry for the noise.
Is there any way to exclude it from static analyser that someone else will
not do the same mistake in future ?

Re: [PATCH] include/asm/cmpxchg.h: Remove duplicate header

[Matt Turner]

On Fri, Nov 2, 2018 at 12:09 PM Souptick Joarder <jrdr.linux@gmail.com> wrote:
>
> On Sat, Nov 3, 2018 at 12:31 AM Matt Turner <mattst88@gmail.com> wrote:
> >
> > On Fri, Nov 2, 2018 at 11:55 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
> > > We run the static analyser "make includecheck" which list out files where
> > > duplicate headers can be removed and based on that we thought to remove
> > > from this file. Didn't understood about the existence of second include ??
> >
> >
> > #define ____xchg(type, args...)         __xchg ## type ## _local(args)
> > #define ____cmpxchg(type, args...)      __cmpxchg ## type ## _local(args)
> > #include <asm/xchg.h>
> > [snip]
> > #undef ____xchg
> > #undef ____cmpxchg
> > #define ____xchg(type, args...)         __xchg ##type(args)
> > #define ____cmpxchg(type, args...)      __cmpxchg ##type(args)
> > #include <asm/xchg.h>
> >
> > asm/xchg.h has a comment at the top that says
> >
> > /*
> >  * xchg/xchg_local and cmpxchg/cmpxchg_local share the same code
> >  * except that local version do not have the expensive memory barrier.
> >  * So this file is included twice from asm/cmpxchg.h.
> >  */
>
> Thanks Matt. Sorry for the noise.
> Is there any way to exclude it from static analyser that someone else will
> not do the same mistake in future ?

Since this is not an uncommon pattern in C, I think any static
analysis tool that attempts to find duplicate includes should attempt
to recognize such a pattern.

That, or humans should review the output of their static analysis
tools. Or you could try to compile the patches produced. I think any
of those would have caught the problem with the patch.

Re: [PATCH] include/asm/cmpxchg.h: Remove duplicate header

[kbuild test robot]

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

Hi Brajeswar,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on sof-driver-fuweitax/master]
[also build test ERROR on v4.19 next-20181102]
[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/Brajeswar-Ghosh/include-asm-cmpxchg-h-Remove-duplicate-header/20181103-072531
base:   https://github.com/fuweitax/linux master
config: alpha-allyesconfig (attached as .config)
compiler: alpha-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
        GCC_VERSION=7.2.0 make.cross ARCH=alpha 

All errors (new ones prefixed by >>):

   In file included from arch/alpha/include/asm/atomic.h:7:0,
                    from include/linux/atomic.h:5,
                    from include/linux/rcupdate.h:38,
                    from include/linux/rculist.h:11,
                    from include/linux/pid.h:5,
                    from include/linux/sched.h:14,
                    from arch/alpha/kernel/asm-offsets.c:10:
   include/linux/atomic.h: In function 'atomic_inc_not_zero_hint':
   arch/alpha/include/asm/cmpxchg.h:61:31: error: implicit declaration of function '__cmpxchg'; did you mean 'cmpxchg'? [-Werror=implicit-function-declaration]
     __ret = (__typeof__(*(ptr))) __cmpxchg((ptr),   \
                                  ^
   arch/alpha/include/asm/atomic.h:205:38: note: in expansion of macro 'cmpxchg'
    #define atomic_cmpxchg(v, old, new) (cmpxchg(&((v)->counter), old, new))
                                         ^~~~~~~
   include/linux/atomic.h:596:9: note: in expansion of macro 'atomic_cmpxchg'
      val = atomic_cmpxchg(v, c, c + 1);
            ^~~~~~~~~~~~~~
   In file included from arch/alpha/include/asm/atomic.h:7:0,
                    from include/linux/atomic.h:5,
                    from include/linux/rcupdate.h:38,
                    from include/linux/rculist.h:11,
                    from include/linux/pid.h:5,
                    from include/linux/sched.h:14,
                    from arch/alpha/kernel/asm-offsets.c:10:
   include/linux/debug_locks.h: In function '__debug_locks_off':
>> arch/alpha/include/asm/cmpxchg.h:50:3: error: implicit declaration of function '__xchg'; did you mean 'xchg'? [-Werror=implicit-function-declaration]
      __xchg((ptr), (unsigned long)_x_, sizeof(*(ptr))); \
      ^
   include/linux/debug_locks.h:17:9: note: in expansion of macro 'xchg'
     return xchg(&debug_locks, 0);
            ^~~~
   include/linux/llist.h: In function 'llist_del_all':
   arch/alpha/include/asm/cmpxchg.h:49:10: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
     __ret = (__typeof__(*(ptr)))     \
             ^
   include/linux/llist.h:234:9: note: in expansion of macro 'xchg'
     return xchg(&head->first, NULL);
            ^~~~
   cc1: some warnings being treated as errors
   make[2]: *** [arch/alpha/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [sub-make] Error 2

vim +50 arch/alpha/include/asm/cmpxchg.h

5ba840f9 Paul Gortmaker 2012-04-02  39  
fbfcd019 Andrea Parri   2018-02-27  40  /*
fbfcd019 Andrea Parri   2018-02-27  41   * The leading and the trailing memory barriers guarantee that these
fbfcd019 Andrea Parri   2018-02-27  42   * operations are fully ordered.
fbfcd019 Andrea Parri   2018-02-27  43   */
5ba840f9 Paul Gortmaker 2012-04-02  44  #define xchg(ptr, x)							\
5ba840f9 Paul Gortmaker 2012-04-02  45  ({									\
fbfcd019 Andrea Parri   2018-02-27  46  	__typeof__(*(ptr)) __ret;					\
5ba840f9 Paul Gortmaker 2012-04-02  47  	__typeof__(*(ptr)) _x_ = (x);					\
fbfcd019 Andrea Parri   2018-02-27  48  	smp_mb();							\
fbfcd019 Andrea Parri   2018-02-27  49  	__ret = (__typeof__(*(ptr)))					\
fbfcd019 Andrea Parri   2018-02-27 @50  		__xchg((ptr), (unsigned long)_x_, sizeof(*(ptr)));	\
fbfcd019 Andrea Parri   2018-02-27  51  	smp_mb();							\
fbfcd019 Andrea Parri   2018-02-27  52  	__ret;								\
5ba840f9 Paul Gortmaker 2012-04-02  53  })
5ba840f9 Paul Gortmaker 2012-04-02  54  

:::::: The code at line 50 was first introduced by commit
:::::: fbfcd0199170984bd3c2812e49ed0fe7b226959a locking/xchg/alpha: Remove superfluous memory barriers from the _local() variants

:::::: TO: Andrea Parri <parri.andrea@gmail.com>
:::::: CC: Ingo Molnar <mingo@kernel.org>

---
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: 52359 bytes --]

Re: [PATCH] include/asm/cmpxchg.h: Remove duplicate header

[kbuild test robot]

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

Hi Brajeswar,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on sof-driver-fuweitax/master]
[also build test ERROR on v4.19 next-20181102]
[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/Brajeswar-Ghosh/include-asm-cmpxchg-h-Remove-duplicate-header/20181103-072531
base:   https://github.com/fuweitax/linux master
config: alpha-allmodconfig (attached as .config)
compiler: alpha-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
        GCC_VERSION=7.2.0 make.cross ARCH=alpha 

All error/warnings (new ones prefixed by >>):

   In file included from arch/alpha/include/asm/atomic.h:7:0,
                    from include/linux/atomic.h:5,
                    from include/linux/rcupdate.h:38,
                    from include/linux/rculist.h:11,
                    from include/linux/pid.h:5,
                    from include/linux/sched.h:14,
                    from arch/alpha/kernel/asm-offsets.c:10:
   include/linux/atomic.h: In function 'atomic_inc_not_zero_hint':
>> arch/alpha/include/asm/cmpxchg.h:61:31: error: implicit declaration of function '__cmpxchg'; did you mean 'cmpxchg'? [-Werror=implicit-function-declaration]
     __ret = (__typeof__(*(ptr))) __cmpxchg((ptr),   \
                                  ^
>> arch/alpha/include/asm/atomic.h:205:38: note: in expansion of macro 'cmpxchg'
    #define atomic_cmpxchg(v, old, new) (cmpxchg(&((v)->counter), old, new))
                                         ^~~~~~~
>> include/linux/atomic.h:596:9: note: in expansion of macro 'atomic_cmpxchg'
      val = atomic_cmpxchg(v, c, c + 1);
            ^~~~~~~~~~~~~~
   In file included from arch/alpha/include/asm/atomic.h:7:0,
                    from include/linux/atomic.h:5,
                    from include/linux/rcupdate.h:38,
                    from include/linux/rculist.h:11,
                    from include/linux/pid.h:5,
                    from include/linux/sched.h:14,
                    from arch/alpha/kernel/asm-offsets.c:10:
   include/linux/debug_locks.h: In function '__debug_locks_off':
>> arch/alpha/include/asm/cmpxchg.h:50:3: error: implicit declaration of function '__xchg'; did you mean '____xchg'? [-Werror=implicit-function-declaration]
      __xchg((ptr), (unsigned long)_x_, sizeof(*(ptr))); \
      ^
>> include/linux/debug_locks.h:17:9: note: in expansion of macro 'xchg'
     return xchg(&debug_locks, 0);
            ^~~~
   include/linux/llist.h: In function 'llist_del_all':
>> arch/alpha/include/asm/cmpxchg.h:49:10: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
     __ret = (__typeof__(*(ptr)))     \
             ^
>> include/linux/llist.h:234:9: note: in expansion of macro 'xchg'
     return xchg(&head->first, NULL);
            ^~~~
   cc1: some warnings being treated as errors
   make[2]: *** [arch/alpha/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [sub-make] Error 2

vim +61 arch/alpha/include/asm/cmpxchg.h

5ba840f9 Paul Gortmaker 2012-04-02  39  
fbfcd019 Andrea Parri   2018-02-27  40  /*
fbfcd019 Andrea Parri   2018-02-27  41   * The leading and the trailing memory barriers guarantee that these
fbfcd019 Andrea Parri   2018-02-27  42   * operations are fully ordered.
fbfcd019 Andrea Parri   2018-02-27  43   */
5ba840f9 Paul Gortmaker 2012-04-02  44  #define xchg(ptr, x)							\
5ba840f9 Paul Gortmaker 2012-04-02  45  ({									\
fbfcd019 Andrea Parri   2018-02-27  46  	__typeof__(*(ptr)) __ret;					\
5ba840f9 Paul Gortmaker 2012-04-02  47  	__typeof__(*(ptr)) _x_ = (x);					\
fbfcd019 Andrea Parri   2018-02-27  48  	smp_mb();							\
fbfcd019 Andrea Parri   2018-02-27 @49  	__ret = (__typeof__(*(ptr)))					\
fbfcd019 Andrea Parri   2018-02-27 @50  		__xchg((ptr), (unsigned long)_x_, sizeof(*(ptr)));	\
fbfcd019 Andrea Parri   2018-02-27  51  	smp_mb();							\
fbfcd019 Andrea Parri   2018-02-27  52  	__ret;								\
5ba840f9 Paul Gortmaker 2012-04-02  53  })
5ba840f9 Paul Gortmaker 2012-04-02  54  
5ba840f9 Paul Gortmaker 2012-04-02  55  #define cmpxchg(ptr, o, n)						\
5ba840f9 Paul Gortmaker 2012-04-02  56  ({									\
fbfcd019 Andrea Parri   2018-02-27  57  	__typeof__(*(ptr)) __ret;					\
5ba840f9 Paul Gortmaker 2012-04-02  58  	__typeof__(*(ptr)) _o_ = (o);					\
5ba840f9 Paul Gortmaker 2012-04-02  59  	__typeof__(*(ptr)) _n_ = (n);					\
fbfcd019 Andrea Parri   2018-02-27  60  	smp_mb();							\
fbfcd019 Andrea Parri   2018-02-27 @61  	__ret = (__typeof__(*(ptr))) __cmpxchg((ptr),			\
fbfcd019 Andrea Parri   2018-02-27  62  		(unsigned long)_o_, (unsigned long)_n_, sizeof(*(ptr)));\
fbfcd019 Andrea Parri   2018-02-27  63  	smp_mb();							\
fbfcd019 Andrea Parri   2018-02-27  64  	__ret;								\
5ba840f9 Paul Gortmaker 2012-04-02  65  })
5ba840f9 Paul Gortmaker 2012-04-02  66  

:::::: The code at line 61 was first introduced by commit
:::::: fbfcd0199170984bd3c2812e49ed0fe7b226959a locking/xchg/alpha: Remove superfluous memory barriers from the _local() variants

:::::: TO: Andrea Parri <parri.andrea@gmail.com>
:::::: CC: Ingo Molnar <mingo@kernel.org>

---
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: 52265 bytes --]