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@xxxxxxxxx
> > Signed-off-by: Serge Semin <fancer.lancer@xxxxxxxxx>
>
> 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
