From: Barry Song <[email protected]>
xtensa's flush_dcache_page() can be a no-op sometimes. There is a
generic implementation for this case in include/asm-generic/
cacheflush.h.
#ifndef ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE
static inline void flush_dcache_page(struct page *page)
{
}
#define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
#endif
So remove the superfluous flush_dcache_page() definition, which also
helps silence potential build warnings complaining the page variable
passed to flush_dcache_page() is not used.
In file included from crypto/scompress.c:12:
include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
76 | struct page *page;
| ^~~~
crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
>> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
174 | struct page *dst_page = sg_page(req->dst);
|
The issue was originally reported on LoongArch by kernel test
robot (Huacai fixed it on LoongArch), then reported by Guenter
and me on xtensa.
This patch also removes lots of redundant macros which have
been defined by asm-generic/cacheflush.h.
Cc: Huacai Chen <[email protected]>
Cc: Herbert Xu <[email protected]>
Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Reported-by: Barry Song <[email protected]>
Closes: https://lore.kernel.org/all/CAGsJ_4yDk1+axbte7FKQEwD7X2oxUCFrEc9M5YOS1BobfDFXPA@mail.gmail.com/
Reported-by: Guenter Roeck <[email protected]>
Closes: https://lore.kernel.org/all/[email protected]/
Fixes: 77292bb8ca69 ("crypto: scomp - remove memcpy if sg_nents is 1 and pages are lowmem")
Signed-off-by: Barry Song <[email protected]>
---
-v2: include asm-generic/cacheflush.h and remove lots of redundant macros
arch/xtensa/include/asm/cacheflush.h | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/arch/xtensa/include/asm/cacheflush.h b/arch/xtensa/include/asm/cacheflush.h
index 38bcecb0e457..a2b6bb5429f5 100644
--- a/arch/xtensa/include/asm/cacheflush.h
+++ b/arch/xtensa/include/asm/cacheflush.h
@@ -100,6 +100,10 @@ void flush_cache_range(struct vm_area_struct*, ulong, ulong);
void flush_icache_range(unsigned long start, unsigned long end);
void flush_cache_page(struct vm_area_struct*,
unsigned long, unsigned long);
+#define flush_cache_all flush_cache_all
+#define flush_cache_range flush_cache_range
+#define flush_icache_range flush_icache_range
+#define flush_cache_page flush_cache_page
#else
#define flush_cache_all local_flush_cache_all
#define flush_cache_range local_flush_cache_range
@@ -136,20 +140,7 @@ void local_flush_cache_page(struct vm_area_struct *vma,
#else
-#define flush_cache_all() do { } while (0)
-#define flush_cache_mm(mm) do { } while (0)
-#define flush_cache_dup_mm(mm) do { } while (0)
-
-#define flush_cache_vmap(start,end) do { } while (0)
-#define flush_cache_vmap_early(start,end) do { } while (0)
-#define flush_cache_vunmap(start,end) do { } while (0)
-
-#define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
-#define flush_dcache_page(page) do { } while (0)
-
#define flush_icache_range local_flush_icache_range
-#define flush_cache_page(vma, addr, pfn) do { } while (0)
-#define flush_cache_range(vma, start, end) do { } while (0)
#endif
@@ -162,15 +153,14 @@ void local_flush_cache_page(struct vm_area_struct *vma,
__invalidate_icache_range(start,(end) - (start)); \
} while (0)
-#define flush_dcache_mmap_lock(mapping) do { } while (0)
-#define flush_dcache_mmap_unlock(mapping) do { } while (0)
-
#if defined(CONFIG_MMU) && (DCACHE_WAY_SIZE > PAGE_SIZE)
extern void copy_to_user_page(struct vm_area_struct*, struct page*,
unsigned long, void*, const void*, unsigned long);
extern void copy_from_user_page(struct vm_area_struct*, struct page*,
unsigned long, void*, const void*, unsigned long);
+#define copy_to_user_page copy_to_user_page
+#define copy_from_user_page copy_from_user_page
#else
@@ -186,4 +176,6 @@ extern void copy_from_user_page(struct vm_area_struct*, struct page*,
#endif
+#include <asm-generic/cacheflush.h>
+
#endif /* _XTENSA_CACHEFLUSH_H */
--
2.34.1
On Tue, Mar 19, 2024 at 2:09 PM Barry Song <[email protected]> wrote:
>
> From: Barry Song <[email protected]>
>
> xtensa's flush_dcache_page() can be a no-op sometimes. There is a
> generic implementation for this case in include/asm-generic/
> cacheflush.h.
> #ifndef ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE
> static inline void flush_dcache_page(struct page *page)
> {
> }
>
> #define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
> #endif
>
> So remove the superfluous flush_dcache_page() definition, which also
> helps silence potential build warnings complaining the page variable
> passed to flush_dcache_page() is not used.
>
> In file included from crypto/scompress.c:12:
> include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
> include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
> 76 | struct page *page;
> | ^~~~
> crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
> >> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
> 174 | struct page *dst_page = sg_page(req->dst);
> |
>
> The issue was originally reported on LoongArch by kernel test
> robot (Huacai fixed it on LoongArch), then reported by Guenter
> and me on xtensa.
>
> This patch also removes lots of redundant macros which have
> been defined by asm-generic/cacheflush.h.
>
> Cc: Huacai Chen <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> Reported-by: Barry Song <[email protected]>
> Closes: https://lore.kernel.org/all/CAGsJ_4yDk1+axbte7FKQEwD7X2oxUCFrEc9M5YOS1BobfDFXPA@mail.gmail.com/
> Reported-by: Guenter Roeck <[email protected]>
Hi Guenter,
I am not a xtensa guy, so I will need your help for a full test. if
turns out it is a too big(ambitious)
fix, a minimal fix might be:
diff --git a/arch/xtensa/include/asm/cacheflush.h
b/arch/xtensa/include/asm/cacheflush.h
index 38bcecb0e457..fdc692cf2b78 100644
--- a/arch/xtensa/include/asm/cacheflush.h
+++ b/arch/xtensa/include/asm/cacheflush.h
@@ -145,7 +145,7 @@ void local_flush_cache_page(struct vm_area_struct *vma,
#define flush_cache_vunmap(start,end) do { } while (0)
#define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
-#define flush_dcache_page(page) do { } while (0)
+#define flush_dcache_page(page) do {
(void)(page); } while (0)
#define flush_icache_range local_flush_icache_range
#define flush_cache_page(vma, addr, pfn) do { } while (0)
> Closes: https://lore.kernel.org/all/[email protected]/
> Fixes: 77292bb8ca69 ("crypto: scomp - remove memcpy if sg_nents is 1 and pages are lowmem")
> Signed-off-by: Barry Song <[email protected]>
> ---
> -v2: include asm-generic/cacheflush.h and remove lots of redundant macros
>
> arch/xtensa/include/asm/cacheflush.h | 24 ++++++++----------------
> 1 file changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/arch/xtensa/include/asm/cacheflush.h b/arch/xtensa/include/asm/cacheflush.h
> index 38bcecb0e457..a2b6bb5429f5 100644
> --- a/arch/xtensa/include/asm/cacheflush.h
> +++ b/arch/xtensa/include/asm/cacheflush.h
> @@ -100,6 +100,10 @@ void flush_cache_range(struct vm_area_struct*, ulong, ulong);
> void flush_icache_range(unsigned long start, unsigned long end);
> void flush_cache_page(struct vm_area_struct*,
> unsigned long, unsigned long);
> +#define flush_cache_all flush_cache_all
> +#define flush_cache_range flush_cache_range
> +#define flush_icache_range flush_icache_range
> +#define flush_cache_page flush_cache_page
> #else
> #define flush_cache_all local_flush_cache_all
> #define flush_cache_range local_flush_cache_range
> @@ -136,20 +140,7 @@ void local_flush_cache_page(struct vm_area_struct *vma,
>
> #else
>
> -#define flush_cache_all() do { } while (0)
> -#define flush_cache_mm(mm) do { } while (0)
> -#define flush_cache_dup_mm(mm) do { } while (0)
> -
> -#define flush_cache_vmap(start,end) do { } while (0)
> -#define flush_cache_vmap_early(start,end) do { } while (0)
> -#define flush_cache_vunmap(start,end) do { } while (0)
> -
> -#define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
> -#define flush_dcache_page(page) do { } while (0)
> -
> #define flush_icache_range local_flush_icache_range
> -#define flush_cache_page(vma, addr, pfn) do { } while (0)
> -#define flush_cache_range(vma, start, end) do { } while (0)
>
> #endif
>
> @@ -162,15 +153,14 @@ void local_flush_cache_page(struct vm_area_struct *vma,
> __invalidate_icache_range(start,(end) - (start)); \
> } while (0)
>
> -#define flush_dcache_mmap_lock(mapping) do { } while (0)
> -#define flush_dcache_mmap_unlock(mapping) do { } while (0)
> -
> #if defined(CONFIG_MMU) && (DCACHE_WAY_SIZE > PAGE_SIZE)
>
> extern void copy_to_user_page(struct vm_area_struct*, struct page*,
> unsigned long, void*, const void*, unsigned long);
> extern void copy_from_user_page(struct vm_area_struct*, struct page*,
> unsigned long, void*, const void*, unsigned long);
> +#define copy_to_user_page copy_to_user_page
> +#define copy_from_user_page copy_from_user_page
>
> #else
>
> @@ -186,4 +176,6 @@ extern void copy_from_user_page(struct vm_area_struct*, struct page*,
>
> #endif
>
> +#include <asm-generic/cacheflush.h>
> +
> #endif /* _XTENSA_CACHEFLUSH_H */
> --
> 2.34.1
>
Thanks
Barry
On Mon, Mar 18, 2024 at 6:27 PM Barry Song <[email protected]> wrote:
> On Tue, Mar 19, 2024 at 2:09 PM Barry Song <[email protected]> wrote:
> > From: Barry Song <[email protected]>
> >
> > xtensa's flush_dcache_page() can be a no-op sometimes. There is a
> > generic implementation for this case in include/asm-generic/
> > cacheflush.h.
> > #ifndef ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE
> > static inline void flush_dcache_page(struct page *page)
> > {
> > }
> >
> > #define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
> > #endif
> >
> > So remove the superfluous flush_dcache_page() definition, which also
> > helps silence potential build warnings complaining the page variable
> > passed to flush_dcache_page() is not used.
> >
> > In file included from crypto/scompress.c:12:
> > include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
> > include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
> > 76 | struct page *page;
> > | ^~~~
> > crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
> > >> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
> > 174 | struct page *dst_page = sg_page(req->dst);
> > |
> >
> > The issue was originally reported on LoongArch by kernel test
> > robot (Huacai fixed it on LoongArch), then reported by Guenter
> > and me on xtensa.
I wonder why this warning is considered useful in the first place?
If I'm missing something and it's indeed useful, should there be a
rule in the Documentation/process/coding-style.rst saying that
function-like macros should evaluate all their arguments?
> > This patch also removes lots of redundant macros which have
> > been defined by asm-generic/cacheflush.h.
> >
> > Cc: Huacai Chen <[email protected]>
> > Cc: Herbert Xu <[email protected]>
> > Reported-by: kernel test robot <[email protected]>
> > Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > Reported-by: Barry Song <[email protected]>
> > Closes: https://lore.kernel.org/all/CAGsJ_4yDk1+axbte7FKQEwD7X2oxUCFrEc9M5YOS1BobfDFXPA@mail.gmail.com/
> > Reported-by: Guenter Roeck <[email protected]>
>
> Hi Guenter,
> I am not a xtensa guy, so I will need your help for a full test. if
> turns out it is a too big(ambitious)
> fix, a minimal fix might be:
>
> diff --git a/arch/xtensa/include/asm/cacheflush.h
> b/arch/xtensa/include/asm/cacheflush.h
> index 38bcecb0e457..fdc692cf2b78 100644
> --- a/arch/xtensa/include/asm/cacheflush.h
> +++ b/arch/xtensa/include/asm/cacheflush.h
> @@ -145,7 +145,7 @@ void local_flush_cache_page(struct vm_area_struct *vma,
> #define flush_cache_vunmap(start,end) do { } while (0)
>
> #define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
> -#define flush_dcache_page(page) do { } while (0)
> +#define flush_dcache_page(page) do {
> (void)(page); } while (0)
This looks like a good fix, IMO better than adding __maybe_unused
to the variables reported in the warnings.
--
Thanks.
-- Max
On Tue, Mar 19, 2024 at 3:12 PM Max Filippov <[email protected]> wrote:
>
> On Mon, Mar 18, 2024 at 6:27 PM Barry Song <[email protected]> wrote:
> > On Tue, Mar 19, 2024 at 2:09 PM Barry Song <[email protected]> wrote:
> > > From: Barry Song <[email protected]>
> > >
> > > xtensa's flush_dcache_page() can be a no-op sometimes. There is a
> > > generic implementation for this case in include/asm-generic/
> > > cacheflush.h.
> > > #ifndef ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE
> > > static inline void flush_dcache_page(struct page *page)
> > > {
> > > }
> > >
> > > #define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
> > > #endif
> > >
> > > So remove the superfluous flush_dcache_page() definition, which also
> > > helps silence potential build warnings complaining the page variable
> > > passed to flush_dcache_page() is not used.
> > >
> > > In file included from crypto/scompress.c:12:
> > > include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
> > > include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
> > > 76 | struct page *page;
> > > | ^~~~
> > > crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
> > > >> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
> > > 174 | struct page *dst_page = sg_page(req->dst);
> > > |
> > >
> > > The issue was originally reported on LoongArch by kernel test
> > > robot (Huacai fixed it on LoongArch), then reported by Guenter
> > > and me on xtensa.
>
> I wonder why this warning is considered useful in the first place?
Guenter reported it as an error not a warning
"
Error log:
crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
crypto/scompress.c:174:38: error: unused variable 'dst_page'"
is it because xtensa's toolchain is different with others?
> If I'm missing something and it's indeed useful, should there be a
> rule in the Documentation/process/coding-style.rst saying that
> function-like macros should evaluate all their arguments?
>
> > > This patch also removes lots of redundant macros which have
> > > been defined by asm-generic/cacheflush.h.
> > >
> > > Cc: Huacai Chen <[email protected]>
> > > Cc: Herbert Xu <[email protected]>
> > > Reported-by: kernel test robot <[email protected]>
> > > Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > > Reported-by: Barry Song <[email protected]>
> > > Closes: https://lore.kernel.org/all/CAGsJ_4yDk1+axbte7FKQEwD7X2oxUCFrEc9M5YOS1BobfDFXPA@mail.gmail.com/
> > > Reported-by: Guenter Roeck <[email protected]>
> >
> > Hi Guenter,
> > I am not a xtensa guy, so I will need your help for a full test. if
> > turns out it is a too big(ambitious)
> > fix, a minimal fix might be:
> >
> > diff --git a/arch/xtensa/include/asm/cacheflush.h
> > b/arch/xtensa/include/asm/cacheflush.h
> > index 38bcecb0e457..fdc692cf2b78 100644
> > --- a/arch/xtensa/include/asm/cacheflush.h
> > +++ b/arch/xtensa/include/asm/cacheflush.h
> > @@ -145,7 +145,7 @@ void local_flush_cache_page(struct vm_area_struct *vma,
> > #define flush_cache_vunmap(start,end) do { } while (0)
> >
> > #define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
> > -#define flush_dcache_page(page) do { } while (0)
> > +#define flush_dcache_page(page) do {
> > (void)(page); } while (0)
>
> This looks like a good fix, IMO better than adding __maybe_unused
> to the variables reported in the warnings.
i don't like adding __maybe_unused either as it is not the fault
of the drivers calling flush_dcache_page().
>
> --
> Thanks.
> -- Max
On Tue, Mar 19, 2024 at 02:09:20PM +1300, Barry Song wrote:
> From: Barry Song <[email protected]>
>
> xtensa's flush_dcache_page() can be a no-op sometimes. There is a
> generic implementation for this case in include/asm-generic/
> cacheflush.h.
> #ifndef ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE
> static inline void flush_dcache_page(struct page *page)
> {
> }
>
> #define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
> #endif
>
> So remove the superfluous flush_dcache_page() definition, which also
> helps silence potential build warnings complaining the page variable
> passed to flush_dcache_page() is not used.
>
> In file included from crypto/scompress.c:12:
> include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
> include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
> 76 | struct page *page;
> | ^~~~
> crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
> >> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
> 174 | struct page *dst_page = sg_page(req->dst);
> |
>
> The issue was originally reported on LoongArch by kernel test
> robot (Huacai fixed it on LoongArch), then reported by Guenter
> and me on xtensa.
>
> This patch also removes lots of redundant macros which have
> been defined by asm-generic/cacheflush.h.
>
> Cc: Huacai Chen <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> Reported-by: Barry Song <[email protected]>
> Closes: https://lore.kernel.org/all/CAGsJ_4yDk1+axbte7FKQEwD7X2oxUCFrEc9M5YOS1BobfDFXPA@mail.gmail.com/
> Reported-by: Guenter Roeck <[email protected]>
> Closes: https://lore.kernel.org/all/[email protected]/
> Fixes: 77292bb8ca69 ("crypto: scomp - remove memcpy if sg_nents is 1 and pages are lowmem")
> Signed-off-by: Barry Song <[email protected]>
Tested-by: Guenter Roeck <[email protected]>
Guenter
On 3/18/24 19:16, Barry Song wrote:
> On Tue, Mar 19, 2024 at 3:12 PM Max Filippov <[email protected]> wrote:
>>
>> On Mon, Mar 18, 2024 at 6:27 PM Barry Song <[email protected]> wrote:
>>> On Tue, Mar 19, 2024 at 2:09 PM Barry Song <[email protected]> wrote:
>>>> From: Barry Song <[email protected]>
>>>>
>>>> xtensa's flush_dcache_page() can be a no-op sometimes. There is a
>>>> generic implementation for this case in include/asm-generic/
>>>> cacheflush.h.
>>>> #ifndef ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE
>>>> static inline void flush_dcache_page(struct page *page)
>>>> {
>>>> }
>>>>
>>>> #define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
>>>> #endif
>>>>
>>>> So remove the superfluous flush_dcache_page() definition, which also
>>>> helps silence potential build warnings complaining the page variable
>>>> passed to flush_dcache_page() is not used.
>>>>
>>>> In file included from crypto/scompress.c:12:
>>>> include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
>>>> include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
>>>> 76 | struct page *page;
>>>> | ^~~~
>>>> crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
>>>>>> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
>>>> 174 | struct page *dst_page = sg_page(req->dst);
>>>> |
>>>>
>>>> The issue was originally reported on LoongArch by kernel test
>>>> robot (Huacai fixed it on LoongArch), then reported by Guenter
>>>> and me on xtensa.
>>
>> I wonder why this warning is considered useful in the first place?
>
> Guenter reported it as an error not a warning
> "
>
> Error log:
> crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
> crypto/scompress.c:174:38: error: unused variable 'dst_page'"
>
> is it because xtensa's toolchain is different with others?
>
It is because -Werror is now default in allmodconfig builds.
Other test build systems may manually override that.
Guenter
On 3/18/24 18:27, Barry Song wrote:
> On Tue, Mar 19, 2024 at 2:09 PM Barry Song <[email protected]> wrote:
>>
>> From: Barry Song <[email protected]>
>>
>> xtensa's flush_dcache_page() can be a no-op sometimes. There is a
>> generic implementation for this case in include/asm-generic/
>> cacheflush.h.
>> #ifndef ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE
>> static inline void flush_dcache_page(struct page *page)
>> {
>> }
>>
>> #define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
>> #endif
>>
>> So remove the superfluous flush_dcache_page() definition, which also
>> helps silence potential build warnings complaining the page variable
>> passed to flush_dcache_page() is not used.
>>
>> In file included from crypto/scompress.c:12:
>> include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
>> include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
>> 76 | struct page *page;
>> | ^~~~
>> crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
>>>> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
>> 174 | struct page *dst_page = sg_page(req->dst);
>> |
>>
>> The issue was originally reported on LoongArch by kernel test
>> robot (Huacai fixed it on LoongArch), then reported by Guenter
>> and me on xtensa.
>>
>> This patch also removes lots of redundant macros which have
>> been defined by asm-generic/cacheflush.h.
>>
>> Cc: Huacai Chen <[email protected]>
>> Cc: Herbert Xu <[email protected]>
>> Reported-by: kernel test robot <[email protected]>
>> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>> Reported-by: Barry Song <[email protected]>
>> Closes: https://lore.kernel.org/all/CAGsJ_4yDk1+axbte7FKQEwD7X2oxUCFrEc9M5YOS1BobfDFXPA@mail.gmail.com/
>> Reported-by: Guenter Roeck <[email protected]>
>
> Hi Guenter,
> I am not a xtensa guy, so I will need your help for a full test. if
> turns out it is a too big(ambitious)
I sent a Tested-by: tag to your patch. As far as my testing goes, it works fine,
and I added your patch to my "testing" branch (which tries to be buildable
and testable for all architectures).
> fix, a minimal fix might be:
>
FWIW, I think a minimal fix would have been to mark the variable as __maybe_unused,
but as others have pointed out it would be nice if there would be a guidance
about optional API functions like this one that specifies if it may be
implemented as macro and, if so, how it has to handle unused variables.
Thanks,
Guenter
On Wed, Mar 20, 2024 at 3:18 AM Guenter Roeck <[email protected]> wrote:
>
> On 3/18/24 18:27, Barry Song wrote:
> > On Tue, Mar 19, 2024 at 2:09 PM Barry Song <[email protected]> wrote:
> >>
> >> From: Barry Song <[email protected]>
> >>
> >> xtensa's flush_dcache_page() can be a no-op sometimes. There is a
> >> generic implementation for this case in include/asm-generic/
> >> cacheflush.h.
> >> #ifndef ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE
> >> static inline void flush_dcache_page(struct page *page)
> >> {
> >> }
> >>
> >> #define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
> >> #endif
> >>
> >> So remove the superfluous flush_dcache_page() definition, which also
> >> helps silence potential build warnings complaining the page variable
> >> passed to flush_dcache_page() is not used.
> >>
> >> In file included from crypto/scompress.c:12:
> >> include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
> >> include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
> >> 76 | struct page *page;
> >> | ^~~~
> >> crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
> >>>> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
> >> 174 | struct page *dst_page = sg_page(req->dst);
> >> |
> >>
> >> The issue was originally reported on LoongArch by kernel test
> >> robot (Huacai fixed it on LoongArch), then reported by Guenter
> >> and me on xtensa.
> >>
> >> This patch also removes lots of redundant macros which have
> >> been defined by asm-generic/cacheflush.h.
> >>
> >> Cc: Huacai Chen <[email protected]>
> >> Cc: Herbert Xu <[email protected]>
> >> Reported-by: kernel test robot <[email protected]>
> >> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> >> Reported-by: Barry Song <[email protected]>
> >> Closes: https://lore.kernel.org/all/CAGsJ_4yDk1+axbte7FKQEwD7X2oxUCFrEc9M5YOS1BobfDFXPA@mail.gmail.com/
> >> Reported-by: Guenter Roeck <[email protected]>
> >
> > Hi Guenter,
> > I am not a xtensa guy, so I will need your help for a full test. if
> > turns out it is a too big(ambitious)
>
> I sent a Tested-by: tag to your patch. As far as my testing goes, it works fine,
> and I added your patch to my "testing" branch (which tries to be buildable
> and testable for all architectures).
Thank you very much, Guenter. It would be nice if xtensa can leverage
the common cacheflush.h just like other architectures.
>
> > fix, a minimal fix might be:
> >
>
> FWIW, I think a minimal fix would have been to mark the variable as __maybe_unused,
I am not quite sure we want this as the point is that drivers should
be independent of
architectural differences.
The driver code, for itself, seems quite innocent,
struct page *dst_page = sg_page(req->dst);
for (i = 0; i < nr_pages; i++)
flush_dcache_page(dst_page + i);
The only problem is that xtensa's flush_dcache_page is a macro and
doesn't use the "page"
parameter.
if we re-use the inline function in common cacheflush.h, it won't be a problem.
> but as others have pointed out it would be nice if there would be a guidance
> about optional API functions like this one that specifies if it may be
> implemented as macro and, if so, how it has to handle unused variables.
I agree. personally I like this and will have a go in
Documentation/process/coding-style.rst
and see others' opinions.
>
> Thanks,
> Guenter
Thanks
Barry
Hi,
On Tue, Mar 19, 2024 at 02:09:20PM +1300, Barry Song wrote:
> From: Barry Song <[email protected]>
>
> xtensa's flush_dcache_page() can be a no-op sometimes. There is a
> generic implementation for this case in include/asm-generic/
> cacheflush.h.
> #ifndef ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE
> static inline void flush_dcache_page(struct page *page)
> {
> }
>
> #define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
> #endif
>
> So remove the superfluous flush_dcache_page() definition, which also
> helps silence potential build warnings complaining the page variable
> passed to flush_dcache_page() is not used.
>
> In file included from crypto/scompress.c:12:
> include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
> include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
> 76 | struct page *page;
> | ^~~~
> crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
> >> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
> 174 | struct page *dst_page = sg_page(req->dst);
> |
>
> The issue was originally reported on LoongArch by kernel test
> robot (Huacai fixed it on LoongArch), then reported by Guenter
> and me on xtensa.
>
> This patch also removes lots of redundant macros which have
> been defined by asm-generic/cacheflush.h.
>
> Cc: Huacai Chen <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> Reported-by: Barry Song <[email protected]>
> Closes: https://lore.kernel.org/all/CAGsJ_4yDk1+axbte7FKQEwD7X2oxUCFrEc9M5YOS1BobfDFXPA@mail.gmail.com/
> Reported-by: Guenter Roeck <[email protected]>
> Closes: https://lore.kernel.org/all/[email protected]/
> Fixes: 77292bb8ca69 ("crypto: scomp - remove memcpy if sg_nents is 1 and pages are lowmem")
> Signed-off-by: Barry Song <[email protected]>
The mainline kernel still fails to build xtensa:allmodconfig.
Building xtensa:allmodconfig ... failed
--------------
Error log:
crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
crypto/scompress.c:174:38: error: unused variable 'dst_page' [-Werror=unused-variable]
174 | struct page *dst_page = sg_page(req->dst);
This patch fixes the problem. Is there a chance to get it applied to the
upstream kernel, or should I just stop build testing xtensa:allmodconfig ?
Thanks,
Guenter
On Sun, Apr 28, 2024 at 7:05 AM Guenter Roeck <[email protected]> wrote:
>
> Hi,
>
> On Tue, Mar 19, 2024 at 02:09:20PM +1300, Barry Song wrote:
> > From: Barry Song <[email protected]>
> >
> > xtensa's flush_dcache_page() can be a no-op sometimes. There is a
> > generic implementation for this case in include/asm-generic/
> > cacheflush.h.
> > #ifndef ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE
> > static inline void flush_dcache_page(struct page *page)
> > {
> > }
> >
> > #define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
> > #endif
> >
> > So remove the superfluous flush_dcache_page() definition, which also
> > helps silence potential build warnings complaining the page variable
> > passed to flush_dcache_page() is not used.
> >
> > In file included from crypto/scompress.c:12:
> > include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
> > include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
> > 76 | struct page *page;
> > | ^~~~
> > crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
> > >> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
> > 174 | struct page *dst_page = sg_page(req->dst);
> > |
> >
> > The issue was originally reported on LoongArch by kernel test
> > robot (Huacai fixed it on LoongArch), then reported by Guenter
> > and me on xtensa.
> >
> > This patch also removes lots of redundant macros which have
> > been defined by asm-generic/cacheflush.h.
> >
> > Cc: Huacai Chen <[email protected]>
> > Cc: Herbert Xu <[email protected]>
> > Reported-by: kernel test robot <[email protected]>
> > Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > Reported-by: Barry Song <[email protected]>
> > Closes: https://lore.kernel.org/all/CAGsJ_4yDk1+axbte7FKQEwD7X2oxUCFrEc9M5YOS1BobfDFXPA@mail.gmail.com/
> > Reported-by: Guenter Roeck <[email protected]>
> > Closes: https://lore.kernel.org/all/[email protected]/
> > Fixes: 77292bb8ca69 ("crypto: scomp - remove memcpy if sg_nents is 1 and pages are lowmem")
> > Signed-off-by: Barry Song <[email protected]>
>
> The mainline kernel still fails to build xtensa:allmodconfig.
>
> Building xtensa:allmodconfig ... failed
> --------------
> Error log:
> crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
> crypto/scompress.c:174:38: error: unused variable 'dst_page' [-Werror=unused-variable]
> 174 | struct page *dst_page = sg_page(req->dst);
>
> This patch fixes the problem. Is there a chance to get it applied to the
> upstream kernel, or should I just stop build testing xtensa:allmodconfig ?
I'd prefer this can be applied upstream.
Hi Andrew, would you like to pull it?
>
> Thanks,
> Guenter
Thanks
Barry
On Sat, Apr 27, 2024 at 12:05 PM Guenter Roeck <[email protected]> wrote:
>
> Hi,
>
> On Tue, Mar 19, 2024 at 02:09:20PM +1300, Barry Song wrote:
> > From: Barry Song <[email protected]>
> >
> > xtensa's flush_dcache_page() can be a no-op sometimes. There is a
> > generic implementation for this case in include/asm-generic/
> > cacheflush.h.
> > #ifndef ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE
> > static inline void flush_dcache_page(struct page *page)
> > {
> > }
> >
> > #define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
> > #endif
> >
> > So remove the superfluous flush_dcache_page() definition, which also
> > helps silence potential build warnings complaining the page variable
> > passed to flush_dcache_page() is not used.
> >
> > In file included from crypto/scompress.c:12:
> > include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
> > include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
> > 76 | struct page *page;
> > | ^~~~
> > crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
> > >> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
> > 174 | struct page *dst_page = sg_page(req->dst);
> > |
> >
> > The issue was originally reported on LoongArch by kernel test
> > robot (Huacai fixed it on LoongArch), then reported by Guenter
> > and me on xtensa.
> >
> > This patch also removes lots of redundant macros which have
> > been defined by asm-generic/cacheflush.h.
> >
> > Cc: Huacai Chen <[email protected]>
> > Cc: Herbert Xu <[email protected]>
> > Reported-by: kernel test robot <[email protected]>
> > Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > Reported-by: Barry Song <[email protected]>
> > Closes: https://lore.kernel.org/all/CAGsJ_4yDk1+axbte7FKQEwD7X2oxUCFrEc9M5YOS1BobfDFXPA@mail.gmail.com/
> > Reported-by: Guenter Roeck <[email protected]>
> > Closes: https://lore.kernel.org/all/[email protected]/
> > Fixes: 77292bb8ca69 ("crypto: scomp - remove memcpy if sg_nents is 1 and pages are lowmem")
> > Signed-off-by: Barry Song <[email protected]>
>
> The mainline kernel still fails to build xtensa:allmodconfig.
>
> Building xtensa:allmodconfig ... failed
> --------------
> Error log:
> crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
> crypto/scompress.c:174:38: error: unused variable 'dst_page' [-Werror=unused-variable]
> 174 | struct page *dst_page = sg_page(req->dst);
>
> This patch fixes the problem. Is there a chance to get it applied to the
> upstream kernel, or should I just stop build testing xtensa:allmodconfig ?
Applied to my xtensa tree.
I was still hoping to see rationale for why this is a useful warning.
--
Thanks.
-- Max
On Mon, Apr 29, 2024 at 5:10 PM Max Filippov <[email protected]> wrote:
>
> On Sat, Apr 27, 2024 at 12:05 PM Guenter Roeck <[email protected]> wrote:
> >
> > Hi,
> >
> > On Tue, Mar 19, 2024 at 02:09:20PM +1300, Barry Song wrote:
> > > From: Barry Song <[email protected]>
> > >
> > > xtensa's flush_dcache_page() can be a no-op sometimes. There is a
> > > generic implementation for this case in include/asm-generic/
> > > cacheflush.h.
> > > #ifndef ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE
> > > static inline void flush_dcache_page(struct page *page)
> > > {
> > > }
> > >
> > > #define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
> > > #endif
> > >
> > > So remove the superfluous flush_dcache_page() definition, which also
> > > helps silence potential build warnings complaining the page variable
> > > passed to flush_dcache_page() is not used.
> > >
> > > In file included from crypto/scompress.c:12:
> > > include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
> > > include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
> > > 76 | struct page *page;
> > > | ^~~~
> > > crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
> > > >> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
> > > 174 | struct page *dst_page = sg_page(req->dst);
> > > |
> > >
> > > The issue was originally reported on LoongArch by kernel test
> > > robot (Huacai fixed it on LoongArch), then reported by Guenter
> > > and me on xtensa.
> > >
> > > This patch also removes lots of redundant macros which have
> > > been defined by asm-generic/cacheflush.h.
> > >
> > > Cc: Huacai Chen <[email protected]>
> > > Cc: Herbert Xu <[email protected]>
> > > Reported-by: kernel test robot <[email protected]>
> > > Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > > Reported-by: Barry Song <[email protected]>
> > > Closes: https://lore.kernel.org/all/CAGsJ_4yDk1+axbte7FKQEwD7X2oxUCFrEc9M5YOS1BobfDFXPA@mail.gmail.com/
> > > Reported-by: Guenter Roeck <[email protected]>
> > > Closes: https://lore.kernel.org/all/[email protected]/
> > > Fixes: 77292bb8ca69 ("crypto: scomp - remove memcpy if sg_nents is 1 and pages are lowmem")
> > > Signed-off-by: Barry Song <[email protected]>
> >
> > The mainline kernel still fails to build xtensa:allmodconfig.
> >
> > Building xtensa:allmodconfig ... failed
> > --------------
> > Error log:
> > crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
> > crypto/scompress.c:174:38: error: unused variable 'dst_page' [-Werror=unused-variable]
> > 174 | struct page *dst_page = sg_page(req->dst);
> >
> > This patch fixes the problem. Is there a chance to get it applied to the
> > upstream kernel, or should I just stop build testing xtensa:allmodconfig ?
>
> Applied to my xtensa tree.
Thanks for taking care of this, Max!
> I was still hoping to see rationale for why this is a useful warning.
Personally, I think the reason is quite obvious.
This warning serves as a valuable alert, highlighting an issue where Xtensa
fails to properly implement flush_dcache_page(). The problem lies in a macro
definition that neglects to evaluate its argument. Such discrepancies can
impact driver functionality across various architectures, rendering them
independent of specific architectural considerations.
>
> --
> Thanks.
> -- Max
Thanks
Barry
On 4/28/24 22:09, Max Filippov wrote:
> On Sat, Apr 27, 2024 at 12:05 PM Guenter Roeck <[email protected]> wrote:
>>
>> Hi,
>>
>> On Tue, Mar 19, 2024 at 02:09:20PM +1300, Barry Song wrote:
>>> From: Barry Song <[email protected]>
>>>
>>> xtensa's flush_dcache_page() can be a no-op sometimes. There is a
>>> generic implementation for this case in include/asm-generic/
>>> cacheflush.h.
>>> #ifndef ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE
>>> static inline void flush_dcache_page(struct page *page)
>>> {
>>> }
>>>
>>> #define ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 0
>>> #endif
>>>
>>> So remove the superfluous flush_dcache_page() definition, which also
>>> helps silence potential build warnings complaining the page variable
>>> passed to flush_dcache_page() is not used.
>>>
>>> In file included from crypto/scompress.c:12:
>>> include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
>>> include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
>>> 76 | struct page *page;
>>> | ^~~~
>>> crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
>>>>> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
>>> 174 | struct page *dst_page = sg_page(req->dst);
>>> |
>>>
>>> The issue was originally reported on LoongArch by kernel test
>>> robot (Huacai fixed it on LoongArch), then reported by Guenter
>>> and me on xtensa.
>>>
>>> This patch also removes lots of redundant macros which have
>>> been defined by asm-generic/cacheflush.h.
>>>
>>> Cc: Huacai Chen <[email protected]>
>>> Cc: Herbert Xu <[email protected]>
>>> Reported-by: kernel test robot <[email protected]>
>>> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>>> Reported-by: Barry Song <[email protected]>
>>> Closes: https://lore.kernel.org/all/CAGsJ_4yDk1+axbte7FKQEwD7X2oxUCFrEc9M5YOS1BobfDFXPA@mail.gmail.com/
>>> Reported-by: Guenter Roeck <[email protected]>
>>> Closes: https://lore.kernel.org/all/[email protected]/
>>> Fixes: 77292bb8ca69 ("crypto: scomp - remove memcpy if sg_nents is 1 and pages are lowmem")
>>> Signed-off-by: Barry Song <[email protected]>
>>
>> The mainline kernel still fails to build xtensa:allmodconfig.
>>
>> Building xtensa:allmodconfig ... failed
>> --------------
>> Error log:
>> crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
>> crypto/scompress.c:174:38: error: unused variable 'dst_page' [-Werror=unused-variable]
>> 174 | struct page *dst_page = sg_page(req->dst);
>>
>> This patch fixes the problem. Is there a chance to get it applied to the
>> upstream kernel, or should I just stop build testing xtensa:allmodconfig ?
>
> Applied to my xtensa tree.
> I was still hoping to see rationale for why this is a useful warning.
>
Useful in general or here ? In general it helps a lot to identify unnecessary
or buggy code, and I think it is very useful. In this case, the other option
would have been to declare the variable as __maybe_unused. I think that
has been discussed. Personally I preferred the more comprehensive patch
because it makes the code more generic, but at this point in the release
cycle I really only want to know what to do with xtensa:allmodconfig test
builds.
Thanks,
Guenter