Re: [PATCH v2 1/2] mips: cm: Convert __mips_cm_l2sync_phys_base() to weak function

["Arnd Bergmann" <arnd@arndb.de>]

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@gmail.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