Messages in this thread | | | From | Alexander Potapenko <> | Date | Fri, 14 Jul 2023 13:17:08 +0200 | Subject | Re: [v2 3/5] arm64: mte: implement CONFIG_ARM64_MTE_COMP |
| |
On Fri, Jul 14, 2023 at 12:47 PM 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 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.
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.
| |