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]

Message-Id: <34af21b5-a878-418e-a70b-299cab61b37e@app.fastmail.com>
Date: Mon, 26 Feb 2024 12:04:06 +0100
From: "Arnd Bergmann" <arnd@...db.de>
To: "Serge Semin" <fancer.lancer@...il.com>,
 "Thomas Bogendoerfer" <tsbogend@...ha.franken.de>,
 "Jiaxun Yang" <jiaxun.yang@...goat.com>,
 "Andrew Morton" <akpm@...ux-foundation.org>
Cc: "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 11:54, Serge Semin wrote:
> The __mips_cm_l2sync_phys_base() and mips_cm_l2sync_phys_base() couple was
> introduced in commit 9f98f3dd0c51 ("MIPS: Add generic CM probe & access
> code") where the former method was a weak implementation of the later
> function. Such design pattern permitted to re-define the original method
> and to use the weak implementation in the new function. A similar approach
> was introduced in the framework of another arch-specific programmable
> interface: mips_cm_phys_base() and __mips_cm_phys_base(). The only
> difference is that the underscored method of the later couple was declared
> in the "asm/mips-cm.h" header file, but it wasn't done for the CM L2-sync
> methods in the subject. Due to the missing global function declaration
> the "missing prototype" warning was spotted in the framework of the commit
> 9a2036724cd6 ("mips: mark local function static if possible") and fixed
> just be re-qualifying the weak method as static. Doing that broke what was
> originally implied by having the weak implementation globally defined.
>
> Let's fix the broken CM2 L2-sync arch-interface by dropping the static
> qualifier and, seeing the implemented pattern hasn't been used for over 10
> years but will be required soon (see the link for the discussion around
> it), converting it to a single weakly defined method:
> mips_cm_l2sync_phys_base().
>
> Fixes: 9a2036724cd6 ("mips: mark local function static if possible")
> Link: 
> https://lore.kernel.org/linux-mips/20240215171740.14550-3-fancer.lancer@gmail.com
> Signed-off-by: Serge Semin <fancer.lancer@...il.com>

I'm sorry I introduced the regression here, thanks for addressing it.

> -static phys_addr_t __mips_cm_l2sync_phys_base(void)
> +phys_addr_t __weak mips_cm_l2sync_phys_base(void)
>  {
>  	u32 base_reg;
> 
> @@ -217,9 +217,6 @@ static phys_addr_t __mips_cm_l2sync_phys_base(void)
>  	return mips_cm_phys_base() + MIPS_CM_GCR_SIZE;
>  }
> 
> -phys_addr_t mips_cm_l2sync_phys_base(void)
> -	__attribute__((weak, alias("__mips_cm_l2sync_phys_base")));
> -

I generally have a bad feeling about weak functions, as they tend
to cause more problems than they solve, specifically with how they
hide what's going on, and how I still can't figure out what this
one aliases 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?

      Arnd