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/[email protected]
> Signed-off-by: Serge Semin <[email protected]>
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/[email protected]
> > Signed-off-by: Serge Semin <[email protected]>
>
> 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
On Mon, Feb 26, 2024, at 12:27, Serge Semin wrote:
> On Mon, Feb 26, 2024 at 12:04:06PM +0100, Arnd Bergmann wrote:
>> On Mon, Feb 26, 2024, at 11:54, Serge Semin wrote:
s 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.
I meant that instead of having both a weak and an optional strong
version that get linked together, always define exactly one of the
two, such as:
#ifndef CONFIG_MIPS_CM_xxx
static phys_addr_t mips_cm_l2sync_phys_base(void)
{
/* current implementation ... */
}
#endif
where CONFIG_MIPS_CM_xxx is the Kconfig symbol that decides
whether the file with the strong version is built or not.
This way you always get exactly one of the two versions
of the function built, the local version can be inlined
if the compiler thinks that is better, and the #ifdef
documents exactly whether the function is used or not
for a given configuration, rather than a reader having
to track down how many other definitions exist and whether
a config includes them.
Arnd
On Mon, Feb 26, 2024 at 01:04:33PM +0100, Arnd Bergmann wrote:
> On Mon, Feb 26, 2024, at 12:27, Serge Semin wrote:
> > On Mon, Feb 26, 2024 at 12:04:06PM +0100, Arnd Bergmann wrote:
> >> On Mon, Feb 26, 2024, at 11:54, Serge Semin wrote:
> s 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.
>
> I meant that instead of having both a weak and an optional strong
> version that get linked together, always define exactly one of the
> two, such as:
>
> #ifndef CONFIG_MIPS_CM_xxx
> static phys_addr_t mips_cm_l2sync_phys_base(void)
> {
> /* current implementation ... */
> }
> #endif
>
> where CONFIG_MIPS_CM_xxx is the Kconfig symbol that decides
> whether the file with the strong version is built or not.
>
> This way you always get exactly one of the two versions
> of the function built, the local version can be inlined
> if the compiler thinks that is better, and the #ifdef
> documents exactly whether the function is used or not
> for a given configuration, rather than a reader having
> to track down how many other definitions exist and whether
> a config includes them.
I see your point now. Thanks for clarification. IMO it would be less
readable due to the ifdef-ery and the new config, and less
maintainable due to the conditional compilation, but would provide a
more performant solution since the compiler will be able to inline the
singly used static method. Basically you suggest to emulate the weak
implementation by an additional kernel config. Not sure whether it
would be better than a well-known weak-attribute-based pattern. Anyway
let's wait for the Thomas' opinion about your suggestion. If he thinks
it would be better I'll update the patches.
-Serge(y)
>
> Arnd
On Mon, Feb 26, 2024, at 13:20, Serge Semin wrote:
> On Mon, Feb 26, 2024 at 01:04:33PM +0100, Arnd Bergmann wrote:
>> On Mon, Feb 26, 2024, at 12:27, Serge Semin wrote:
> I see your point now. Thanks for clarification. IMO it would be less
> readable due to the ifdef-ery and the new config, and less
> maintainable due to the conditional compilation, but would provide a
> more performant solution since the compiler will be able to inline the
> singly used static method. Basically you suggest to emulate the weak
> implementation by an additional kernel config.
I mean the kernel config that you already need here, since
the strong version of the function is already optional.
> Not sure whether it would be better than a well-known
> weak-attribute-based pattern. Anyway let's wait for the
> Thomas' opinion about your suggestion. If he thinks
> it would be better I'll update the patches.
Weak functions are not used all that much outside of a
couple of parts of the kernel. There is a lot of them
in drivers/pci/, a little bit in acpi and efi, and
then a bit in arch/*/, though most of that is in mips.
Ifdef checks in .c files are not great, but at least they
are much more common than __weak functions and self-documenting.
Arnd
On Mon, Feb 26, 2024 at 01:29:54PM +0100, Arnd Bergmann wrote:
> On Mon, Feb 26, 2024, at 13:20, Serge Semin wrote:
> > On Mon, Feb 26, 2024 at 01:04:33PM +0100, Arnd Bergmann wrote:
> >> On Mon, Feb 26, 2024, at 12:27, Serge Semin wrote:
>
> > I see your point now. Thanks for clarification. IMO it would be less
> > readable due to the ifdef-ery and the new config, and less
> > maintainable due to the conditional compilation, but would provide a
> > more performant solution since the compiler will be able to inline the
> > singly used static method. Basically you suggest to emulate the weak
> > implementation by an additional kernel config.
>
> I mean the kernel config that you already need here, since
> the strong version of the function is already optional.
Why would I need it if after this patch is applied the
mips_cm_l2sync_phys_base() method will be converted to a global weak
implementation?
>
> > Not sure whether it would be better than a well-known
> > weak-attribute-based pattern. Anyway let's wait for the
> > Thomas' opinion about your suggestion. If he thinks
> > it would be better I'll update the patches.
>
> Weak functions are not used all that much outside of a
> couple of parts of the kernel. There is a lot of them
> in drivers/pci/, a little bit in acpi and efi, and
> then a bit in arch/*/, though most of that is in mips.
+ a lot of them in kernel/*, some in mm/* .)
>
> Ifdef checks in .c files are not great, but at least they
> are much more common than __weak functions and self-documenting.
Ok. I don't have concretely strong opinion about what is better. Let's
wait for what Thomas thinks about this.
-Serge(y)
>
> Arnd
On Mon, Feb 26, 2024 at 04:11:05PM +0300, Serge Semin wrote:
> On Mon, Feb 26, 2024 at 01:29:54PM +0100, Arnd Bergmann wrote:
> > On Mon, Feb 26, 2024, at 13:20, Serge Semin wrote:
> > > On Mon, Feb 26, 2024 at 01:04:33PM +0100, Arnd Bergmann wrote:
> > >> On Mon, Feb 26, 2024, at 12:27, Serge Semin wrote:
> >
> > > I see your point now. Thanks for clarification. IMO it would be less
> > > readable due to the ifdef-ery and the new config, and less
> > > maintainable due to the conditional compilation, but would provide a
> > > more performant solution since the compiler will be able to inline the
> > > singly used static method. Basically you suggest to emulate the weak
> > > implementation by an additional kernel config.
> >
> > I mean the kernel config that you already need here, since
> > the strong version of the function is already optional.
>
> Why would I need it if after this patch is applied the
> mips_cm_l2sync_phys_base() method will be converted to a global weak
> implementation?
>
> >
> > > Not sure whether it would be better than a well-known
> > > weak-attribute-based pattern. Anyway let's wait for the
> > > Thomas' opinion about your suggestion. If he thinks
> > > it would be better I'll update the patches.
> >
> > Weak functions are not used all that much outside of a
> > couple of parts of the kernel. There is a lot of them
> > in drivers/pci/, a little bit in acpi and efi, and
> > then a bit in arch/*/, though most of that is in mips.
>
> + a lot of them in kernel/*, some in mm/* .)
>
> >
> > Ifdef checks in .c files are not great, but at least they
> > are much more common than __weak functions and self-documenting.
>
> Ok. I don't have concretely strong opinion about what is better. Let's
> wait for what Thomas thinks about this.
I've taken your patches as we get rid of this alias thing. As long as
there is no big push against __weak I'm ok with this case.
Thomas.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]