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: <20230713125706.2884502-1-glider@google.com> <20230713125706.2884502-4-glider@google.com>
 <ZLAzG+Ue3JqDM/F3@smile.fi.intel.com> <CAG_fn=W4Uv2YaO=Udwo80_f74y8o0+WWVVqTNK3iW5VDs5B8+w@mail.gmail.com>
 <ZLEnublo8C5yX3si@smile.fi.intel.com>
In-Reply-To: <ZLEnublo8C5yX3si@smile.fi.intel.com>
From:   Alexander Potapenko <glider@google.com>
Date:   Fri, 14 Jul 2023 13:17:08 +0200
Message-ID: <CAG_fn=V3k0PLHh=NbKyPjK7RNXwpzBRb_JALM_xCNoc2D+yE8g@mail.gmail.com>
Subject: Re: [v2 3/5] arm64: mte: implement CONFIG_ARM64_MTE_COMP
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     catalin.marinas@arm.com, will@kernel.org, pcc@google.com,
        andreyknvl@gmail.com, linux@rasmusvillemoes.dk,
        yury.norov@gmail.com, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, eugenis@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jul 14, 2023 at 12:47=E2=80=AFPM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Jul 14, 2023 at 11:25:41AM +0200, Alexander Potapenko wrote:
>
> ...
>
> > > Not sure why fls() / BIT() can't be used directly instead of these fu=
nctions,
> > > but okay, they are not too ugly.
> >
> > They can't be used directly because 128 maps to 0, but I can sure
> > simplify them a bit.
>
> Right, that's why I'm okay with the current implementation. But
> if you want to rewrite, up to you.
>
> ...
>
> > > > +                     if (pos % 2 =3D=3D 0)
> > >
> > > Would be better to keep this aligned with above?
> > >
> > >                         if (pos % 2)
> > >                                 ...
> > >                         else
> > >                                 ...
> >
> > It would, but i % 2 above didn't survive the rewrite, so I assume it
> > is fine to keep pos % 2 =3D=3D 0 as is.
>
> Not big deal, but less characters improve the brain process, so
>
>         if (pos % 2)
>
> kinda quicker to read and understand in my opinion.

Ok, will do.

> ...
>
> > > > +EXPORT_SYMBOL(ea0_storage_size);
> > >
> > > Btw, can we go to the namespaced export from day 1?
> >
> > Am I getting it right that I just need to change EXPORT_SYMBOL to
> > EXPORT_SYMBOL_NS and import the namespace in
> > arch/arm64/mm/test_mtecomp.c?
> > I.e. MODULE_IMPORT_NS is not needed in mteswap_comp.c, because it is
> > linked into the kernel?
>
> I think you always need to include MODULE_IMPORT_NS for the sake of
> robustness of the code.

Thanks! The docs aren't very specific on this.


> ...
>
> > > > +
> > >
> > > Unneeded blank line.
> >
> > I think there's no agreement on this in the kernel code, but my
> > version is more popular:
> >
> > $ git grep -B2 '^module_init(' | grep '\-}' -A2 | grep module_init | wc
> >    2688    2707  164023
> > $ git grep -B2 '^module_init(' | grep '\-}' -A1 | grep module_init | wc
> >     505     523   30989
>
> Even though, there is no need for this blank line. And note, for better
> argument, compare this for the new code added let's say for the past 2
> years. I believe numbers will tend to my variant.
>
> I.o.w. you need to count on trends and not only on frequencies.

Fair enough, will fix.