Re: [v2 3/5] arm64: mte: implement CONFIG_ARM64_MTE_COMP

Andy Shevchenko
2023-07-14T10:47:32Z

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 functions,
> > 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 == 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 == 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.

...

> > > +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.

...

> > > + if (sizes[i] > largest) {
> > > + largest = sizes[i];
> > > + largest_idx = i;
> > > + }
> >
> > (alas max_array() can't be used here)
> There's no max_array() in the kernel, am I missing something?

There will be (via ASoC tree and maybe IIO tree later on) in v6.6-rc1, but
as I think it can't be used anyway because you need the index of the value
as well.

...

> > > +void ea0_release_handle(u64 handle)
> > > +{
> > > + void *storage = ea0_storage(handle);
> > > + int size = ea0_storage_size(handle);
> > > + struct kmem_cache *c;
> >
> > > + if (!handle || !storage)
> > > + return;
> >
> > You use handle before this check. Haven't you run static analysers?
>
> Sparse doesn't report anything in these files, are there any
> alternatives adopted in the kernel?
>
> Note that handle is not dereferenced above, so there's no error per se.

Even if it's a simple pointer arithmetics, the storage might (theoretically?)
have a dangling pointer, no?

> Yet (as pointed out below) these checks are redundant, so I'll remove
> some of them.

...

> > > +
> >
> > 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.

--
With Best Regards,
Andy Shevchenko