Received-SPF: pass (google.com: domain of linux-kernel+bounces-81245-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-153-g7e3bb84806-fm-20240215.007-g7e3bb848
Precedence: bulk
MIME-Version: 1.0
Message-Id: <34af21b5-a878-418e-a70b-299cab61b37e@app.fastmail.com>
In-Reply-To: <20240226105427.7191-2-fancer.lancer@gmail.com>
References: <20240226105427.7191-1-fancer.lancer@gmail.com>
 <20240226105427.7191-2-fancer.lancer@gmail.com>
Date: Mon, 26 Feb 2024 12:04:06 +0100
From: "Arnd Bergmann" <arnd@arndb.de>
To: "Serge Semin" <fancer.lancer@gmail.com>,
 "Thomas Bogendoerfer" <tsbogend@alpha.franken.de>,
 "Jiaxun Yang" <jiaxun.yang@flygoat.com>,
 "Andrew Morton" <akpm@linux-foundation.org>
Cc: "Alexey Malahov" <Alexey.Malahov@baikalelectronics.ru>,
 linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] mips: cm: Convert __mips_cm_l2sync_phys_base() to weak
 function
Content-Type: text/plain

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