Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230908111053.1660033-1-chenhuacai@loongson.cn> <CAHk-=wh-uoZ_XsZ4fDodqjUY+rYJq=D3RVme3f=zBDB5neWhKQ@mail.gmail.com>
In-Reply-To: <CAHk-=wh-uoZ_XsZ4fDodqjUY+rYJq=D3RVme3f=zBDB5neWhKQ@mail.gmail.com>
From:   Huacai Chen <chenhuacai@kernel.org>
Date:   Sat, 9 Sep 2023 16:05:44 +0800
Message-ID: <CAAhV-H64=ZWBnzFmtS-FMB83nn21DSm20adrXdsrYkoka_oyjw@mail.gmail.com>
Subject: Re: [GIT PULL] LoongArch changes for v6.6
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Huacai Chen <chenhuacai@loongson.cn>,
        Arnd Bergmann <arnd@arndb.de>, loongarch@lists.linux.dev,
        linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org,
        Guo Ren <guoren@kernel.org>, Xuerui Wang <kernel@xen0n.name>,
        Jiaxun Yang <jiaxun.yang@flygoat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi, Linus,

On Sat, Sep 9, 2023 at 3:48=E2=80=AFAM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, 8 Sept 2023 at 04:11, Huacai Chen <chenhuacai@loongson.cn> wrote:
> >
> > 7, Add KASAN (Kernel Address Sanitizer) support
>
> I have pulled this, but please at least consider
>
>  (a) don't use the stupid and random __HAVE_ARCH_xyz defines
>
> Yes, yes, we have historical uses of it. That doesn't make it good.
> Instead of making up new random symbol names, just *USE* the names you
> are defining per architecture.
>
> IOW, instead of doing
>
>   #define __HAVE_ARCH_SHADOW_MAP
>
> and defining your own helper replacement functions for
> kasan_mem_to_shadow() etc, just use those names as-is.
>
> So if you have an architecture that has its own version of
> "kasan_mem_to_shadow()", then use *THAT* name for the #ifdef too.
> Don't make up another name entirely of the form "__HAVE_ARCH_xyz".
>
> Example: architectures can override the default generic versions of
> "arch_atomic_xyz()" operations, and the way you do that is (for
> example)
>
>   static __always_inline int arch_atomic_add_return(int i, atomic_t *v)
>   {
>         return i + xadd(&v->counter, i);
>   }
>   #define arch_atomic_add_return arch_atomic_add_return
>
> note how the define to let you know the name exists is the name itself
> (and because the implementation is an inline function, not the macro,
> the marker is then just a "define the name to itself").
>
> End result: no made-up secondary names for other things. When you grep
> for the thing that is used, you find both the implementation and the
> marker for "look, I am overriding it".
>
> And also
>
>  (b) do you really want to inline those kasan_mem_to_shadow() and
> kasan_shadow_to_mem() things?
>
> Yes, the caller is addr_has_metadata(), and that one is
> "__always_inline",. because otherwise objtool would complain about
> doing function calls in SMAP-enabled regions.
>
> HOWEVER:
>
>  - on LoongArch, I don't think you have that objtool / SMAP issue
>
>  - and if  you did, the function should be __always_inline, not just
> plain inline anyway
>
> so I get the feeling that the inline is simply wrong either way. Maybe
> that function should just be declared, with the implementation being
> out-of-line? It seems unnecessarily big to be an inline function, and
> it doesn't seem performance-critical?
>
> Neither of the above issues are critical, and the second one in
> particular really is just a "did you really mean to do that" kind of
> reaction, but since I was looking at this, I decided to write up my
> reactions.
Thank you for your suggestions, I will make cleanup patches for the
two issues before v6.6 is released.

Huacai
>
>               Linus
>