* Re: [PATCH] soc: fsl: qe: remove unused function
From: Daniel Axtens @ 2021-04-16 6:57 UTC (permalink / raw)
To: Jiapeng Chong, qiang.zhao
Cc: Jiapeng Chong, linuxppc-dev, linux-kernel, linux-arm-kernel,
leoyang.li
In-Reply-To: <1618475663-100748-1-git-send-email-jiapeng.chong@linux.alibaba.com>
Hi Jiapeng,
> Fix the following clang warning:
>
> drivers/soc/fsl/qe/qe_ic.c:234:29: warning: unused function
> 'qe_ic_from_irq' [-Wunused-function].
>
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
> ---
> drivers/soc/fsl/qe/qe_ic.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/soc/fsl/qe/qe_ic.c b/drivers/soc/fsl/qe/qe_ic.c
> index 0390af9..b573712 100644
> --- a/drivers/soc/fsl/qe/qe_ic.c
> +++ b/drivers/soc/fsl/qe/qe_ic.c
> @@ -231,11 +231,6 @@ static inline void qe_ic_write(__be32 __iomem *base, unsigned int reg,
> qe_iowrite32be(value, base + (reg >> 2));
> }
>
> -static inline struct qe_ic *qe_ic_from_irq(unsigned int virq)
> -{
> - return irq_get_chip_data(virq);
> -}
This seems good to me.
* We know that this function can't be called directly from outside the
file, because it is static.
* The function address isn't used as a function pointer anywhere, so
that means it can't be called from outside the file that way (also
it's inline, which would make using a function pointer unwise!)
* There's no obvious macros in that file that might construct the name
of the function in a way that is hidden from grep.
All in all, I am fairly confident that the function is indeed not used.
Reviewed-by: Daniel Axtens <dja@axtens.net>
Kind regards,
Daniel
> -
> static inline struct qe_ic *qe_ic_from_irq_data(struct irq_data *d)
> {
> return irq_data_get_irq_chip_data(d);
> --
> 1.8.3.1
^ permalink raw reply
* Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
From: Christophe Leroy @ 2021-04-16 7:00 UTC (permalink / raw)
To: Daniel Axtens, Lakshmi Ramasubramanian, robh, dan.carpenter
Cc: devicetree, linuxppc-dev, kbuild-all, bauerman, lkp
In-Reply-To: <87eefag241.fsf@linkitivity.dja.id.au>
Le 16/04/2021 à 08:44, Daniel Axtens a écrit :
> Hi Lakshmi,
>
>> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
>>
>> Sorry - missed copying device-tree and powerpc mailing lists.
>>
>>> There are a few "goto out;" statements before the local variable "fdt"
>>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
>>> elf64_load(). This will result in an uninitialized "fdt" being passed
>>> to kvfree() in this function if there is an error before the call to
>>> of_kexec_alloc_and_setup_fdt().
>>>
>>> Initialize the local variable "fdt" to NULL.
>>>
> I'm a huge fan of initialising local variables! But I'm struggling to
> find the code path that will lead to an uninit fdt being returned...
>
> The out label reads in part:
>
> /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
> return ret ? ERR_PTR(ret) : fdt;
>
> As far as I can tell, any time we get a non-zero ret, we're going to
> return an error pointer rather than the uninitialised value...
I don't think GCC is smart enough to detect that.
>
> (btw, it does look like we might leak fdt if we have an error after we
> successfully kmalloc it.)
>
> Am I missing something? Can you link to the report for the kernel test
> robot or from Dan?
>
> FWIW, I think it's worth including this patch _anyway_ because initing
> local variables is good practice, but I'm just not sure on the
> justification.
I don't think local systematically initing local variables is a good practice at all, as it leads to
bugs where you get a wrong value because of pathes where you forgot to set the correct value.
If you don't init local variable at declaration and forget to set it in some pathes, the compiler
will detect it and warn you.
If you init the local variable with an arbitrary value at declaration and forget to set it later in
some pathes, the compiler won't be able to detect it and you will go with the wrong value.
Christophe
>
> Kind regards,
> Daniel
>
>>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> ---
>>> arch/powerpc/kexec/elf_64.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
>>> index 5a569bb51349..0051440c1f77 100644
>>> --- a/arch/powerpc/kexec/elf_64.c
>>> +++ b/arch/powerpc/kexec/elf_64.c
>>> @@ -32,7 +32,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>>> int ret;
>>> unsigned long kernel_load_addr;
>>> unsigned long initrd_load_addr = 0, fdt_load_addr;
>>> - void *fdt;
>>> + void *fdt = NULL;
>>> const void *slave_code;
>>> struct elfhdr ehdr;
>>> char *modified_cmdline = NULL;
>>>
>>
>> thanks,
>> -lakshmi
^ permalink raw reply
* 答复: [PATCH] symbol : Make the size of the compile-related array fixed
From: 韩大鹏(Han Dapeng) @ 2021-04-16 6:25 UTC (permalink / raw)
To: Christophe Leroy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, x86@kernel.org, H. Peter Anvin, Masahiro Yamada,
Michal Marek, Mike Rapoport, Pekka Enberg, Andrew Morton,
Arseny Solokha, Arvind Sankar, Kees Cook, Joerg Roedel,
Christian Brauner, Kirill Tkhai, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org, linux-s390@vger.kernel.org,
linux-kbuild@vger.kernel.org
Cc: 陈安庆(Anqing)
In-Reply-To: <2dfd1880-556e-1324-0b22-0907d24c7d5a@csgroup.eu>
The raw text is shown below:
====================================================================
From 540e6a6c36e6372d4f99eeb4a50c8eaa6d7989b3 Mon Sep 17 00:00:00 2001
From: Han Dapeng <handapeng@oppo.com>
Date: Fri, 16 Apr 2021 10:36:38 +0800
Subject: [PATCH] symbol : Make the size of the compile-related array fixed
For the same code, the machine's user name, hostname, or compilation time
may cause the kernel symbol address to be inconsistent, which is not
friendly to some symbol-dependent software, such as Crash.
Signed-off-by: Han Dapeng <handapeng@oppo.com>
---
arch/powerpc/mm/nohash/kaslr_booke.c | 2 +-
arch/s390/boot/version.c | 2 +-
arch/x86/boot/compressed/kaslr.c | 2 +-
arch/x86/boot/version.c | 2 +-
init/version.c | 4 ++--
scripts/mkcompile_h | 2 ++
6 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/mm/nohash/kaslr_booke.c b/arch/powerpc/mm/nohash/kaslr_booke.c
index 4c74e8a5482b..494ef408e60c 100644
--- a/arch/powerpc/mm/nohash/kaslr_booke.c
+++ b/arch/powerpc/mm/nohash/kaslr_booke.c
@@ -37,7 +37,7 @@ struct regions {
};
/* Simplified build-specific string for starting entropy. */
-static const char build_str[] = UTS_RELEASE " (" LINUX_COMPILE_BY "@"
+static const char build_str[COMPILE_STR_MAX] = UTS_RELEASE " (" LINUX_COMPILE_BY "@"
LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION;
struct regions __initdata regions;
diff --git a/arch/s390/boot/version.c b/arch/s390/boot/version.c
index d32e58bdda6a..627416a27d74 100644
--- a/arch/s390/boot/version.c
+++ b/arch/s390/boot/version.c
@@ -3,5 +3,5 @@
#include <generated/compile.h>
#include "boot.h"
-const char kernel_version[] = UTS_RELEASE
+const char kernel_version[COMPILE_STR_MAX] = UTS_RELEASE
" (" LINUX_COMPILE_BY "@" LINUX_COMPILE_HOST ") " UTS_VERSION;
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index b92fffbe761f..7b72b518a4c8 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -43,7 +43,7 @@
extern unsigned long get_cmd_line_ptr(void);
/* Simplified build-specific string for starting entropy. */
-static const char build_str[] = UTS_RELEASE " (" LINUX_COMPILE_BY "@"
+static const char build_str[COMPILE_STR_MAX] = UTS_RELEASE " (" LINUX_COMPILE_BY "@"
LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION;
static unsigned long rotate_xor(unsigned long hash, const void *area,
diff --git a/arch/x86/boot/version.c b/arch/x86/boot/version.c
index a1aaaf6c06a6..08feaa2d7a10 100644
--- a/arch/x86/boot/version.c
+++ b/arch/x86/boot/version.c
@@ -14,6 +14,6 @@
#include <generated/utsrelease.h>
#include <generated/compile.h>
-const char kernel_version[] =
+const char kernel_version[COMPILE_STR_MAX] =
UTS_RELEASE " (" LINUX_COMPILE_BY "@" LINUX_COMPILE_HOST ") "
UTS_VERSION;
diff --git a/init/version.c b/init/version.c
index 92afc782b043..adfc9e91b56b 100644
--- a/init/version.c
+++ b/init/version.c
@@ -35,11 +35,11 @@ struct uts_namespace init_uts_ns = {
EXPORT_SYMBOL_GPL(init_uts_ns);
/* FIXED STRINGS! Don't touch! */
-const char linux_banner[] =
+const char linux_banner[COMPILE_STR_MAX] =
"Linux version " UTS_RELEASE " (" LINUX_COMPILE_BY "@"
LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION "\n";
-const char linux_proc_banner[] =
+const char linux_proc_banner[COMPILE_STR_MAX] =
"%s version %s"
" (" LINUX_COMPILE_BY "@" LINUX_COMPILE_HOST ")"
" (" LINUX_COMPILER ") %s\n";
diff --git a/scripts/mkcompile_h b/scripts/mkcompile_h
index 4ae735039daf..02b9d9d54da9 100755
--- a/scripts/mkcompile_h
+++ b/scripts/mkcompile_h
@@ -65,6 +65,8 @@ UTS_VERSION="$(echo $UTS_VERSION $CONFIG_FLAGS $TIMESTAMP | cut -b -$UTS_LEN)"
LD_VERSION=$($LD -v | head -n1 | sed 's/(compatible with [^)]*)//' \
| sed 's/[[:space:]]*$//')
printf '#define LINUX_COMPILER "%s"\n' "$CC_VERSION, $LD_VERSION"
+
+ echo \#define COMPILE_STR_MAX 512
} > .tmpcompile
# Only replace the real compile.h if the new one is different,
--
2.27.0
=====================================================================================
This statement is automatically attached to our company's mail client.
Our company encourages giving code back to the community, no problem, I will take responsibility for this.
Thank you!
-----邮件原件-----
发件人: Christophe Leroy <christophe.leroy@csgroup.eu>
发送时间: 2021年4月16日 14:13
收件人: 韩大鹏(Han Dapeng) <handapeng@oppo.com>; Michael Ellerman <mpe@ellerman.id.au>; Benjamin Herrenschmidt <benh@kernel.crashing.org>; Paul Mackerras <paulus@samba.org>; Heiko Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian Borntraeger <borntraeger@de.ibm.com>; Thomas Gleixner <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; Borislav Petkov <bp@alien8.de>; x86@kernel.org; H. Peter Anvin <hpa@zytor.com>; Masahiro Yamada <masahiroy@kernel.org>; Michal Marek <michal.lkml@markovi.net>; Mike Rapoport <rppt@kernel.org>; Pekka Enberg <penberg@kernel.org>; Andrew Morton <akpm@linux-foundation.org>; Arseny Solokha <asolokha@kb.kras.ru>; Arvind Sankar <nivedita@alum.mit.edu>; Kees Cook <keescook@chromium.org>; Joerg Roedel <jroedel@suse.de>; Christian Brauner <christian.brauner@ubuntu.com>; Kirill Tkhai <ktkhai@virtuozzo.com>; linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; linux-s390@vger.kernel.org; linux-kbuild@vger.kernel.org
抄送: 陈安庆(Anqing) <chenanqing@oppo.com>
主题: Re: [PATCH] symbol : Make the size of the compile-related array fixed
Also, the following statement which appears at the end of your mail is puzzling. What can we do with your patch if there are such limitations ?
This e-mail and its attachments contain confidential information from OPPO, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or
dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!
Le 16/04/2021 à 08:08, Christophe Leroy a écrit :
> Hi,
>
> This mail is unreadable.
>
> Please send your patch as raw text mail, not as attached file.
>
> Thanks
> Christophe
>
> Le 16/04/2021 à 05:12, 韩大鹏(Han Dapeng) a écrit :
>> ---------------------------------------------------------------------
>> -------------------------------
>> *OPPO*
>> *
>> *
>> 本电子邮件及其附件含有OPPO公司的保密信息，仅限于邮件指明的收件人使用（包含个人及群组）。禁止任何人
>> 在 未经授权的情况下以任何形式使用。如果您错收了本邮件，请立即以电子邮件通知发件人并删除本邮件及其
>> 附件。
>>
>> This e-mail and its attachments contain confidential information from
>> OPPO, which is intended only for the person or entity whose address
>> is listed above. Any use of the information contained herein in any
>> way (including, but not limited to, total or partial disclosure,
>> reproduction, or
>> dissemination) by persons other than the intended recipient(s) is
>> prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!
>>
^ permalink raw reply related
* 答复: [PATCH] symbol : Make the size of the compile-related array fixed
From: 韩大鹏(Han Dapeng) @ 2021-04-16 6:34 UTC (permalink / raw)
To: Daniel Axtens, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, x86@kernel.org, H. Peter Anvin, Masahiro Yamada,
Michal Marek, Mike Rapoport, Pekka Enberg, Andrew Morton,
Arseny Solokha, Arvind Sankar, Kees Cook, Joerg Roedel,
Christian Brauner, Kirill Tkhai, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org, linux-s390@vger.kernel.org,
linux-kbuild@vger.kernel.org
Cc: 陈安庆(Anqing)
In-Reply-To: <87h7k6g39v.fsf@linkitivity.dja.id.au>
Thank you!Got it.
I tried to use Git, but it didn't work.
I'll do it next time.
-----邮件原件-----
发件人: Daniel Axtens <dja@axtens.net>
发送时间: 2021年4月16日 14:19
收件人: 韩大鹏(Han Dapeng) <handapeng@oppo.com>; Michael Ellerman <mpe@ellerman.id.au>; Benjamin Herrenschmidt <benh@kernel.crashing.org>; Paul Mackerras <paulus@samba.org>; Heiko Carstens <hca@linux.ibm.com>; Vasily Gorbik <gor@linux.ibm.com>; Christian Borntraeger <borntraeger@de.ibm.com>; Thomas Gleixner <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; Borislav Petkov <bp@alien8.de>; x86@kernel.org; H. Peter Anvin <hpa@zytor.com>; Masahiro Yamada <masahiroy@kernel.org>; Michal Marek <michal.lkml@markovi.net>; Mike Rapoport <rppt@kernel.org>; Pekka Enberg <penberg@kernel.org>; Andrew Morton <akpm@linux-foundation.org>; Arseny Solokha <asolokha@kb.kras.ru>; 韩大鹏(Han Dapeng) <handapeng@oppo.com>; Arvind Sankar <nivedita@alum.mit.edu>; Kees Cook <keescook@chromium.org>; Joerg Roedel <jroedel@suse.de>; Christian Brauner <christian.brauner@ubuntu.com>; Kirill Tkhai <ktkhai@virtuozzo.com>; linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; linux-s390@vger.kernel.org; linux-kbuild@vger.kernel.org
抄送: 陈安庆(Anqing) <chenanqing@oppo.com>
主题: Re: [PATCH] symbol : Make the size of the compile-related array fixed
Hi,
Thanks for your contribution to the kernel!
I notice that your patch is sumbitted as an attachment. In future, please could you submit your patch inline, rather than as an attachment?
See https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.kernel.org%2Fdoc%2Fhtml%2Fv4.15%2Fprocess%2F5.Posting.html&data=04%7C01%7Chandapeng%40oppo.com%7Cd00adeb268d34207f5bb08d9009f98b2%7Cf1905eb1c35341c5951662b4a54b5ee6%7C0%7C0%7C637541508341234090%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=8Sncu0Yyqvv3AtEZiL73vexVEpwmxmwrUjJ124ez%2BPs%3D&reserved=0
I'd recommend you use git send-email if possible: see e.g.
https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.kernel.org%2Fdoc%2Fhtml%2Fv4.15%2Fprocess%2Femail-clients.html&data=04%7C01%7Chandapeng%40oppo.com%7Cd00adeb268d34207f5bb08d9009f98b2%7Cf1905eb1c35341c5951662b4a54b5ee6%7C0%7C0%7C637541508341234090%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Ny2BFB4AE05nHHqITJkixtOwfPrB8s1KboWVLTFVXF8%3D&reserved=0
> Subject: [PATCH] symbol : Make the size of the compile-related array
> fixed
>
> For the same code, the machine's user name, hostname, or compilation
> time may cause the kernel symbol address to be inconsistent, which is
> not friendly to some symbol-dependent software, such as Crash.
If I understand correctly, this patch makes it easier to recompile the kernel from the same source but at a different time or on a different machine or with a different user, but still get the same symbols.
Is that right?
I wonder if there are other reproducible build techniques that might be simpler to apply? There is a kernel documentation page at
https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.kernel.org%2Fdoc%2Fhtml%2Flatest%2Fkbuild%2Freproducible-builds.html&data=04%7C01%7Chandapeng%40oppo.com%7Cd00adeb268d34207f5bb08d9009f98b2%7Cf1905eb1c35341c5951662b4a54b5ee6%7C0%7C0%7C637541508341234090%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=s54GLV2Pue3ArPgX4OcEyhAGsanJyBy1LRESkVMOGts%3D&reserved=0
which gives exisiting techniques to override the date, user and host.
Would they be sufficient to address your use case?
Kind regards,
Daniel
>
> Signed-off-by: Han Dapeng <handapeng@oppo.com>
> ---
> arch/powerpc/mm/nohash/kaslr_booke.c | 2 +-
> arch/s390/boot/version.c | 2 +-
> arch/x86/boot/compressed/kaslr.c | 2 +-
> arch/x86/boot/version.c | 2 +-
> init/version.c | 4 ++--
> scripts/mkcompile_h | 2 ++
> 6 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/mm/nohash/kaslr_booke.c
> b/arch/powerpc/mm/nohash/kaslr_booke.c
> index 4c74e8a5482b..494ef408e60c 100644
> --- a/arch/powerpc/mm/nohash/kaslr_booke.c
> +++ b/arch/powerpc/mm/nohash/kaslr_booke.c
> @@ -37,7 +37,7 @@ struct regions {
> };
>
> /* Simplified build-specific string for starting entropy. */ -static
> const char build_str[] = UTS_RELEASE " (" LINUX_COMPILE_BY "@"
> +static const char build_str[COMPILE_STR_MAX] = UTS_RELEASE " (" LINUX_COMPILE_BY "@"
> LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION;
>
> struct regions __initdata regions;
> diff --git a/arch/s390/boot/version.c b/arch/s390/boot/version.c index
> d32e58bdda6a..627416a27d74 100644
> --- a/arch/s390/boot/version.c
> +++ b/arch/s390/boot/version.c
> @@ -3,5 +3,5 @@
> #include <generated/compile.h>
> #include "boot.h"
>
> -const char kernel_version[] = UTS_RELEASE
> +const char kernel_version[COMPILE_STR_MAX] = UTS_RELEASE
> " (" LINUX_COMPILE_BY "@" LINUX_COMPILE_HOST ") " UTS_VERSION; diff
> --git a/arch/x86/boot/compressed/kaslr.c
> b/arch/x86/boot/compressed/kaslr.c
> index b92fffbe761f..7b72b518a4c8 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -43,7 +43,7 @@
> extern unsigned long get_cmd_line_ptr(void);
>
> /* Simplified build-specific string for starting entropy. */ -static
> const char build_str[] = UTS_RELEASE " (" LINUX_COMPILE_BY "@"
> +static const char build_str[COMPILE_STR_MAX] = UTS_RELEASE " (" LINUX_COMPILE_BY "@"
> LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION;
>
> static unsigned long rotate_xor(unsigned long hash, const void *area,
> diff --git a/arch/x86/boot/version.c b/arch/x86/boot/version.c index
> a1aaaf6c06a6..08feaa2d7a10 100644
> --- a/arch/x86/boot/version.c
> +++ b/arch/x86/boot/version.c
> @@ -14,6 +14,6 @@
> #include <generated/utsrelease.h>
> #include <generated/compile.h>
>
> -const char kernel_version[] =
> +const char kernel_version[COMPILE_STR_MAX] =
> UTS_RELEASE " (" LINUX_COMPILE_BY "@" LINUX_COMPILE_HOST ") "
> UTS_VERSION;
> diff --git a/init/version.c b/init/version.c index
> 92afc782b043..adfc9e91b56b 100644
> --- a/init/version.c
> +++ b/init/version.c
> @@ -35,11 +35,11 @@ struct uts_namespace init_uts_ns = {
> EXPORT_SYMBOL_GPL(init_uts_ns);
>
> /* FIXED STRINGS! Don't touch! */
> -const char linux_banner[] =
> +const char linux_banner[COMPILE_STR_MAX] =
> "Linux version " UTS_RELEASE " (" LINUX_COMPILE_BY "@"
> LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION "\n";
>
> -const char linux_proc_banner[] =
> +const char linux_proc_banner[COMPILE_STR_MAX] =
> "%s version %s"
> " (" LINUX_COMPILE_BY "@" LINUX_COMPILE_HOST ")"
> " (" LINUX_COMPILER ") %s\n";
> diff --git a/scripts/mkcompile_h b/scripts/mkcompile_h index
> 4ae735039daf..02b9d9d54da9 100755
> --- a/scripts/mkcompile_h
> +++ b/scripts/mkcompile_h
> @@ -65,6 +65,8 @@ UTS_VERSION="$(echo $UTS_VERSION $CONFIG_FLAGS $TIMESTAMP | cut -b -$UTS_LEN)"
> LD_VERSION=$($LD -v | head -n1 | sed 's/(compatible with [^)]*)//' \
> | sed 's/[[:space:]]*$//')
> printf '#define LINUX_COMPILER "%s"\n' "$CC_VERSION, $LD_VERSION"
> +
> + echo \#define COMPILE_STR_MAX 512
> } > .tmpcompile
>
> # Only replace the real compile.h if the new one is different,
> --
> 2.27.0
________________________________
OPPO
本电子邮件及其附件含有OPPO公司的保密信息，仅限于邮件指明的收件人使用（包含个人及群组）。禁止任何人在未经授权的情况下以任何形式使用。如果您错收了本邮件，请立即以电子邮件通知发件人并删除本邮件及其附件。
This e-mail and its attachments contain confidential information from OPPO, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!
^ permalink raw reply
* Re: [PATCH] soc: fsl: qe: remove unused function
From: Christophe Leroy @ 2021-04-16 7:06 UTC (permalink / raw)
To: Daniel Axtens, Jiapeng Chong, qiang.zhao
Cc: linuxppc-dev, linux-kernel, linux-arm-kernel, leoyang.li
In-Reply-To: <87blaeg1ig.fsf@linkitivity.dja.id.au>
Le 16/04/2021 à 08:57, Daniel Axtens a écrit :
> Hi Jiapeng,
>
>> Fix the following clang warning:
You are not fixing a warning, you are removing a function in order to fix a warning ...
>>
>> drivers/soc/fsl/qe/qe_ic.c:234:29: warning: unused function
>> 'qe_ic_from_irq' [-Wunused-function].
Would be wise to tell that the last users of the function where removed by commit d7c2878cfcfa
("soc: fsl: qe: remove unused qe_ic_set_* functions")
https://github.com/torvalds/linux/commit/d7c2878cfcfa
>>
>> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
>> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
>> ---
>> drivers/soc/fsl/qe/qe_ic.c | 5 -----
>> 1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/soc/fsl/qe/qe_ic.c b/drivers/soc/fsl/qe/qe_ic.c
>> index 0390af9..b573712 100644
>> --- a/drivers/soc/fsl/qe/qe_ic.c
>> +++ b/drivers/soc/fsl/qe/qe_ic.c
>> @@ -231,11 +231,6 @@ static inline void qe_ic_write(__be32 __iomem *base, unsigned int reg,
>> qe_iowrite32be(value, base + (reg >> 2));
>> }
>>
>> -static inline struct qe_ic *qe_ic_from_irq(unsigned int virq)
>> -{
>> - return irq_get_chip_data(virq);
>> -}
>
> This seems good to me.
>
> * We know that this function can't be called directly from outside the
> file, because it is static.
>
> * The function address isn't used as a function pointer anywhere, so
> that means it can't be called from outside the file that way (also
> it's inline, which would make using a function pointer unwise!)
>
> * There's no obvious macros in that file that might construct the name
> of the function in a way that is hidden from grep.
>
> All in all, I am fairly confident that the function is indeed not used.
>
> Reviewed-by: Daniel Axtens <dja@axtens.net>
>
> Kind regards,
> Daniel
>
>> -
>> static inline struct qe_ic *qe_ic_from_irq_data(struct irq_data *d)
>> {
>> return irq_data_get_irq_chip_data(d);
>> --
>> 1.8.3.1
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: extract host bridge from pci_bus prior to bus removal
From: Daniel Axtens @ 2021-04-16 7:15 UTC (permalink / raw)
To: Tyrel Datwyler, mpe; +Cc: Tyrel Datwyler, linuxppc-dev, linux-kernel
In-Reply-To: <20210211182435.47968-1-tyreld@linux.ibm.com>
Hi Tyrel,
> The pci_bus->bridge reference may no longer be valid after
> pci_bus_remove() resulting in passing a bad value to device_unregister()
> for the associated bridge device.
>
> Store the host_bridge reference in a separate variable prior to
> pci_bus_remove().
>
The patch certainly seems to do what you say. I'm not really up on the
innards of PCI, so I'm struggling to figure out by what code path
pci_bus_remove() might invalidate pci_bus->bridge? A quick look at
pci_remove_bus was not very illuminating but I didn't chase down every
call it made.
Kind regards,
Daniel
> Fixes: 7340056567e3 ("powerpc/pci: Reorder pci bus/bridge unregistration during PHB removal")
> Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
> ---
> arch/powerpc/platforms/pseries/pci_dlpar.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c
> index f9ae17e8a0f4..a8f9140a24fa 100644
> --- a/arch/powerpc/platforms/pseries/pci_dlpar.c
> +++ b/arch/powerpc/platforms/pseries/pci_dlpar.c
> @@ -50,6 +50,7 @@ EXPORT_SYMBOL_GPL(init_phb_dynamic);
> int remove_phb_dynamic(struct pci_controller *phb)
> {
> struct pci_bus *b = phb->bus;
> + struct pci_host_bridge *host_bridge = to_pci_host_bridge(b->bridge);
> struct resource *res;
> int rc, i;
>
> @@ -76,7 +77,8 @@ int remove_phb_dynamic(struct pci_controller *phb)
> /* Remove the PCI bus and unregister the bridge device from sysfs */
> phb->bus = NULL;
> pci_remove_bus(b);
> - device_unregister(b->bridge);
> + host_bridge->bus = NULL;
> + device_unregister(&host_bridge->dev);
>
> /* Now release the IO resource */
> if (res->flags & IORESOURCE_IO)
> --
> 2.27.0
^ permalink raw reply
* RE: [PATCH 1/1] mm: Fix struct page layout on 32-bit systems
From: David Laight @ 2021-04-16 7:32 UTC (permalink / raw)
To: 'Matthew Wilcox'
Cc: Arnd Bergmann, Grygorii Strashko, netdev@vger.kernel.org,
Ilias Apalodimas, linux-kernel@vger.kernel.org,
linux-mips@vger.kernel.org, linux-mm@kvack.org,
Jesper Dangaard Brouer, Matteo Croce,
linuxppc-dev@lists.ozlabs.org, Christoph Hellwig,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20210415222211.GG2531743@casper.infradead.org>
From: Matthew Wilcox <willy@infradead.org>
> Sent: 15 April 2021 23:22
>
> On Thu, Apr 15, 2021 at 09:11:56PM +0000, David Laight wrote:
> > Isn't it possible to move the field down one long?
> > This might require an explicit zero - but this is not a common
> > code path - the extra write will be noise.
>
> Then it overlaps page->mapping. See emails passim.
The rules on overlaps make be wonder if every 'long'
should be in its own union.
The comments would need to say when each field is used.
It would, at least, make these errors less common.
That doesn't solve the 64bit dma_addr though.
Actually rather that word-swapping dma_addr on 32bit BE
could you swap over the two fields it overlays with.
That might look messy in the .h, but it doesn't require
an accessor function to do the swap - easily missed.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
From: Dan Carpenter @ 2021-04-16 7:40 UTC (permalink / raw)
To: Daniel Axtens
Cc: devicetree, kbuild-all, lkp, robh, Lakshmi Ramasubramanian,
linuxppc-dev, bauerman
In-Reply-To: <87eefag241.fsf@linkitivity.dja.id.au>
On Fri, Apr 16, 2021 at 04:44:30PM +1000, Daniel Axtens wrote:
> Hi Lakshmi,
>
> > On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
> >
> > Sorry - missed copying device-tree and powerpc mailing lists.
> >
> >> There are a few "goto out;" statements before the local variable "fdt"
> >> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
> >> elf64_load(). This will result in an uninitialized "fdt" being passed
> >> to kvfree() in this function if there is an error before the call to
> >> of_kexec_alloc_and_setup_fdt().
> >>
> >> Initialize the local variable "fdt" to NULL.
> >>
> I'm a huge fan of initialising local variables!
Don't be! It just disables static checker warnings and hides bugs.
The kbuild emails are archived but the email is mangled and unreadable.
https://www.mail-archive.com/kbuild@lists.01.org/msg06371.html
I think maybe you're not on the most recent code. In linux-next this
code looks like:
arch/powerpc/kexec/elf_64.c
27 static void *elf64_load(struct kimage *image, char *kernel_buf,
28 unsigned long kernel_len, char *initrd,
29 unsigned long initrd_len, char *cmdline,
30 unsigned long cmdline_len)
31 {
32 int ret;
33 unsigned long kernel_load_addr;
34 unsigned long initrd_load_addr = 0, fdt_load_addr;
35 void *fdt;
36 const void *slave_code;
37 struct elfhdr ehdr;
38 char *modified_cmdline = NULL;
39 struct kexec_elf_info elf_info;
40 struct kexec_buf kbuf = { .image = image, .buf_min = 0,
41 .buf_max = ppc64_rma_size };
42 struct kexec_buf pbuf = { .image = image, .buf_min = 0,
43 .buf_max = ppc64_rma_size, .top_down = true,
44 .mem = KEXEC_BUF_MEM_UNKNOWN };
45
46 ret = kexec_build_elf_info(kernel_buf, kernel_len, &ehdr, &elf_info);
47 if (ret)
48 goto out;
^^^^^^^^
I really despise "goto out;" because freeing things which haven't been
allocated is always dangerous.
[ snip ].
143 out:
144 kfree(modified_cmdline);
145 kexec_free_elf_info(&elf_info);
^^^^^^^^
There is a possibility that "elf_info" has holds uninitialized stack
data if elf_read_ehdr() fails so that's probably fixing as well. kexec()
is root only so this can't be exploited.
146
147 /*
148 * Once FDT buffer has been successfully passed to kexec_add_buffer(),
149 * the FDT buffer address is saved in image->arch.fdt. In that case,
150 * the memory cannot be freed here in case of any other error.
151 */
152 if (ret && !image->arch.fdt)
153 kvfree(fdt);
^^^
Uninitialized.
154
155 return ret ? ERR_PTR(ret) : NULL;
156 }
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/sstep: Add emulation support for ‘setb’ instruction
From: Daniel Axtens @ 2021-04-16 7:44 UTC (permalink / raw)
To: Sathvika Vasireddy, linuxppc-dev; +Cc: naveen.n.rao, Sathvika Vasireddy
In-Reply-To: <767e53c4c27da024ca277e21ffcd0cff131f5c73.1618469454.git.sathvika@linux.vnet.ibm.com>
Sathvika Vasireddy <sathvika@linux.vnet.ibm.com> writes:
> This adds emulation support for the following instruction:
> * Set Boolean (setb)
>
> Signed-off-by: Sathvika Vasireddy <sathvika@linux.vnet.ibm.com>
> ---
> arch/powerpc/lib/sstep.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index c6aebc149d14..263c613d7490 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -1964,6 +1964,18 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> op->val = ~(regs->gpr[rd] | regs->gpr[rb]);
> goto logical_done;
>
> + case 128: /* setb */
> + if (!cpu_has_feature(CPU_FTR_ARCH_300))
> + goto unknown_opcode;
Ok, if I've understood correctly...
> + ra = ra & ~0x3;
This masks off the bits of RA that are not part of BTF:
ra is in [0, 31] which is [0b00000, 0b11111]
Then ~0x3 = ~0b00011
ra = ra & 0b11100
This gives us then,
ra = btf << 2; or
btf = ra >> 2;
Let's then check to see if your calculations read the right fields.
> + if ((regs->ccr) & (1 << (31 - ra)))
> + op->val = -1;
> + else if ((regs->ccr) & (1 << (30 - ra)))
> + op->val = 1;
> + else
> + op->val = 0;
CR field: 7 6 5 4 3 2 1 0
bit: 0123 0123 0123 0123 0123 0123 0123 0123
normal bit #: 0.....................................31
ibm bit #: 31.....................................0
If btf = 0, ra = 0, check normal bits 31 and 30, which are both in CR0.
CR field: 7 6 5 4 3 2 1 0
bit: 0123 0123 0123 0123 0123 0123 0123 0123
^^
If btf = 7, ra = 0b11100 = 28, so check normal bits 31-28 and 30-28,
which are 3 and 2.
CR field: 7 6 5 4 3 2 1 0
bit: 0123 0123 0123 0123 0123 0123 0123 0123
^^
If btf = 3, ra = 0b01100 = 12, for normal bits 19 and 18:
CR field: 7 6 5 4 3 2 1 0
bit: 0123 0123 0123 0123 0123 0123 0123 0123
^^
So yes, your calculations, while I struggle to follow _how_ they work,
do in fact seem to work.
Checkpatch does have one complaint:
CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'regs->ccr'
#30: FILE: arch/powerpc/lib/sstep.c:1971:
+ if ((regs->ccr) & (1 << (31 - ra)))
I don't really mind the parenteses: I think you are safe to ignore
checkpatch here unless someone else complains :)
If you do end up respinning the patch, I think it would be good to make
the maths a bit clearer. I think it works because a left shift of 2 is
the same as multiplying by 4, but it would be easier to follow if you
used a temporary variable for btf.
However, I do think this is still worth adding to the kernel either way,
so:
Reviewed-by: Daniel Axtens <dja@axtens.net>
Kind regards,
Daniel
> + goto compute_done;
> +
> case 154: /* prtyw */
> do_prty(regs, op, regs->gpr[rd], 32);
> goto logical_done_nocc;
> --
> 2.16.4
^ permalink raw reply
* Re: [PATCH v1 1/1] powerpc/papr_scm: Properly handle UUID types and API
From: Aneesh Kumar K.V @ 2021-04-16 7:58 UTC (permalink / raw)
To: Andy Shevchenko, Vaibhav Jain, linuxppc-dev, linux-kernel
Cc: Oliver O'Halloran, Paul Mackerras
In-Reply-To: <20210415134637.17770-1-andriy.shevchenko@linux.intel.com>
On 4/15/21 7:16 PM, Andy Shevchenko wrote:
> Parse to and export from UUID own type, before dereferencing.
> This also fixes wrong comment (Little Endian UUID is something else)
> and should fix Sparse warnings about assigning strict types to POD.
>
> Fixes: 43001c52b603 ("powerpc/papr_scm: Use ibm,unit-guid as the iset cookie")
> Fixes: 259a948c4ba1 ("powerpc/pseries/scm: Use a specific endian format for storing uuid from the device tree")
> Cc: Oliver O'Halloran <oohall@gmail.com>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> Not tested
> arch/powerpc/platforms/pseries/papr_scm.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index ae6f5d80d5ce..4366e1902890 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -1085,8 +1085,9 @@ static int papr_scm_probe(struct platform_device *pdev)
> u32 drc_index, metadata_size;
> u64 blocks, block_size;
> struct papr_scm_priv *p;
> + u8 uuid_raw[UUID_SIZE];
> const char *uuid_str;
> - u64 uuid[2];
> + uuid_t uuid;
> int rc;
>
> /* check we have all the required DT properties */
> @@ -1129,16 +1130,18 @@ static int papr_scm_probe(struct platform_device *pdev)
> p->hcall_flush_required = of_property_read_bool(dn, "ibm,hcall-flush-required");
>
> /* We just need to ensure that set cookies are unique across */
> - uuid_parse(uuid_str, (uuid_t *) uuid);
> + uuid_parse(uuid_str, &uuid);
> +
> /*
> * cookie1 and cookie2 are not really little endian
> - * we store a little endian representation of the
> + * we store a raw buffer representation of the
> * uuid str so that we can compare this with the label
> * area cookie irrespective of the endian config with which
> * the kernel is built.
> */
> - p->nd_set.cookie1 = cpu_to_le64(uuid[0]);
> - p->nd_set.cookie2 = cpu_to_le64(uuid[1]);
> + export_uuid(uuid_raw, &uuid);
> + p->nd_set.cookie1 = get_unaligned_le64(&uuid_raw[0]);
> + p->nd_set.cookie2 = get_unaligned_le64(&uuid_raw[8]);
>
ok that does the equivalent of cpu_to_le64 there. So we are good. But
the comment update is missing the details why we did that
get_unaligned_le64. Maybe raw buffer representation is the correct term?
Should we add an example in the comment. ie,
/*
* Historically we stored the cookie in the below format.
for a uuid str 72511b67-0b3b-42fd-8d1d-5be3cae8bcaa
cookie1 was 0xfd423b0b671b5172 cookie2 was 0xaabce8cae35b1d8d
*/
> /* might be zero */
> p->metadata_size = metadata_size;
>
^ permalink raw reply
* Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
From: Dan Carpenter @ 2021-04-16 8:09 UTC (permalink / raw)
To: Christophe Leroy
Cc: devicetree, kbuild-all, lkp, robh, Lakshmi Ramasubramanian,
linuxppc-dev, bauerman, Daniel Axtens
In-Reply-To: <f82a9fe2-3254-3f25-616c-10e56103bdc6@csgroup.eu>
On Fri, Apr 16, 2021 at 09:00:12AM +0200, Christophe Leroy wrote:
>
>
> Le 16/04/2021 à 08:44, Daniel Axtens a écrit :
> > Hi Lakshmi,
> >
> > > On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
> > >
> > > Sorry - missed copying device-tree and powerpc mailing lists.
> > >
> > > > There are a few "goto out;" statements before the local variable "fdt"
> > > > is initialized through the call to of_kexec_alloc_and_setup_fdt() in
> > > > elf64_load(). This will result in an uninitialized "fdt" being passed
> > > > to kvfree() in this function if there is an error before the call to
> > > > of_kexec_alloc_and_setup_fdt().
> > > >
> > > > Initialize the local variable "fdt" to NULL.
> > > >
> > I'm a huge fan of initialising local variables! But I'm struggling to
> > find the code path that will lead to an uninit fdt being returned...
> >
> > The out label reads in part:
> >
> > /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
> > return ret ? ERR_PTR(ret) : fdt;
> >
> > As far as I can tell, any time we get a non-zero ret, we're going to
> > return an error pointer rather than the uninitialised value...
>
> I don't think GCC is smart enough to detect that.
>
We disabled uninitialized variable checking for GCC.
But actually is something that has been on my mind recently. Smatch is
supposed to parse this correctly but there is a bug that affects powerpc
and I don't know how to debug it. The kbuild bot is doing cross
platform compiles but I don't have one set up on myself. Could someone
with Smatch installed test something for me?
Or if you don't have Smatch installed then you should definitely install
it. :P
https://www.spinics.net/lists/smatch/msg00568.html
Apply the patch from below and edit the path to point to the correct
directory. Then run kchecker and email me the output?
~/path/to/smatch_scripts/kchecker arch/powerpc/kernel/hw_breakpoint.c
regads,
dan carpenter
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 8fc7a14e4d71..f2dfba54e14d 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -167,13 +167,19 @@ static bool can_co_exist(struct breakpoint *b, struct perf_event *bp)
return !(alternate_infra_bp(b, bp) && bp_addr_range_overlap(b->bp, bp));
}
+#include "/home/XXX/path/to/smatch/check_debug.h"
static int task_bps_add(struct perf_event *bp)
{
struct breakpoint *tmp;
tmp = alloc_breakpoint(bp);
- if (IS_ERR(tmp))
+ __smatch_about(tmp);
+ __smatch_debug_on();
+ if (IS_ERR(tmp)) {
+ __smatch_debug_off();
+ __smatch_about(tmp);
return PTR_ERR(tmp);
+ }
list_add(&tmp->list, &task_bps);
return 0;
^ permalink raw reply related
* Re: [PATCH] mm: ptdump: Fix build failure
From: Steven Price @ 2021-04-16 8:42 UTC (permalink / raw)
To: Christophe Leroy, Andrew Morton; +Cc: linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <912b349e2bcaa88939904815ca0af945740c6bd4.1618478922.git.christophe.leroy@csgroup.eu>
On 15/04/2021 10:31, Christophe Leroy wrote:
> CC mm/ptdump.o
> In file included from <command-line>:
> mm/ptdump.c: In function 'ptdump_pte_entry':
> ././include/linux/compiler_types.h:320:38: error: call to '__compiletime_assert_207' declared with attribute error: Unsupported access size for {READ,WRITE}_ONCE().
> 320 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> | ^
> ././include/linux/compiler_types.h:301:4: note: in definition of macro '__compiletime_assert'
> 301 | prefix ## suffix(); \
> | ^~~~~~
> ././include/linux/compiler_types.h:320:2: note: in expansion of macro '_compiletime_assert'
> 320 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> | ^~~~~~~~~~~~~~~~~~~
> ./include/asm-generic/rwonce.h:36:2: note: in expansion of macro 'compiletime_assert'
> 36 | compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
> | ^~~~~~~~~~~~~~~~~~
> ./include/asm-generic/rwonce.h:49:2: note: in expansion of macro 'compiletime_assert_rwonce_type'
> 49 | compiletime_assert_rwonce_type(x); \
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> mm/ptdump.c:114:14: note: in expansion of macro 'READ_ONCE'
> 114 | pte_t val = READ_ONCE(*pte);
> | ^~~~~~~~~
> make[2]: *** [mm/ptdump.o] Error 1
>
> READ_ONCE() cannot be used for reading PTEs. Use ptep_get()
> instead. See commit 481e980a7c19 ("mm: Allow arches to provide ptep_get()")
> and commit c0e1c8c22beb ("powerpc/8xx: Provide ptep_get() with 16k pages")
> for details.
It was cargo-culted from the arm64/x86 implementations (where this
happens to be safe).
> Fixes: 30d621f6723b ("mm: add generic ptdump")
> Cc: Steven Price <steven.price@arm.com>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Reviewed-by: Steven Price <steven.price@arm.com>
Thanks,
Steve
> ---
> mm/ptdump.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/ptdump.c b/mm/ptdump.c
> index 4354c1422d57..da751448d0e4 100644
> --- a/mm/ptdump.c
> +++ b/mm/ptdump.c
> @@ -111,7 +111,7 @@ static int ptdump_pte_entry(pte_t *pte, unsigned long addr,
> unsigned long next, struct mm_walk *walk)
> {
> struct ptdump_state *st = walk->private;
> - pte_t val = READ_ONCE(*pte);
> + pte_t val = ptep_get(pte);
>
> if (st->effective_prot)
> st->effective_prot(st, 4, pte_val(val));
>
^ permalink raw reply
* Re: [PATCH net-next v4 2/2] of: net: fix of_get_mac_addr_nvmem() for non-platform devices
From: Michael Walle @ 2021-04-16 7:29 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Andrew Lunn, Paul Mackerras, Rafał Miłecki,
Nobuhiro Iwamatsu, linux-stm32, Jerome Brunet, Neil Armstrong,
Michal Simek, Jose Abreu, NXP Linux Team, Mark Lee, Vadym Kochan,
Sascha Hauer, Lorenzo Bianconi, linux-omap, Greg Kroah-Hartman,
linux-wireless, linux-kernel, Pengutronix Kernel Team,
Vladimir Oltean, Claudiu Beznea, Jérôme Pouiller,
Kunihiko Hayashi, Chris Snook, Frank Rowand, Gregory Clement,
Madalin Bucur, Martin Blumenstingl, Murali Karicheri,
Yisen Zhuang, Alexandre Torgue, Wingman Kwok, Sean Wang,
Maxime Ripard, Claudiu Manoil, linux-amlogic, Kalle Valo,
Mirko Lindner, Fugang Duan, Bryan Whitehead, ath9k-devel,
UNGLinuxDriver, Taras Chornyi, Maxime Coquelin, Kevin Hilman,
Heiner Kallweit, Andreas Larsson, Giuseppe Cavallaro,
Fabio Estevam, Stanislaw Gruszka, Florian Fainelli, linux-staging,
Chen-Yu Tsai, bcm-kernel-feedback-list, linux-arm-kernel,
Grygorii Strashko, Byungho An, Radhey Shyam Pandey,
Vladimir Zapolskiy, John Crispin, Salil Mehta, Sergei Shtylyov,
linux-oxnas, Shawn Guo, David S . Miller, Helmut Schaa,
Thomas Petazzoni, linux-renesas-soc, Ryder Lee, Russell King,
Hauke Mehrtens, Jakub Kicinski, Vivien Didelot, Sunil Goutham,
Sebastian Hesselbarth, devicetree, Rob Herring, linux-mediatek,
Matthias Brugger, Jernej Skrabec, netdev, Nicolas Ferre, Li Yang,
Stephen Hemminger, Vinod Koul, Joyce Ooi, linuxppc-dev,
Felix Fietkau
In-Reply-To: <730d603b12e590c56770309b4df2bd668f7afbe3.camel@kernel.crashing.org>
Am 2021-04-16 05:24, schrieb Benjamin Herrenschmidt:
> On Mon, 2021-04-12 at 19:47 +0200, Michael Walle wrote:
>>
>> /**
>> * of_get_phy_mode - Get phy mode for given device_node
>> @@ -59,15 +60,39 @@ static int of_get_mac_addr(struct device_node *np,
>> const char *name, u8 *addr)
>> static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
>> {
>> struct platform_device *pdev = of_find_device_by_node(np);
>> + struct nvmem_cell *cell;
>> + const void *mac;
>> + size_t len;
>> int ret;
>>
>> - if (!pdev)
>> - return -ENODEV;
>> + /* Try lookup by device first, there might be a
>> nvmem_cell_lookup
>> + * associated with a given device.
>> + */
>> + if (pdev) {
>> + ret = nvmem_get_mac_address(&pdev->dev, addr);
>> + put_device(&pdev->dev);
>> + return ret;
>> + }
>> +
>
> This smells like the wrong band aid :)
>
> Any struct device can contain an OF node pointer these days.
But not all nodes might have an associated device, see DSA for example.
And as the name suggests of_get_mac_address() operates on a node. So
if a driver calls of_get_mac_address() it should work on the node. What
is wrong IMHO, is that the ethernet drivers where the corresponding
board
has a nvmem_cell_lookup registered is calling of_get_mac_address(node).
It should rather call eth_get_mac_address(dev) in the first place.
One would need to figure out if there is an actual device (with an
assiciated of_node), then call eth_get_mac_address(dev) and if there
isn't a device call of_get_mac_address(node).
But I don't know if that is easy to figure out. Well, one could start
with just the device where nvmem_cell_lookup is used. Then we could
drop the workaround above.
> This seems all backwards. I think we are dealing with bad evolution.
>
> We need to do a lookup for the device because we get passed an of_node.
> We should just get passed a device here... or rather stop calling
> of_get_mac_addr() from all those drivers and instead call
> eth_platform_get_mac_address() which in turns calls of_get_mac_addr().
>
> Then the nvmem stuff gets put in eth_platform_get_mac_address().
>
> of_get_mac_addr() becomes a low-level thingy that most drivers don't
> care about.
The NVMEM thing is just another (optional) way how the MAC address
is fetched from the device tree. Thus, if the drivers have the
of_get_mac_address() call they should automatically get the NVMEM
method, too.
-michael
^ permalink raw reply
* Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
From: Michael Ellerman @ 2021-04-16 9:05 UTC (permalink / raw)
To: Daniel Axtens, Lakshmi Ramasubramanian, robh, dan.carpenter
Cc: devicetree, linuxppc-dev, kbuild-all, bauerman, lkp
In-Reply-To: <87eefag241.fsf@linkitivity.dja.id.au>
Daniel Axtens <dja@axtens.net> writes:
>> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
>>
>> Sorry - missed copying device-tree and powerpc mailing lists.
>>
>>> There are a few "goto out;" statements before the local variable "fdt"
>>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
>>> elf64_load(). This will result in an uninitialized "fdt" being passed
>>> to kvfree() in this function if there is an error before the call to
>>> of_kexec_alloc_and_setup_fdt().
>>>
>>> Initialize the local variable "fdt" to NULL.
>>>
> I'm a huge fan of initialising local variables! But I'm struggling to
> find the code path that will lead to an uninit fdt being returned...
>
> The out label reads in part:
>
> /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
> return ret ? ERR_PTR(ret) : fdt;
>
> As far as I can tell, any time we get a non-zero ret, we're going to
> return an error pointer rather than the uninitialised value...
>
> (btw, it does look like we might leak fdt if we have an error after we
> successfully kmalloc it.)
>
> Am I missing something? Can you link to the report for the kernel test
> robot or from Dan?
>
> FWIW, I think it's worth including this patch _anyway_ because initing
> local variables is good practice, but I'm just not sure on the
> justification.
Why is it good practice?
It defeats -Wuninitialized. So you're guaranteed to be returning
something initialised, but not necessarily initialised to the right
value.
In a case like this NULL seems like a safe choice, but it's still wrong.
The function is meant to return a pointer to the successfully allocated
fdt, or an ERR_PTR() value. NULL is neither of those.
I agree there are security reasons that initialising stack variables is
desirable, but I think that should be handled by the compiler, not at
the source level.
cheers
^ permalink raw reply
* Re: [PATCH v1 1/1] powerpc/papr_scm: Properly handle UUID types and API
From: Andy Shevchenko @ 2021-04-16 9:09 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Oliver O'Halloran, linux-kernel, Paul Mackerras, Vaibhav Jain,
linuxppc-dev
In-Reply-To: <af677216-82b4-f1fa-1d90-3d32dabf8583@linux.ibm.com>
On Fri, Apr 16, 2021 at 01:28:21PM +0530, Aneesh Kumar K.V wrote:
> On 4/15/21 7:16 PM, Andy Shevchenko wrote:
> > Parse to and export from UUID own type, before dereferencing.
> > This also fixes wrong comment (Little Endian UUID is something else)
> > and should fix Sparse warnings about assigning strict types to POD.
> >
> > Fixes: 43001c52b603 ("powerpc/papr_scm: Use ibm,unit-guid as the iset cookie")
> > Fixes: 259a948c4ba1 ("powerpc/pseries/scm: Use a specific endian format for storing uuid from the device tree")
> > Cc: Oliver O'Halloran <oohall@gmail.com>
> > Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > Not tested
> > arch/powerpc/platforms/pseries/papr_scm.c | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> > index ae6f5d80d5ce..4366e1902890 100644
> > --- a/arch/powerpc/platforms/pseries/papr_scm.c
> > +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> > @@ -1085,8 +1085,9 @@ static int papr_scm_probe(struct platform_device *pdev)
> > u32 drc_index, metadata_size;
> > u64 blocks, block_size;
> > struct papr_scm_priv *p;
> > + u8 uuid_raw[UUID_SIZE];
> > const char *uuid_str;
> > - u64 uuid[2];
> > + uuid_t uuid;
> > int rc;
> > /* check we have all the required DT properties */
> > @@ -1129,16 +1130,18 @@ static int papr_scm_probe(struct platform_device *pdev)
> > p->hcall_flush_required = of_property_read_bool(dn, "ibm,hcall-flush-required");
> > /* We just need to ensure that set cookies are unique across */
> > - uuid_parse(uuid_str, (uuid_t *) uuid);
> > + uuid_parse(uuid_str, &uuid);
> > +
> > /*
> > * cookie1 and cookie2 are not really little endian
> > - * we store a little endian representation of the
> > + * we store a raw buffer representation of the
> > * uuid str so that we can compare this with the label
> > * area cookie irrespective of the endian config with which
> > * the kernel is built.
> > */
> > - p->nd_set.cookie1 = cpu_to_le64(uuid[0]);
> > - p->nd_set.cookie2 = cpu_to_le64(uuid[1]);
> > + export_uuid(uuid_raw, &uuid);
> > + p->nd_set.cookie1 = get_unaligned_le64(&uuid_raw[0]);
> > + p->nd_set.cookie2 = get_unaligned_le64(&uuid_raw[8]);
>
> ok that does the equivalent of cpu_to_le64 there. So we are good. But the
> comment update is missing the details why we did that get_unaligned_le64.
> Maybe raw buffer representation is the correct term?
> Should we add an example in the comment. ie,
> /*
> * Historically we stored the cookie in the below format.
> for a uuid str 72511b67-0b3b-42fd-8d1d-5be3cae8bcaa
> cookie1 was 0xfd423b0b671b5172 cookie2 was 0xaabce8cae35b1d8d
> */
I'm fine with the comment. At least it will shed a light on the byte ordering
we are expecting.
> > /* might be zero */
> > p->metadata_size = metadata_size;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v1 3/5] mm: ptdump: Provide page size to notepage()
From: Steven Price @ 2021-04-16 9:28 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, akpm
Cc: linux-arch, linux-s390, x86, linux-kernel, linux-mm, linux-riscv,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <1ef6b954fb7b0f4dfc78820f1e612d2166c13227.1618506910.git.christophe.leroy@csgroup.eu>
On 15/04/2021 18:18, Christophe Leroy wrote:
> In order to support large pages on powerpc, notepage()
> needs to know the page size of the page.
>
> Add a page_size argument to notepage().
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/arm64/mm/ptdump.c | 2 +-
> arch/riscv/mm/ptdump.c | 2 +-
> arch/s390/mm/dump_pagetables.c | 3 ++-
> arch/x86/mm/dump_pagetables.c | 2 +-
> include/linux/ptdump.h | 2 +-
> mm/ptdump.c | 16 ++++++++--------
> 6 files changed, 14 insertions(+), 13 deletions(-)
>
[...]
> diff --git a/mm/ptdump.c b/mm/ptdump.c
> index da751448d0e4..61cd16afb1c8 100644
> --- a/mm/ptdump.c
> +++ b/mm/ptdump.c
> @@ -17,7 +17,7 @@ static inline int note_kasan_page_table(struct mm_walk *walk,
> {
> struct ptdump_state *st = walk->private;
>
> - st->note_page(st, addr, 4, pte_val(kasan_early_shadow_pte[0]));
> + st->note_page(st, addr, 4, pte_val(kasan_early_shadow_pte[0]), PAGE_SIZE);
I'm not completely sure what the page_size is going to be used for, but
note that KASAN presents an interesting case here. We short-cut by
detecting it's a KASAN region at a high level (PGD/P4D/PUD/PMD) and
instead of walking the tree down just call note_page() *once* but with
level==4 because we know KASAN sets up the page table like that.
However the one call actually covers a much larger region - so while
PAGE_SIZE matches the level it doesn't match the region covered. AFAICT
this will lead to odd results if you enable KASAN on powerpc.
To be honest I don't fully understand why powerpc requires the page_size
- it appears to be using it purely to find "holes" in the calls to
note_page(), but I haven't worked out why such holes would occur.
Steve
>
> walk->action = ACTION_CONTINUE;
>
> @@ -41,7 +41,7 @@ static int ptdump_pgd_entry(pgd_t *pgd, unsigned long addr,
> st->effective_prot(st, 0, pgd_val(val));
>
> if (pgd_leaf(val))
> - st->note_page(st, addr, 0, pgd_val(val));
> + st->note_page(st, addr, 0, pgd_val(val), PGDIR_SIZE);
>
> return 0;
> }
> @@ -62,7 +62,7 @@ static int ptdump_p4d_entry(p4d_t *p4d, unsigned long addr,
> st->effective_prot(st, 1, p4d_val(val));
>
> if (p4d_leaf(val))
> - st->note_page(st, addr, 1, p4d_val(val));
> + st->note_page(st, addr, 1, p4d_val(val), P4D_SIZE);
>
> return 0;
> }
> @@ -83,7 +83,7 @@ static int ptdump_pud_entry(pud_t *pud, unsigned long addr,
> st->effective_prot(st, 2, pud_val(val));
>
> if (pud_leaf(val))
> - st->note_page(st, addr, 2, pud_val(val));
> + st->note_page(st, addr, 2, pud_val(val), PUD_SIZE);
>
> return 0;
> }
> @@ -102,7 +102,7 @@ static int ptdump_pmd_entry(pmd_t *pmd, unsigned long addr,
> if (st->effective_prot)
> st->effective_prot(st, 3, pmd_val(val));
> if (pmd_leaf(val))
> - st->note_page(st, addr, 3, pmd_val(val));
> + st->note_page(st, addr, 3, pmd_val(val), PMD_SIZE);
>
> return 0;
> }
> @@ -116,7 +116,7 @@ static int ptdump_pte_entry(pte_t *pte, unsigned long addr,
> if (st->effective_prot)
> st->effective_prot(st, 4, pte_val(val));
>
> - st->note_page(st, addr, 4, pte_val(val));
> + st->note_page(st, addr, 4, pte_val(val), PAGE_SIZE);
>
> return 0;
> }
> @@ -126,7 +126,7 @@ static int ptdump_hole(unsigned long addr, unsigned long next,
> {
> struct ptdump_state *st = walk->private;
>
> - st->note_page(st, addr, depth, 0);
> + st->note_page(st, addr, depth, 0, 0);
>
> return 0;
> }
> @@ -153,5 +153,5 @@ void ptdump_walk_pgd(struct ptdump_state *st, struct mm_struct *mm, pgd_t *pgd)
> mmap_read_unlock(mm);
>
> /* Flush out the last page */
> - st->note_page(st, 0, -1, 0);
> + st->note_page(st, 0, -1, 0, 0);
> }
>
^ permalink raw reply
* Re: [PATCH] powerpc/kdump: fix kdump kernel hangup issue with hot add CPUs
From: Hari Bathini @ 2021-04-16 9:33 UTC (permalink / raw)
To: Sourabh Jain, mpe; +Cc: mahesh, linux-kernel, linuxppc-dev
In-Reply-To: <20210416064749.657585-1-sourabhjain@linux.ibm.com>
On 16/04/21 12:17 pm, Sourabh Jain wrote:
> With the kexec_file_load system call when system crashes on the hot add
> CPU the capture kernel hangs and failed to collect the vmcore.
>
> Kernel panic - not syncing: sysrq triggered crash
> CPU: 24 PID: 6065 Comm: echo Kdump: loaded Not tainted 5.12.0-rc5upstream #54
> Call Trace:
> [c0000000e590fac0] [c0000000007b2400] dump_stack+0xc4/0x114 (unreliable)
> [c0000000e590fb00] [c000000000145290] panic+0x16c/0x41c
> [c0000000e590fba0] [c0000000008892e0] sysrq_handle_crash+0x30/0x40
> [c0000000e590fc00] [c000000000889cdc] __handle_sysrq+0xcc/0x1f0
> [c0000000e590fca0] [c00000000088a538] write_sysrq_trigger+0xd8/0x178
> [c0000000e590fce0] [c0000000005e9b7c] proc_reg_write+0x10c/0x1b0
> [c0000000e590fd10] [c0000000004f26d0] vfs_write+0xf0/0x330
> [c0000000e590fd60] [c0000000004f2aec] ksys_write+0x7c/0x140
> [c0000000e590fdb0] [c000000000031ee0] system_call_exception+0x150/0x290
> [c0000000e590fe10] [c00000000000ca5c] system_call_common+0xec/0x278
> --- interrupt: c00 at 0x7fff905b9664
> NIP: 00007fff905b9664 LR: 00007fff905320c4 CTR: 0000000000000000
> REGS: c0000000e590fe80 TRAP: 0c00 Not tainted (5.12.0-rc5upstream)
> MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 28000242
> XER: 00000000
> IRQMASK: 0
> GPR00: 0000000000000004 00007ffff5fedf30 00007fff906a7300 0000000000000001
> GPR04: 000001002a7355b0 0000000000000002 0000000000000001 00007ffff5fef616
> GPR08: 0000000000000001 0000000000000000 0000000000000000 0000000000000000
> GPR12: 0000000000000000 00007fff9073a160 0000000000000000 0000000000000000
> GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> GPR20: 0000000000000000 00007fff906a4ee0 0000000000000002 0000000000000001
> GPR24: 00007fff906a0898 0000000000000000 0000000000000002 000001002a7355b0
> GPR28: 0000000000000002 00007fff906a1790 000001002a7355b0 0000000000000002
> NIP [00007fff905b9664] 0x7fff905b9664
> LR [00007fff905320c4] 0x7fff905320c4
> --- interrupt: c00
<SNIP>
> /**
> * setup_new_fdt_ppc64 - Update the flattend device-tree of the kernel
> * being loaded.
> @@ -1020,6 +1113,13 @@ int setup_new_fdt_ppc64(const struct kimage *image, void *fdt,
> }
> }
>
> + /* Update cpus nodes information to account hotplug CPUs. */
> + if (image->type == KEXEC_TYPE_CRASH) {
Shouldn't this apply to regular kexec_file_load case as well? Yeah,
there won't be a hang in regular kexec_file_load case but for
correctness, that kernel should also not see stale CPU info in FDT?
Thanks
Hari
^ permalink raw reply
* Re: [PATCH v1 1/1] powerpc/papr_scm: Properly handle UUID types and API
From: Aneesh Kumar K.V @ 2021-04-16 9:35 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Oliver O'Halloran, linux-kernel, Paul Mackerras, Vaibhav Jain,
linuxppc-dev
In-Reply-To: <YHlUNSwm8Ofy9sNr@smile.fi.intel.com>
On 4/16/21 2:39 PM, Andy Shevchenko wrote:
> On Fri, Apr 16, 2021 at 01:28:21PM +0530, Aneesh Kumar K.V wrote:
>> On 4/15/21 7:16 PM, Andy Shevchenko wrote:
>>> Parse to and export from UUID own type, before dereferencing.
>>> This also fixes wrong comment (Little Endian UUID is something else)
>>> and should fix Sparse warnings about assigning strict types to POD.
>>>
>>> Fixes: 43001c52b603 ("powerpc/papr_scm: Use ibm,unit-guid as the iset cookie")
>>> Fixes: 259a948c4ba1 ("powerpc/pseries/scm: Use a specific endian format for storing uuid from the device tree")
>>> Cc: Oliver O'Halloran <oohall@gmail.com>
>>> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> ---
>>> Not tested
>>> arch/powerpc/platforms/pseries/papr_scm.c | 13 ++++++++-----
>>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>>> index ae6f5d80d5ce..4366e1902890 100644
>>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>>> @@ -1085,8 +1085,9 @@ static int papr_scm_probe(struct platform_device *pdev)
>>> u32 drc_index, metadata_size;
>>> u64 blocks, block_size;
>>> struct papr_scm_priv *p;
>>> + u8 uuid_raw[UUID_SIZE];
>>> const char *uuid_str;
>>> - u64 uuid[2];
>>> + uuid_t uuid;
>>> int rc;
>>> /* check we have all the required DT properties */
>>> @@ -1129,16 +1130,18 @@ static int papr_scm_probe(struct platform_device *pdev)
>>> p->hcall_flush_required = of_property_read_bool(dn, "ibm,hcall-flush-required");
>>> /* We just need to ensure that set cookies are unique across */
>>> - uuid_parse(uuid_str, (uuid_t *) uuid);
>>> + uuid_parse(uuid_str, &uuid);
>>> +
>>> /*
>>> * cookie1 and cookie2 are not really little endian
>>> - * we store a little endian representation of the
>>> + * we store a raw buffer representation of the
>>> * uuid str so that we can compare this with the label
>>> * area cookie irrespective of the endian config with which
>>> * the kernel is built.
>>> */
>>> - p->nd_set.cookie1 = cpu_to_le64(uuid[0]);
>>> - p->nd_set.cookie2 = cpu_to_le64(uuid[1]);
>>> + export_uuid(uuid_raw, &uuid);
>>> + p->nd_set.cookie1 = get_unaligned_le64(&uuid_raw[0]);
>>> + p->nd_set.cookie2 = get_unaligned_le64(&uuid_raw[8]);
>>
>> ok that does the equivalent of cpu_to_le64 there. So we are good. But the
>> comment update is missing the details why we did that get_unaligned_le64.
>> Maybe raw buffer representation is the correct term?
>> Should we add an example in the comment. ie,
>
>> /*
>> * Historically we stored the cookie in the below format.
>> for a uuid str 72511b67-0b3b-42fd-8d1d-5be3cae8bcaa
>> cookie1 was 0xfd423b0b671b5172 cookie2 was 0xaabce8cae35b1d8d
>> */
>
> I'm fine with the comment. At least it will shed a light on the byte ordering
> we are expecting.
>
Will you be sending an update? Also it will be good to list the sparse
warning in the commit message?
-aneesh
^ permalink raw reply
* Re: [PATCH v1 3/5] mm: ptdump: Provide page size to notepage()
From: Christophe Leroy @ 2021-04-16 10:38 UTC (permalink / raw)
To: Steven Price, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, akpm
Cc: linux-arch, linux-s390, x86, linux-kernel, linux-mm, linux-riscv,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <41819925-3ee5-4771-e98b-0073e8f095cf@arm.com>
Le 16/04/2021 à 11:28, Steven Price a écrit :
> On 15/04/2021 18:18, Christophe Leroy wrote:
>> In order to support large pages on powerpc, notepage()
>> needs to know the page size of the page.
>>
>> Add a page_size argument to notepage().
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> arch/arm64/mm/ptdump.c | 2 +-
>> arch/riscv/mm/ptdump.c | 2 +-
>> arch/s390/mm/dump_pagetables.c | 3 ++-
>> arch/x86/mm/dump_pagetables.c | 2 +-
>> include/linux/ptdump.h | 2 +-
>> mm/ptdump.c | 16 ++++++++--------
>> 6 files changed, 14 insertions(+), 13 deletions(-)
>>
> [...]
>> diff --git a/mm/ptdump.c b/mm/ptdump.c
>> index da751448d0e4..61cd16afb1c8 100644
>> --- a/mm/ptdump.c
>> +++ b/mm/ptdump.c
>> @@ -17,7 +17,7 @@ static inline int note_kasan_page_table(struct mm_walk *walk,
>> {
>> struct ptdump_state *st = walk->private;
>> - st->note_page(st, addr, 4, pte_val(kasan_early_shadow_pte[0]));
>> + st->note_page(st, addr, 4, pte_val(kasan_early_shadow_pte[0]), PAGE_SIZE);
>
> I'm not completely sure what the page_size is going to be used for, but note that KASAN presents an
> interesting case here. We short-cut by detecting it's a KASAN region at a high level
> (PGD/P4D/PUD/PMD) and instead of walking the tree down just call note_page() *once* but with
> level==4 because we know KASAN sets up the page table like that.
>
> However the one call actually covers a much larger region - so while PAGE_SIZE matches the level it
> doesn't match the region covered. AFAICT this will lead to odd results if you enable KASAN on powerpc.
Hum .... I successfully tested it with KASAN, I now realise that I tested it with
CONFIG_KASAN_VMALLOC selected. In this situation, since
https://github.com/torvalds/linux/commit/af3d0a686 we don't have any common shadow page table anymore.
I'll test again without CONFIG_KASAN_VMALLOC.
>
> To be honest I don't fully understand why powerpc requires the page_size - it appears to be using it
> purely to find "holes" in the calls to note_page(), but I haven't worked out why such holes would
> occur.
I was indeed introduced for KASAN. We have a first commit
https://github.com/torvalds/linux/commit/cabe8138 which uses page size to detect whether it is a
KASAN like stuff.
Then came https://github.com/torvalds/linux/commit/b00ff6d8c as a fix. I can't remember what the
problem was exactly, something around the use of hugepages for kernel memory, came as part of the
series
https://patchwork.ozlabs.org/project/linuxppc-dev/cover/cover.1589866984.git.christophe.leroy@csgroup.eu/
Christophe
^ permalink raw reply
* Re: [PATCH v3] powerpc: fix EDEADLOCK redefinition error in uapi/asm/errno.h
From: Michael Ellerman @ 2021-04-16 10:41 UTC (permalink / raw)
To: Tony Ambardar
Cc: linux-arch, Arnd Bergmann, LKML, Stable, Paul Mackerras,
Rosen Penev, bpf, linuxppc-dev
In-Reply-To: <CAPGftE-Q+Q479j7SikDBQLiM+VKbpXpRYnTeEJeAHeZrh_Ok2A@mail.gmail.com>
Tony Ambardar <tony.ambardar@gmail.com> writes:
> Hello Michael,
>
> The latest version of this patch addressed all feedback I'm aware of
> when submitted last September, and I've seen no further comments from
> reviewers since then.
>
> Could you please let me know where this stands and if anything further
> is needed?
Sorry, it's still sitting in my inbox :/
I was going to reply to suggest we split the tools change out. The
headers under tools are usually updated by another maintainer, I think
it might even be scripted.
Anyway I've applied your patch and done that (dropped the change to
tools/.../errno.h), which should also mean the stable backport is more
likely to work automatically.
It will hit mainline in v5.13-rc1 and then be backported to the stable
trees.
I don't think you actually need the tools version of the header updated
to fix your bug? In which case we can probably just wait for it to be
updated automatically when the tools headers are sync'ed with the kernel
versions.
cheers
> On Thu, 17 Sept 2020 at 06:54, Tony Ambardar <tony.ambardar@gmail.com> wrote:
>>
>> A few archs like powerpc have different errno.h values for macros
>> EDEADLOCK and EDEADLK. In code including both libc and linux versions of
>> errno.h, this can result in multiple definitions of EDEADLOCK in the
>> include chain. Definitions to the same value (e.g. seen with mips) do
>> not raise warnings, but on powerpc there are redefinitions changing the
>> value, which raise warnings and errors (if using "-Werror").
>>
>> Guard against these redefinitions to avoid build errors like the following,
>> first seen cross-compiling libbpf v5.8.9 for powerpc using GCC 8.4.0 with
>> musl 1.1.24:
>>
>> In file included from ../../arch/powerpc/include/uapi/asm/errno.h:5,
>> from ../../include/linux/err.h:8,
>> from libbpf.c:29:
>> ../../include/uapi/asm-generic/errno.h:40: error: "EDEADLOCK" redefined [-Werror]
>> #define EDEADLOCK EDEADLK
>>
>> In file included from toolchain-powerpc_8540_gcc-8.4.0_musl/include/errno.h:10,
>> from libbpf.c:26:
>> toolchain-powerpc_8540_gcc-8.4.0_musl/include/bits/errno.h:58: note: this is the location of the previous definition
>> #define EDEADLOCK 58
>>
>> cc1: all warnings being treated as errors
>>
>> CC: Stable <stable@vger.kernel.org>
>> Reported-by: Rosen Penev <rosenp@gmail.com>
>> Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com>
>> ---
>> v1 -> v2:
>> * clean up commit description formatting
>>
>> v2 -> v3: (per Michael Ellerman)
>> * drop indeterminate 'Fixes' tags, request stable backports instead
>> ---
>> arch/powerpc/include/uapi/asm/errno.h | 1 +
>> tools/arch/powerpc/include/uapi/asm/errno.h | 1 +
>> 2 files changed, 2 insertions(+)
>>
>> diff --git a/arch/powerpc/include/uapi/asm/errno.h b/arch/powerpc/include/uapi/asm/errno.h
>> index cc79856896a1..4ba87de32be0 100644
>> --- a/arch/powerpc/include/uapi/asm/errno.h
>> +++ b/arch/powerpc/include/uapi/asm/errno.h
>> @@ -2,6 +2,7 @@
>> #ifndef _ASM_POWERPC_ERRNO_H
>> #define _ASM_POWERPC_ERRNO_H
>>
>> +#undef EDEADLOCK
>> #include <asm-generic/errno.h>
>>
>> #undef EDEADLOCK
>> diff --git a/tools/arch/powerpc/include/uapi/asm/errno.h b/tools/arch/powerpc/include/uapi/asm/errno.h
>> index cc79856896a1..4ba87de32be0 100644
>> --- a/tools/arch/powerpc/include/uapi/asm/errno.h
>> +++ b/tools/arch/powerpc/include/uapi/asm/errno.h
>> @@ -2,6 +2,7 @@
>> #ifndef _ASM_POWERPC_ERRNO_H
>> #define _ASM_POWERPC_ERRNO_H
>>
>> +#undef EDEADLOCK
>> #include <asm-generic/errno.h>
>>
>> #undef EDEADLOCK
>> --
>> 2.25.1
>>
^ permalink raw reply
* Re: [PATCH v1 3/5] mm: ptdump: Provide page size to notepage()
From: Steven Price @ 2021-04-16 10:51 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, akpm
Cc: linux-arch, linux-s390, x86, linux-kernel, linux-mm, linux-riscv,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <da53d2f2-b472-0c38-bdd5-99c5a098675d@csgroup.eu>
On 16/04/2021 11:38, Christophe Leroy wrote:
>
>
> Le 16/04/2021 à 11:28, Steven Price a écrit :
>> On 15/04/2021 18:18, Christophe Leroy wrote:
>>> In order to support large pages on powerpc, notepage()
>>> needs to know the page size of the page.
>>>
>>> Add a page_size argument to notepage().
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> ---
>>> arch/arm64/mm/ptdump.c | 2 +-
>>> arch/riscv/mm/ptdump.c | 2 +-
>>> arch/s390/mm/dump_pagetables.c | 3 ++-
>>> arch/x86/mm/dump_pagetables.c | 2 +-
>>> include/linux/ptdump.h | 2 +-
>>> mm/ptdump.c | 16 ++++++++--------
>>> 6 files changed, 14 insertions(+), 13 deletions(-)
>>>
>> [...]
>>> diff --git a/mm/ptdump.c b/mm/ptdump.c
>>> index da751448d0e4..61cd16afb1c8 100644
>>> --- a/mm/ptdump.c
>>> +++ b/mm/ptdump.c
>>> @@ -17,7 +17,7 @@ static inline int note_kasan_page_table(struct
>>> mm_walk *walk,
>>> {
>>> struct ptdump_state *st = walk->private;
>>> - st->note_page(st, addr, 4, pte_val(kasan_early_shadow_pte[0]));
>>> + st->note_page(st, addr, 4, pte_val(kasan_early_shadow_pte[0]),
>>> PAGE_SIZE);
>>
>> I'm not completely sure what the page_size is going to be used for,
>> but note that KASAN presents an interesting case here. We short-cut by
>> detecting it's a KASAN region at a high level (PGD/P4D/PUD/PMD) and
>> instead of walking the tree down just call note_page() *once* but with
>> level==4 because we know KASAN sets up the page table like that.
>>
>> However the one call actually covers a much larger region - so while
>> PAGE_SIZE matches the level it doesn't match the region covered.
>> AFAICT this will lead to odd results if you enable KASAN on powerpc.
>
> Hum .... I successfully tested it with KASAN, I now realise that I
> tested it with CONFIG_KASAN_VMALLOC selected. In this situation, since
> https://github.com/torvalds/linux/commit/af3d0a686 we don't have any
> common shadow page table anymore.
>
> I'll test again without CONFIG_KASAN_VMALLOC.
>
>>
>> To be honest I don't fully understand why powerpc requires the
>> page_size - it appears to be using it purely to find "holes" in the
>> calls to note_page(), but I haven't worked out why such holes would
>> occur.
>
> I was indeed introduced for KASAN. We have a first commit
> https://github.com/torvalds/linux/commit/cabe8138 which uses page size
> to detect whether it is a KASAN like stuff.
>
> Then came https://github.com/torvalds/linux/commit/b00ff6d8c as a fix. I
> can't remember what the problem was exactly, something around the use of
> hugepages for kernel memory, came as part of the series
> https://patchwork.ozlabs.org/project/linuxppc-dev/cover/cover.1589866984.git.christophe.leroy@csgroup.eu/
Ah, that's useful context. So it looks like powerpc took a different
route to reducing the KASAN output to x86.
Given the generic ptdump code has handling for KASAN already it should
be possible to drop that from the powerpc arch code, which I think means
we don't actually need to provide page size to notepage(). Hopefully
that means more code to delete ;)
Steve
^ permalink raw reply
* Re: linux-next: build warning after merge of the powerpc tree
From: Michael Ellerman @ 2021-04-16 10:54 UTC (permalink / raw)
To: Stephen Rothwell, PowerPC
Cc: Linux Next Mailing List, Linux Kernel Mailing List,
Shivaprasad G Bhat
In-Reply-To: <20210415185214.01e1e64f@canb.auug.org.au>
Stephen Rothwell <sfr@canb.auug.org.au> writes:
> Hi all,
>
> After merging the powerpc tree, today's linux-next build (powerpc
> allyesconfig) produced this warning:
>
> In file included from include/linux/device.h:15,
> from arch/powerpc/include/asm/io.h:27,
> from include/linux/io.h:13,
> from include/linux/irq.h:20,
> from arch/powerpc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:11,
> from include/linux/highmem.h:10,
> from include/linux/bio.h:8,
> from include/linux/libnvdimm.h:14,
> from arch/powerpc/platforms/pseries/papr_scm.c:12:
> arch/powerpc/platforms/pseries/papr_scm.c: In function 'papr_scm_pmem_flush':
> arch/powerpc/platforms/pseries/papr_scm.c:144:26: warning: format '%lld' expects argument of type 'long long int', but argument 3 has type 'long int' [-Wformat=]
> 144 | dev_err(&p->pdev->dev, "flush error: %lld", rc);
> | ^~~~~~~~~~~~~~~~~~~
> include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt'
> 19 | #define dev_fmt(fmt) fmt
> | ^~~
> arch/powerpc/platforms/pseries/papr_scm.c:144:3: note: in expansion of macro 'dev_err'
> 144 | dev_err(&p->pdev->dev, "flush error: %lld", rc);
> | ^~~~~~~
> arch/powerpc/platforms/pseries/papr_scm.c:144:43: note: format string is defined here
> 144 | dev_err(&p->pdev->dev, "flush error: %lld", rc);
> | ~~~^
> | |
> | long long int
> | %ld
>
> Introduced by commit
>
> 75b7c05ebf90 ("powerpc/papr_scm: Implement support for H_SCM_FLUSH hcall")
My bad.
cheers
^ permalink raw reply
* Re: [PATCH 1/1] mm: Fix struct page layout on 32-bit systems
From: Matthew Wilcox @ 2021-04-16 11:05 UTC (permalink / raw)
To: David Laight
Cc: Arnd Bergmann, Grygorii Strashko, netdev@vger.kernel.org,
Ilias Apalodimas, linux-kernel@vger.kernel.org,
linux-mips@vger.kernel.org, linux-mm@kvack.org,
Jesper Dangaard Brouer, Matteo Croce,
linuxppc-dev@lists.ozlabs.org, Christoph Hellwig,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <f51e1aa98cb94880a236d58c75c20994@AcuMS.aculab.com>
On Fri, Apr 16, 2021 at 07:32:35AM +0000, David Laight wrote:
> From: Matthew Wilcox <willy@infradead.org>
> > Sent: 15 April 2021 23:22
> >
> > On Thu, Apr 15, 2021 at 09:11:56PM +0000, David Laight wrote:
> > > Isn't it possible to move the field down one long?
> > > This might require an explicit zero - but this is not a common
> > > code path - the extra write will be noise.
> >
> > Then it overlaps page->mapping. See emails passim.
>
> The rules on overlaps make be wonder if every 'long'
> should be in its own union.
That was what we used to have. It was worse.
> The comments would need to say when each field is used.
> It would, at least, make these errors less common.
>
> That doesn't solve the 64bit dma_addr though.
>
> Actually rather that word-swapping dma_addr on 32bit BE
> could you swap over the two fields it overlays with.
> That might look messy in the .h, but it doesn't require
> an accessor function to do the swap - easily missed.
No.
^ permalink raw reply
* Re: [PATCH v1 3/5] mm: ptdump: Provide page size to notepage()
From: Christophe Leroy @ 2021-04-16 11:08 UTC (permalink / raw)
To: Steven Price, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, akpm
Cc: linux-arch, linux-s390, x86, linux-kernel, linux-mm, linux-riscv,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <1102cda1-b00f-b6ef-6bf3-22068cc11510@arm.com>
Le 16/04/2021 à 12:51, Steven Price a écrit :
> On 16/04/2021 11:38, Christophe Leroy wrote:
>>
>>
>> Le 16/04/2021 à 11:28, Steven Price a écrit :
>>> On 15/04/2021 18:18, Christophe Leroy wrote:
>>>> In order to support large pages on powerpc, notepage()
>>>> needs to know the page size of the page.
>>>>
>>>> Add a page_size argument to notepage().
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>> ---
>>>> arch/arm64/mm/ptdump.c | 2 +-
>>>> arch/riscv/mm/ptdump.c | 2 +-
>>>> arch/s390/mm/dump_pagetables.c | 3 ++-
>>>> arch/x86/mm/dump_pagetables.c | 2 +-
>>>> include/linux/ptdump.h | 2 +-
>>>> mm/ptdump.c | 16 ++++++++--------
>>>> 6 files changed, 14 insertions(+), 13 deletions(-)
>>>>
>>> [...]
>>>> diff --git a/mm/ptdump.c b/mm/ptdump.c
>>>> index da751448d0e4..61cd16afb1c8 100644
>>>> --- a/mm/ptdump.c
>>>> +++ b/mm/ptdump.c
>>>> @@ -17,7 +17,7 @@ static inline int note_kasan_page_table(struct mm_walk *walk,
>>>> {
>>>> struct ptdump_state *st = walk->private;
>>>> - st->note_page(st, addr, 4, pte_val(kasan_early_shadow_pte[0]));
>>>> + st->note_page(st, addr, 4, pte_val(kasan_early_shadow_pte[0]), PAGE_SIZE);
>>>
>>> I'm not completely sure what the page_size is going to be used for, but note that KASAN presents
>>> an interesting case here. We short-cut by detecting it's a KASAN region at a high level
>>> (PGD/P4D/PUD/PMD) and instead of walking the tree down just call note_page() *once* but with
>>> level==4 because we know KASAN sets up the page table like that.
>>>
>>> However the one call actually covers a much larger region - so while PAGE_SIZE matches the level
>>> it doesn't match the region covered. AFAICT this will lead to odd results if you enable KASAN on
>>> powerpc.
>>
>> Hum .... I successfully tested it with KASAN, I now realise that I tested it with
>> CONFIG_KASAN_VMALLOC selected. In this situation, since
>> https://github.com/torvalds/linux/commit/af3d0a686 we don't have any common shadow page table
>> anymore.
>>
>> I'll test again without CONFIG_KASAN_VMALLOC.
>>
>>>
>>> To be honest I don't fully understand why powerpc requires the page_size - it appears to be using
>>> it purely to find "holes" in the calls to note_page(), but I haven't worked out why such holes
>>> would occur.
>>
>> I was indeed introduced for KASAN. We have a first commit
>> https://github.com/torvalds/linux/commit/cabe8138 which uses page size to detect whether it is a
>> KASAN like stuff.
>>
>> Then came https://github.com/torvalds/linux/commit/b00ff6d8c as a fix. I can't remember what the
>> problem was exactly, something around the use of hugepages for kernel memory, came as part of the
>> series
>> https://patchwork.ozlabs.org/project/linuxppc-dev/cover/cover.1589866984.git.christophe.leroy@csgroup.eu/
>
>
> Ah, that's useful context. So it looks like powerpc took a different route to reducing the KASAN
> output to x86.
>
> Given the generic ptdump code has handling for KASAN already it should be possible to drop that from
> the powerpc arch code, which I think means we don't actually need to provide page size to
> notepage(). Hopefully that means more code to delete ;)
>
Yes ... and no.
It looks like the generic ptdump handles the case when several pgdir entries points to the same
kasan_early_shadow_pte. But it doesn't take into account the powerpc case where we have regular page
tables where several (if not all) PTEs are pointing to the kasan_early_shadow_page .
Christophe
^ permalink raw reply
* [PATCH 1/2] powerpc/configs: Add PAPR_SCM to pseries_defconfig
From: Michael Ellerman @ 2021-04-16 11:12 UTC (permalink / raw)
To: linuxppc-dev
This is a pseries only driver, it should be built by default as part of
pseries_defconfig to get some build coverage.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/configs/pseries_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/configs/pseries_defconfig b/arch/powerpc/configs/pseries_defconfig
index 777221775c83..968095d7682c 100644
--- a/arch/powerpc/configs/pseries_defconfig
+++ b/arch/powerpc/configs/pseries_defconfig
@@ -41,6 +41,7 @@ CONFIG_DTL=y
CONFIG_SCANLOG=m
CONFIG_PPC_SMLPAR=y
CONFIG_IBMEBUS=y
+CONFIG_PAPR_SCM=m
CONFIG_PPC_SVM=y
# CONFIG_PPC_PMAC is not set
CONFIG_RTAS_FLASH=m
--
2.25.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox