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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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