linux-kernel - Re: [PATCH v2 1/2] mips: cm: Convert __mips_cm_l2sync_phys

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 26 Feb 2024 15:20:43 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Arnd Bergmann <arnd@...db.de>
Cc: Thomas Bogendoerfer <tsbogend@...ha.franken.de>, 
	Jiaxun Yang <jiaxun.yang@...goat.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>, linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] mips: cm: Convert __mips_cm_l2sync_phys_base() to
 weak function

On Mon, Feb 26, 2024 at 01:04:33PM +0100, Arnd Bergmann wrote:
> On Mon, Feb 26, 2024, at 12:27, Serge Semin wrote:
> > On Mon, Feb 26, 2024 at 12:04:06PM +0100, Arnd Bergmann wrote:
> >> On Mon, Feb 26, 2024, at 11:54, Serge Semin wrote:
> s to.
> >> 
> >> Since the resolution of the alias is all done at link time
> >> anyway, could you just convert these to an #ifdef check
> >> that documents exactly when each of the versions is used?
> >
> > Not sure I've completely understood what you meant. Do you suggest to
> > add a mips_cm_l2sync_phys_base macro which would be defined if a
> > "strong" version of the method is defined (and surround the
> > underscored function by it)?
> >
> > Please note after this patch is applied no aliases will
> > be left, but only a single weakly defined method:
> > mips_cm_l2sync_phys_base()
> > This is what we agreed to do with Thomas:
> > https://lore.kernel.org/linux-mips/pf6cvzper4g5364nqhd4wd2pmlkyygoymobeqduulpslcjhyy6@kf66z7chjbl3
> > Thus there will be no need in the macro you suggest since the
> > weak-version of the method will be discarded by the linker as it will
> > have been replaced with the "strong" one. 
> 
> I meant that instead of having both a weak and an optional strong
> version that get linked together, always define exactly one of the
> two, such as:
> 
> #ifndef CONFIG_MIPS_CM_xxx
> static phys_addr_t mips_cm_l2sync_phys_base(void)
> {
>        /* current implementation ... */
> }
> #endif
> 
> where CONFIG_MIPS_CM_xxx is the Kconfig symbol that decides
> whether the file with the strong version is built or not.
> 
> This way you always get exactly one of the two versions
> of the function built, the local version can be inlined
> if the compiler thinks that is better, and the #ifdef
> documents exactly whether the function is used or not
> for a given configuration, rather than a reader having
> to track down how many other definitions exist and whether
> a config includes them.

I see your point now. Thanks for clarification. IMO it would be less
readable due to the ifdef-ery and the new config, and less
maintainable due to the conditional compilation, but would provide a
more performant solution since the compiler will be able to inline the
singly used static method. Basically you suggest to emulate the weak
implementation by an additional kernel config. Not sure whether it
would be better than a well-known weak-attribute-based pattern. Anyway
let's wait for the Thomas' opinion about your suggestion. If he thinks
it would be better I'll update the patches.

-Serge(y)

> 
>        Arnd