linux-kernel - Re: [PATCH] atomic: cleanup asm-generic atomic*.h inclusion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BANLkTinQhBfAJd5-ZaPXbiV778c9fvXeWg@mail.gmail.com>
Date:	Fri, 17 Jun 2011 15:07:04 -0400
From:	Mike Frysinger <vapier.adi@...il.com>
To:	Arun Sharma <asharma@...com>
Cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Eric Dumazet <eric.dumazet@...il.com>,
	Ingo Molnar <mingo@...e.hu>, David Miller <davem@...emloft.net>
Subject: Re: [PATCH] atomic: cleanup asm-generic atomic*.h inclusion

On Fri, Jun 17, 2011 at 14:42, Arun Sharma wrote:
> On 6/17/11 11:06 AM, Mike Frysinger wrote:
>> fixes one thing while breaking another.  linux/atomic.h includes
>> asm/atomic.h which includes asm-generic/atomic.h which includes
>> asm-generic/atomic-long.h which needs atomic_add_unless(), but that
>> isnt provided until after the asm/atomic.h include in linux/atomic.h.
>>
>> but linux/atomic.h needs asm/atomic.h before atomic_add_unless()
>> because it relies on the new __atomic_add_unless().
>
> Right. I think we need to get rid of that include as well:

ok, then we get to your proposed asm-generic/atomic.h change not being
correct.  instead of deleting atomic_add_unless(), it probably should
redo it to __atomic_add_unless().  people using asm-generic/atomic.h
are not defining __atomic_add_unless() themselves.

  CC      arch/blackfin/kernel/asm-offsets.s
In file included from include/linux/spinlock.h:387,
                 from include/linux/seqlock.h:29,
                 from include/linux/time.h:8,
                 from include/linux/timex.h:56,
                 from include/linux/sched.h:57,
                 from arch/blackfin/kernel/asm-offsets.c:10:
include/linux/atomic.h: In function ‘atomic_add_unless’:
include/linux/atomic.h:17: error: implicit declaration of function
‘__atomic_add_unless’
make[1]: *** [arch/blackfin/kernel/asm-offsets.s] Error 1

>> having linux/atomic.h and asm-generic/atomic.h just strikes me as
>> wrong.  the point of asm-generic is to unify things, but now we have
>> two places to unify things without 0 indication as to which is for
>> which ?  i'm wondering if we shouldnt convert all arches to
>> asm-generic/atomic.h and then add your new logic there and just skip
>> this whole linux/atomic.h mess.
>
> I believe the logic is:
>
> <asm-generic/atomic.h> shared code for simple archs that don't want to
> define their own primitives. Only used by 4 archs.

i think the 4 is only really because asm-generic/atomic.h is so new.
those arches are also new and started with it, or did work to convert
over.  i imagine a lot of the existing arches could convert over to
it.

> <linux/atomic.h> a header file that's usable in machine independent kernel
> code.

if that's the direction we want to take things, then this needs to be
documented at the top of both files.  i also see quite a lot of
primitives in asm-generic/atomic.h that would be better placed using
your definitions in linux/atomic.h.

i would refine your asm-generic/atomic.h definition as "simple C
definitions useful for UP systems only"

> My guess is that if <linux/atomic.h> existed in 2006, people would've added
> shared code over there.

i'd propose that the guy adding linux/atomic.h was lazy and punked out
rather than doing real work

> Also, the code in <asm-generic/atomic.h> is not truly generic:

sure it is.  it'll work on any arch in a UP setup.  it (currently)
does not support SMP by design.
-mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/