Date: Mon, 26 Feb 2024 14:27:56 +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

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

Powered by blists - more mailing lists