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>

---

Changelog v2:
- Convert the underscored method to a single weakly defined function.
---
 arch/mips/include/asm/mips-cm.h | 13 +++++++++++++
 arch/mips/kernel/mips-cm.c      |  5 +----
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/mips/include/asm/mips-cm.h b/arch/mips/include/asm/mips-cm.h
index 23c67c0871b1..6cc79296c8ef 100644
--- a/arch/mips/include/asm/mips-cm.h
+++ b/arch/mips/include/asm/mips-cm.h
@@ -33,6 +33,19 @@ extern void __iomem *mips_cm_l2sync_base;
  */
 extern phys_addr_t __mips_cm_phys_base(void);
 
+/**
+ * mips_cm_l2sync_phys_base - retrieve the physical base address of the CM
+ *                            L2-sync region
+ *
+ * This function returns the physical base address of the Coherence Manager
+ * L2-cache only region. It provides a default implementation which reads the
+ * CMGCRL2OnlySyncBase register where available or returns a 4K region just
+ * behind the CM GCR base address. It may be overridden by platforms which
+ * determine this address in a different way by defining a function with the
+ * same prototype.
+ */
+extern phys_addr_t mips_cm_l2sync_phys_base(void);
+
 /*
  * mips_cm_is64 - determine CM register width
  *
diff --git a/arch/mips/kernel/mips-cm.c b/arch/mips/kernel/mips-cm.c
index 84b3affb9de8..268ac0b811e3 100644
--- a/arch/mips/kernel/mips-cm.c
+++ b/arch/mips/kernel/mips-cm.c
@@ -201,7 +201,7 @@ phys_addr_t __mips_cm_phys_base(void)
 phys_addr_t mips_cm_phys_base(void)
 	__attribute__((weak, alias("__mips_cm_phys_base")));
 
-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")));
-
 static void mips_cm_probe_l2sync(void)
 {
 	unsigned major_rev;
-- 
2.43.0

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

Hi Arnd

On Mon, Feb 26, 2024 at 12:04:06PM +0100, Arnd Bergmann wrote:
> 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.

No worries. I've noticed it in my local tree only. Neither CM nor CM
L2-sync base address getters aren't currently re-defined in the
mainline code. So the generic kernel code hasn't been affected.

> 
> > -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?

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. 

-Serge(y)

> 
>       Arnd