Messages in this thread | | | Date | Fri, 14 Jul 2023 13:47:21 +0300 | From | Andy Shevchenko <> | Subject | Re: [v2 3/5] arm64: mte: implement CONFIG_ARM64_MTE_COMP |
| |
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
| |