* [PATCH v2 0/2] MIPS: Fix missing proto and passing arg warnings @ 2024-02-26 10:54 Serge Semin 2024-02-26 10:54 ` [PATCH v2 1/2] mips: cm: Convert __mips_cm_l2sync_phys_base() to weak function Serge Semin ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Serge Semin @ 2024-02-26 10:54 UTC (permalink / raw) To: Thomas Bogendoerfer, Arnd Bergmann, Jiaxun Yang Cc: Serge Semin, Alexey Malahov, Andrew Morton, linux-mips, linux-kernel After getting my local tree rebased onto the kernel 6.8-rc3 the MIPS32 kernel build procedure produced a couple of warnings which I suggest to fix in the framework of this series. A first warning is of the "no previous prototype for `<func>`" type. In particular my arch-specific code has the mips_cm_l2sync_phys_base() method re-defined, but even though the function is global it' prototype isn't declared anywhere. Fix that by moving the __mips_cm_l2sync_phys_base() body to a weak implementation of mips_cm_l2sync_phys_base() and adding the method prototype declaration to the mips/include/asm/mips-cm.h header file. For the sake of unification a similar solution was provided for the mips_cm_phys_base()/__mips_cm_phys_base() couple. The following text describes the patches which have already merged in at v1 stage of the patchset (see changelog v2). One more case of the denoted earlier warning I spotted in the self-extracting kernel (so called zboot) with the debug printouts enabled. In particular there are several putc() method re-definitions available in: arch/mips/boot/compressed/uart-prom.c arch/mips/boot/compressed/uart-16550.c arch/mips/boot/compressed/uart-alchemy.c All of these files lacked the prototype declaration what caused having the "no previous prototype for ‘putc’" printed on my build with the next configs enabled: CONFIG_SYS_SUPPORTS_ZBOOT=y CONFIG_SYS_SUPPORTS_ZBOOT_UART_PROM=y CONFIG_ZBOOT_LOAD_ADDRESS=0x85100000 CONFIG_DEBUG_ZBOOT=y The second warning is of the "passing argument <x> of ‘<func>’ from incompatible pointer type" type which I discovered in the drivers/tty/mips_ejtag_fdc.c driver. The problem most likely happened due to the commit ce7cbd9a6c81 ("tty: mips_ejtag_fdc: use u8 for character pointers"). That's it for today.) Thanks for review in advance. Any tests are very welcome. Link: https://lore.kernel.org/linux-mips/20240215171740.14550-1-fancer.lancer@gmail.com Changelog v2: - Drop aleady applied pateches: [PATCH 3/4] mips: zboot: Fix "no previous prototype" build warning [PATCH 4/4] tty: mips_ejtag_fdc: Fix passing incompatible pointer type warning - Drop Linux serial mailing list and the respective maintainers from the Cc-list. - Covert the underscored versions of the CM2/L2-sync base address getters to being the body of the weakly defined original methods. Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: linux-mips@vger.kernel.org Cc: linux-kernel@vger.kernel.org Serge Semin (2): mips: cm: Convert __mips_cm_l2sync_phys_base() to weak function mips: cm: Convert __mips_cm_phys_base() to weak function arch/mips/include/asm/mips-cm.h | 20 ++++++++++++++++---- arch/mips/kernel/mips-cm.c | 10 ++-------- 2 files changed, 18 insertions(+), 12 deletions(-) -- 2.43.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] mips: cm: Convert __mips_cm_l2sync_phys_base() to weak function 2024-02-26 10:54 [PATCH v2 0/2] MIPS: Fix missing proto and passing arg warnings Serge Semin @ 2024-02-26 10:54 ` Serge Semin 2024-02-26 11:04 ` Arnd Bergmann 2024-02-26 10:54 ` [PATCH v2 2/2] mips: cm: Convert __mips_cm_phys_base() " Serge Semin 2024-03-11 13:05 ` [PATCH v2 0/2] MIPS: Fix missing proto and passing arg warnings Thomas Bogendoerfer 2 siblings, 1 reply; 11+ messages in thread From: Serge Semin @ 2024-02-26 10:54 UTC (permalink / raw) To: Thomas Bogendoerfer, Arnd Bergmann, Jiaxun Yang, Andrew Morton Cc: Serge Semin, Alexey Malahov, linux-mips, linux-kernel 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] mips: cm: Convert __mips_cm_l2sync_phys_base() to weak function 2024-02-26 10:54 ` [PATCH v2 1/2] mips: cm: Convert __mips_cm_l2sync_phys_base() to weak function Serge Semin @ 2024-02-26 11:04 ` Arnd Bergmann 2024-02-26 11:27 ` Serge Semin 0 siblings, 1 reply; 11+ messages in thread From: Arnd Bergmann @ 2024-02-26 11:04 UTC (permalink / raw) To: Serge Semin, Thomas Bogendoerfer, Jiaxun Yang, Andrew Morton Cc: Alexey Malahov, linux-mips, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] mips: cm: Convert __mips_cm_l2sync_phys_base() to weak function 2024-02-26 11:04 ` Arnd Bergmann @ 2024-02-26 11:27 ` Serge Semin 2024-02-26 12:04 ` Arnd Bergmann 0 siblings, 1 reply; 11+ messages in thread From: Serge Semin @ 2024-02-26 11:27 UTC (permalink / raw) To: Arnd Bergmann Cc: Thomas Bogendoerfer, Jiaxun Yang, Andrew Morton, Alexey Malahov, linux-mips, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] mips: cm: Convert __mips_cm_l2sync_phys_base() to weak function 2024-02-26 11:27 ` Serge Semin @ 2024-02-26 12:04 ` Arnd Bergmann 2024-02-26 12:20 ` Serge Semin 0 siblings, 1 reply; 11+ messages in thread From: Arnd Bergmann @ 2024-02-26 12:04 UTC (permalink / raw) To: Serge Semin Cc: Thomas Bogendoerfer, Jiaxun Yang, Andrew Morton, Alexey Malahov, linux-mips, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] mips: cm: Convert __mips_cm_l2sync_phys_base() to weak function 2024-02-26 12:04 ` Arnd Bergmann @ 2024-02-26 12:20 ` Serge Semin 2024-02-26 12:29 ` Arnd Bergmann 0 siblings, 1 reply; 11+ messages in thread From: Serge Semin @ 2024-02-26 12:20 UTC (permalink / raw) To: Arnd Bergmann Cc: Thomas Bogendoerfer, Jiaxun Yang, Andrew Morton, Alexey Malahov, linux-mips, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] mips: cm: Convert __mips_cm_l2sync_phys_base() to weak function 2024-02-26 12:20 ` Serge Semin @ 2024-02-26 12:29 ` Arnd Bergmann 2024-02-26 13:11 ` Serge Semin 0 siblings, 1 reply; 11+ messages in thread From: Arnd Bergmann @ 2024-02-26 12:29 UTC (permalink / raw) To: Serge Semin Cc: Thomas Bogendoerfer, Jiaxun Yang, Andrew Morton, Alexey Malahov, linux-mips, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] mips: cm: Convert __mips_cm_l2sync_phys_base() to weak function 2024-02-26 12:29 ` Arnd Bergmann @ 2024-02-26 13:11 ` Serge Semin 2024-03-11 13:07 ` Thomas Bogendoerfer 0 siblings, 1 reply; 11+ messages in thread From: Serge Semin @ 2024-02-26 13:11 UTC (permalink / raw) To: Arnd Bergmann Cc: Thomas Bogendoerfer, Jiaxun Yang, Andrew Morton, Alexey Malahov, linux-mips, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] mips: cm: Convert __mips_cm_l2sync_phys_base() to weak function 2024-02-26 13:11 ` Serge Semin @ 2024-03-11 13:07 ` Thomas Bogendoerfer 0 siblings, 0 replies; 11+ messages in thread From: Thomas Bogendoerfer @ 2024-03-11 13:07 UTC (permalink / raw) To: Serge Semin Cc: Arnd Bergmann, Jiaxun Yang, Andrew Morton, Alexey Malahov, linux-mips, linux-kernel 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 ] ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] mips: cm: Convert __mips_cm_phys_base() to weak function 2024-02-26 10:54 [PATCH v2 0/2] MIPS: Fix missing proto and passing arg warnings Serge Semin 2024-02-26 10:54 ` [PATCH v2 1/2] mips: cm: Convert __mips_cm_l2sync_phys_base() to weak function Serge Semin @ 2024-02-26 10:54 ` Serge Semin 2024-03-11 13:05 ` [PATCH v2 0/2] MIPS: Fix missing proto and passing arg warnings Thomas Bogendoerfer 2 siblings, 0 replies; 11+ messages in thread From: Serge Semin @ 2024-02-26 10:54 UTC (permalink / raw) To: Thomas Bogendoerfer, Arnd Bergmann, Jiaxun Yang Cc: Serge Semin, Alexey Malahov, Andrew Morton, linux-mips, linux-kernel Based on the design pattern utilized in the CM GCR base address getter implementation, the platform-specific code is capable to re-define the getter and re-use the weakly defined initial version. But since the pattern hasn't been used for over 10 years and another similar case (CM L2-sync only base address getter) has just been fixed, let's unify the interface and convert it to a more traditional single weakly defined method: mips_cm_phys_base() (see the link below for the discussion around this). 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 | 7 +++---- arch/mips/kernel/mips-cm.c | 5 +---- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/arch/mips/include/asm/mips-cm.h b/arch/mips/include/asm/mips-cm.h index 6cc79296c8ef..c4e27970d88f 100644 --- a/arch/mips/include/asm/mips-cm.h +++ b/arch/mips/include/asm/mips-cm.h @@ -22,16 +22,15 @@ extern void __iomem *mips_gcr_base; extern void __iomem *mips_cm_l2sync_base; /** - * __mips_cm_phys_base - retrieve the physical base address of the CM + * mips_cm_phys_base - retrieve the physical base address of the CM * * This function returns the physical base address of the Coherence Manager * global control block, or 0 if no Coherence Manager is present. It provides * a default implementation which reads the CMGCRBase register where available, * and may be overridden by platforms which determine this address in a - * different way by defining a function with the same prototype except for the - * name mips_cm_phys_base (without underscores). + * different way by defining a function with the same prototype. */ -extern phys_addr_t __mips_cm_phys_base(void); +extern phys_addr_t mips_cm_phys_base(void); /** * mips_cm_l2sync_phys_base - retrieve the physical base address of the CM diff --git a/arch/mips/kernel/mips-cm.c b/arch/mips/kernel/mips-cm.c index 268ac0b811e3..3a115fab5573 100644 --- a/arch/mips/kernel/mips-cm.c +++ b/arch/mips/kernel/mips-cm.c @@ -179,7 +179,7 @@ static char *cm3_causes[32] = { static DEFINE_PER_CPU_ALIGNED(spinlock_t, cm_core_lock); static DEFINE_PER_CPU_ALIGNED(unsigned long, cm_core_lock_flags); -phys_addr_t __mips_cm_phys_base(void) +phys_addr_t __weak mips_cm_phys_base(void) { unsigned long cmgcr; @@ -198,9 +198,6 @@ phys_addr_t __mips_cm_phys_base(void) return (cmgcr & MIPS_CMGCRF_BASE) << (36 - 32); } -phys_addr_t mips_cm_phys_base(void) - __attribute__((weak, alias("__mips_cm_phys_base"))); - phys_addr_t __weak mips_cm_l2sync_phys_base(void) { u32 base_reg; -- 2.43.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/2] MIPS: Fix missing proto and passing arg warnings 2024-02-26 10:54 [PATCH v2 0/2] MIPS: Fix missing proto and passing arg warnings Serge Semin 2024-02-26 10:54 ` [PATCH v2 1/2] mips: cm: Convert __mips_cm_l2sync_phys_base() to weak function Serge Semin 2024-02-26 10:54 ` [PATCH v2 2/2] mips: cm: Convert __mips_cm_phys_base() " Serge Semin @ 2024-03-11 13:05 ` Thomas Bogendoerfer 2 siblings, 0 replies; 11+ messages in thread From: Thomas Bogendoerfer @ 2024-03-11 13:05 UTC (permalink / raw) To: Serge Semin Cc: Arnd Bergmann, Jiaxun Yang, Alexey Malahov, Andrew Morton, linux-mips, linux-kernel On Mon, Feb 26, 2024 at 01:54:20PM +0300, Serge Semin wrote: > After getting my local tree rebased onto the kernel 6.8-rc3 the MIPS32 > kernel build procedure produced a couple of warnings which I suggest to > fix in the framework of this series. > > A first warning is of the "no previous prototype for `<func>`" type. In > particular my arch-specific code has the mips_cm_l2sync_phys_base() method > re-defined, but even though the function is global it' prototype isn't > declared anywhere. Fix that by moving the __mips_cm_l2sync_phys_base() > body to a weak implementation of mips_cm_l2sync_phys_base() and adding the > method prototype declaration to the mips/include/asm/mips-cm.h header > file. For the sake of unification a similar solution was provided for the > mips_cm_phys_base()/__mips_cm_phys_base() couple. > > The following text describes the patches which have already merged in at > v1 stage of the patchset (see changelog v2). > > One more case of the denoted earlier warning I spotted in the > self-extracting kernel (so called zboot) with the debug printouts enabled. > In particular there are several putc() method re-definitions available in: > arch/mips/boot/compressed/uart-prom.c > arch/mips/boot/compressed/uart-16550.c > arch/mips/boot/compressed/uart-alchemy.c > All of these files lacked the prototype declaration what caused having the > "no previous prototype for ‘putc’" printed on my build with the next > configs enabled: > CONFIG_SYS_SUPPORTS_ZBOOT=y > CONFIG_SYS_SUPPORTS_ZBOOT_UART_PROM=y > CONFIG_ZBOOT_LOAD_ADDRESS=0x85100000 > CONFIG_DEBUG_ZBOOT=y > > The second warning is of the "passing argument <x> of ‘<func>’ from > incompatible pointer type" type which I discovered in the > drivers/tty/mips_ejtag_fdc.c driver. The problem most likely happened due > to the commit ce7cbd9a6c81 ("tty: mips_ejtag_fdc: use u8 for character > pointers"). > > That's it for today.) Thanks for review in advance. Any tests are very > welcome. > > Link: https://lore.kernel.org/linux-mips/20240215171740.14550-1-fancer.lancer@gmail.com > Changelog v2: > - Drop aleady applied pateches: > [PATCH 3/4] mips: zboot: Fix "no previous prototype" build warning > [PATCH 4/4] tty: mips_ejtag_fdc: Fix passing incompatible pointer type warning > - Drop Linux serial mailing list and the respective maintainers from the > Cc-list. > - Covert the underscored versions of the CM2/L2-sync base address > getters to being the body of the weakly defined original methods. > > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: linux-mips@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > > Serge Semin (2): > mips: cm: Convert __mips_cm_l2sync_phys_base() to weak function > mips: cm: Convert __mips_cm_phys_base() to weak function > > arch/mips/include/asm/mips-cm.h | 20 ++++++++++++++++---- > arch/mips/kernel/mips-cm.c | 10 ++-------- > 2 files changed, 18 insertions(+), 12 deletions(-) > > -- > 2.43.0 series applied to mips-next. Thomas. -- Crap can work. Given enough thrust pigs will fly, but it's not necessarily a good idea. [ RFC1925, 2.3 ] ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-03-11 13:34 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-02-26 10:54 [PATCH v2 0/2] MIPS: Fix missing proto and passing arg warnings Serge Semin 2024-02-26 10:54 ` [PATCH v2 1/2] mips: cm: Convert __mips_cm_l2sync_phys_base() to weak function Serge Semin 2024-02-26 11:04 ` Arnd Bergmann 2024-02-26 11:27 ` Serge Semin 2024-02-26 12:04 ` Arnd Bergmann 2024-02-26 12:20 ` Serge Semin 2024-02-26 12:29 ` Arnd Bergmann 2024-02-26 13:11 ` Serge Semin 2024-03-11 13:07 ` Thomas Bogendoerfer 2024-02-26 10:54 ` [PATCH v2 2/2] mips: cm: Convert __mips_cm_phys_base() " Serge Semin 2024-03-11 13:05 ` [PATCH v2 0/2] MIPS: Fix missing proto and passing arg warnings Thomas Bogendoerfer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).