| Message ID | 20240616173105.7430-2-heinrich.schuchardt@canonical.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | cmd: avoid duplicate weak functions | expand |
On Sun, 16 Jun 2024 at 20:31, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > If we have multiple weak implementations of functions, the linker might > choose any of these. ARM and RISC-V already provide a weak implementation > of flush_dcache_all(). > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > cmd/cache.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/cmd/cache.c b/cmd/cache.c > index 0254ff17f9b..16fa0f7c652 100644 > --- a/cmd/cache.c > +++ b/cmd/cache.c > @@ -52,11 +52,14 @@ static int do_icache(struct cmd_tbl *cmdtp, int flag, int argc, > return 0; > } > > +/* ARM and RISC-V define a weak flush_dcache_all() themselves. */ > +#if !defined(CONFIG_ARM) && !defined(CONFIG_RISCV) > void __weak flush_dcache_all(void) > { > puts("No arch specific flush_dcache_all available!\n"); > /* please define arch specific flush_dcache_all */ > } Aren't we supposed to add a single __weak function so the linker can replace it? IOW why is the declaration for Arm/riscv a weak one? Thanks /Ilias > +#endif > > static int do_dcache(struct cmd_tbl *cmdtp, int flag, int argc, > char *const argv[]) > -- > 2.43.0 >
On 19.06.24 14:23, Ilias Apalodimas wrote: > On Sun, 16 Jun 2024 at 20:31, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: >> >> If we have multiple weak implementations of functions, the linker might >> choose any of these. ARM and RISC-V already provide a weak implementation >> of flush_dcache_all(). >> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> --- >> cmd/cache.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/cmd/cache.c b/cmd/cache.c >> index 0254ff17f9b..16fa0f7c652 100644 >> --- a/cmd/cache.c >> +++ b/cmd/cache.c >> @@ -52,11 +52,14 @@ static int do_icache(struct cmd_tbl *cmdtp, int flag, int argc, >> return 0; >> } >> >> +/* ARM and RISC-V define a weak flush_dcache_all() themselves. */ >> +#if !defined(CONFIG_ARM) && !defined(CONFIG_RISCV) >> void __weak flush_dcache_all(void) >> { >> puts("No arch specific flush_dcache_all available!\n"); >> /* please define arch specific flush_dcache_all */ >> } > > Aren't we supposed to add a single __weak function so the linker can > replace it? IOW why is the declaration for Arm/riscv a weak one? Some sub-architectures override the architecture specific weak implementation, e.g. arch/riscv/cpu/andes/cache.c:44: void flush_dcache_all(void) arch/arm/cpu/arm926ejs/cache.c:17: void flush_dcache_all(void) Best regards Heinrich > > Thanks > /Ilias >> +#endif >> >> static int do_dcache(struct cmd_tbl *cmdtp, int flag, int argc, >> char *const argv[]) >> -- >> 2.43.0 >>
On Wed, 19 Jun 2024 at 15:36, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > On 19.06.24 14:23, Ilias Apalodimas wrote: > > On Sun, 16 Jun 2024 at 20:31, Heinrich Schuchardt > > <heinrich.schuchardt@canonical.com> wrote: > >> > >> If we have multiple weak implementations of functions, the linker might > >> choose any of these. ARM and RISC-V already provide a weak implementation > >> of flush_dcache_all(). > >> > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >> --- > >> cmd/cache.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/cmd/cache.c b/cmd/cache.c > >> index 0254ff17f9b..16fa0f7c652 100644 > >> --- a/cmd/cache.c > >> +++ b/cmd/cache.c > >> @@ -52,11 +52,14 @@ static int do_icache(struct cmd_tbl *cmdtp, int flag, int argc, > >> return 0; > >> } > >> > >> +/* ARM and RISC-V define a weak flush_dcache_all() themselves. */ > >> +#if !defined(CONFIG_ARM) && !defined(CONFIG_RISCV) > >> void __weak flush_dcache_all(void) > >> { > >> puts("No arch specific flush_dcache_all available!\n"); > >> /* please define arch specific flush_dcache_all */ > >> } > > > > Aren't we supposed to add a single __weak function so the linker can > > replace it? IOW why is the declaration for Arm/riscv a weak one? > > Some sub-architectures override the architecture specific weak > implementation, e.g. > > arch/riscv/cpu/andes/cache.c:44: > void flush_dcache_all(void) > > arch/arm/cpu/arm926ejs/cache.c:17: > void flush_dcache_all(void) Ok, this does fix a problem, but afaict this is a band-aid and the cache management is a mess overall -- e.g invalidate_icache_all() will suffer from the same issue if a sub-architecture decides to define its own in the future. Would it be less bad to define static inline __weak flush_dcache_all(void) { } static inline __weak flush_icache_all(void) { } in include/cpu_func.h instead of a random cmd file? We can only define those if !CONFIG_ARM && !!CONFIG_RISCV and add a comment on why. It's kind of putting lipstick on a pig, but at least we'll have them gathered in a single header file and know what we have to fix. Cheers /Ilias > > Best regards > > Heinrich > > > > > Thanks > > /Ilias > >> +#endif > >> > >> static int do_dcache(struct cmd_tbl *cmdtp, int flag, int argc, > >> char *const argv[]) > >> -- > >> 2.43.0 > >> >
On Wed, 19 Jun 2024 at 16:05, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Wed, 19 Jun 2024 at 15:36, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: > > > > On 19.06.24 14:23, Ilias Apalodimas wrote: > > > On Sun, 16 Jun 2024 at 20:31, Heinrich Schuchardt > > > <heinrich.schuchardt@canonical.com> wrote: > > >> > > >> If we have multiple weak implementations of functions, the linker might > > >> choose any of these. ARM and RISC-V already provide a weak implementation > > >> of flush_dcache_all(). > > >> > > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > > >> --- > > >> cmd/cache.c | 3 +++ > > >> 1 file changed, 3 insertions(+) > > >> > > >> diff --git a/cmd/cache.c b/cmd/cache.c > > >> index 0254ff17f9b..16fa0f7c652 100644 > > >> --- a/cmd/cache.c > > >> +++ b/cmd/cache.c > > >> @@ -52,11 +52,14 @@ static int do_icache(struct cmd_tbl *cmdtp, int flag, int argc, > > >> return 0; > > >> } > > >> > > >> +/* ARM and RISC-V define a weak flush_dcache_all() themselves. */ > > >> +#if !defined(CONFIG_ARM) && !defined(CONFIG_RISCV) > > >> void __weak flush_dcache_all(void) > > >> { > > >> puts("No arch specific flush_dcache_all available!\n"); > > >> /* please define arch specific flush_dcache_all */ > > >> } > > > > > > Aren't we supposed to add a single __weak function so the linker can > > > replace it? IOW why is the declaration for Arm/riscv a weak one? > > > > Some sub-architectures override the architecture specific weak > > implementation, e.g. > > > > arch/riscv/cpu/andes/cache.c:44: > > void flush_dcache_all(void) > > > > arch/arm/cpu/arm926ejs/cache.c:17: > > void flush_dcache_all(void) > > Ok, this does fix a problem, but afaict this is a band-aid and the > cache management is a mess overall -- e.g invalidate_icache_all() will > suffer from the same issue if a sub-architecture decides to define its > own in the future. > > Would it be less bad to define > static inline __weak flush_dcache_all(void) > { > } > static inline __weak flush_icache_all(void) > { > } ugh, this but without the inline! Thanks /Ilias > in include/cpu_func.h instead of a random cmd file? > We can only define those if !CONFIG_ARM && !!CONFIG_RISCV and add a > comment on why. > It's kind of putting lipstick on a pig, but at least we'll have them > gathered in a single header file and know what we have to fix. > > Cheers > /Ilias > > > > Best regards > > > > Heinrich > > > > > > > > Thanks > > > /Ilias > > >> +#endif > > >> > > >> static int do_dcache(struct cmd_tbl *cmdtp, int flag, int argc, > > >> char *const argv[]) > > >> -- > > >> 2.43.0 > > >> > >
On 19.06.24 15:19, Ilias Apalodimas wrote: > On Wed, 19 Jun 2024 at 16:05, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: >> >> On Wed, 19 Jun 2024 at 15:36, Heinrich Schuchardt >> <heinrich.schuchardt@canonical.com> wrote: >>> >>> On 19.06.24 14:23, Ilias Apalodimas wrote: >>>> On Sun, 16 Jun 2024 at 20:31, Heinrich Schuchardt >>>> <heinrich.schuchardt@canonical.com> wrote: >>>>> >>>>> If we have multiple weak implementations of functions, the linker might >>>>> choose any of these. ARM and RISC-V already provide a weak implementation >>>>> of flush_dcache_all(). >>>>> >>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >>>>> --- >>>>> cmd/cache.c | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/cmd/cache.c b/cmd/cache.c >>>>> index 0254ff17f9b..16fa0f7c652 100644 >>>>> --- a/cmd/cache.c >>>>> +++ b/cmd/cache.c >>>>> @@ -52,11 +52,14 @@ static int do_icache(struct cmd_tbl *cmdtp, int flag, int argc, >>>>> return 0; >>>>> } >>>>> >>>>> +/* ARM and RISC-V define a weak flush_dcache_all() themselves. */ >>>>> +#if !defined(CONFIG_ARM) && !defined(CONFIG_RISCV) >>>>> void __weak flush_dcache_all(void) >>>>> { >>>>> puts("No arch specific flush_dcache_all available!\n"); >>>>> /* please define arch specific flush_dcache_all */ >>>>> } >>>> >>>> Aren't we supposed to add a single __weak function so the linker can >>>> replace it? IOW why is the declaration for Arm/riscv a weak one? >>> >>> Some sub-architectures override the architecture specific weak >>> implementation, e.g. >>> >>> arch/riscv/cpu/andes/cache.c:44: >>> void flush_dcache_all(void) >>> >>> arch/arm/cpu/arm926ejs/cache.c:17: >>> void flush_dcache_all(void) >> >> Ok, this does fix a problem, but afaict this is a band-aid and the >> cache management is a mess overall -- e.g invalidate_icache_all() will >> suffer from the same issue if a sub-architecture decides to define its >> own in the future. >> >> Would it be less bad to define >> static inline __weak flush_dcache_all(void) >> { >> } >> static inline __weak flush_icache_all(void) >> { >> } > > ugh, this but without the inline! > > Thanks > /Ilias GCC does not allow both a weak and a strong implementation in the same module: CC board/sandbox/sandbox.o arch/sandbox/cpu/cache.c:15:6: error: redefinition of ‘invalidate_icache_all’ 15 | void invalidate_icache_all(void) | ^~~~~~~~~~~~~~~~~~~~~ In file included from arch/sandbox/cpu/cache.c:6: include/cpu_func.h:74:13: note: previous definition of ‘invalidate_icache_all’ with type ‘void(void)’ 74 | void __weak invalidate_icache_all(void) | ^~~~~~~~~~~~~~~~~~~~~ Best regards Heinrich >> in include/cpu_func.h instead of a random cmd file? >> We can only define those if !CONFIG_ARM && !!CONFIG_RISCV and add a >> comment on why. >> It's kind of putting lipstick on a pig, but at least we'll have them >> gathered in a single header file and know what we have to fix. >> >> Cheers >> /Ilias >>> >>> Best regards >>> >>> Heinrich >>> >>>> >>>> Thanks >>>> /Ilias >>>>> +#endif >>>>> >>>>> static int do_dcache(struct cmd_tbl *cmdtp, int flag, int argc, >>>>> char *const argv[]) >>>>> -- >>>>> 2.43.0 >>>>> >>>
On Wed, Jun 19, 2024 at 03:23:36PM +0300, Ilias Apalodimas wrote: > On Sun, 16 Jun 2024 at 20:31, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: > > > > If we have multiple weak implementations of functions, the linker might > > choose any of these. ARM and RISC-V already provide a weak implementation > > of flush_dcache_all(). > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > > --- > > cmd/cache.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/cmd/cache.c b/cmd/cache.c > > index 0254ff17f9b..16fa0f7c652 100644 > > --- a/cmd/cache.c > > +++ b/cmd/cache.c > > @@ -52,11 +52,14 @@ static int do_icache(struct cmd_tbl *cmdtp, int flag, int argc, > > return 0; > > } > > > > +/* ARM and RISC-V define a weak flush_dcache_all() themselves. */ > > +#if !defined(CONFIG_ARM) && !defined(CONFIG_RISCV) > > void __weak flush_dcache_all(void) > > { > > puts("No arch specific flush_dcache_all available!\n"); > > /* please define arch specific flush_dcache_all */ > > } > > Aren't we supposed to add a single __weak function so the linker can > replace it? IOW why is the declaration for Arm/riscv a weak one? Yeah, I'm going to see about re-structuring this a little bit right now.
diff --git a/cmd/cache.c b/cmd/cache.c index 0254ff17f9b..16fa0f7c652 100644 --- a/cmd/cache.c +++ b/cmd/cache.c @@ -52,11 +52,14 @@ static int do_icache(struct cmd_tbl *cmdtp, int flag, int argc, return 0; } +/* ARM and RISC-V define a weak flush_dcache_all() themselves. */ +#if !defined(CONFIG_ARM) && !defined(CONFIG_RISCV) void __weak flush_dcache_all(void) { puts("No arch specific flush_dcache_all available!\n"); /* please define arch specific flush_dcache_all */ } +#endif static int do_dcache(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
If we have multiple weak implementations of functions, the linker might choose any of these. ARM and RISC-V already provide a weak implementation of flush_dcache_all(). Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- cmd/cache.c | 3 +++ 1 file changed, 3 insertions(+)