| Message ID | 20231130023955.5257-6-bhe@redhat.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | kexec_file: print out debugging message if required | expand |
On Thu, Nov 30, 2023 at 10:39:53AM +0800, Baoquan He wrote: $subject has a typo in the arch bit :) > Replace pr_debug() with the newly added kexec_dprintk() in kexec_file > loading related codes. Commit messages should be understandable in isolation, but this only explains (part of) what is obvious in the diff. Why is this change being made? > > And also remove kexec_image_info() because the content has been printed > out in generic code. > > Signed-off-by: Baoquan He <bhe@redhat.com> > --- > arch/riscv/kernel/elf_kexec.c | 11 ++++++----- > arch/riscv/kernel/machine_kexec.c | 26 -------------------------- > 2 files changed, 6 insertions(+), 31 deletions(-) > > diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c > index e60fbd8660c4..5bd1ec3341fe 100644 > --- a/arch/riscv/kernel/elf_kexec.c > +++ b/arch/riscv/kernel/elf_kexec.c > @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > if (ret) > goto out; > kernel_start = image->start; > - pr_notice("The entry point of kernel at 0x%lx\n", image->start); > > /* Add the kernel binary to the image */ > ret = riscv_kexec_elf_load(image, &ehdr, &elf_info, > @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > image->elf_load_addr = kbuf.mem; > image->elf_headers_sz = headers_sz; > > - pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n", > - image->elf_load_addr, kbuf.bufsz, kbuf.memsz); > + kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n", > + image->elf_load_addr, kbuf.bufsz, kbuf.memsz); > > /* Setup cmdline for kdump kernel case */ > modified_cmdline = setup_kdump_cmdline(image, cmdline, > @@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > pr_err("Error loading purgatory ret=%d\n", ret); > goto out; > } > + kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem); > + > ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry", > &kernel_start, > sizeof(kernel_start), 0); > @@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > if (ret) > goto out; > initrd_pbase = kbuf.mem; > - pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase); > + kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase); This is not a pr_debug(). > } > > /* Add the DTB to the image */ > @@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > } > /* Cache the fdt buffer address for memory cleanup */ > image->arch.fdt = fdt; > - pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem); > + kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem); Neither is this. Why are they being moved from pr_notice()? Thanks, Conor. > goto out; > > out_free_fdt: > diff --git a/arch/riscv/kernel/machine_kexec.c b/arch/riscv/kernel/machine_kexec.c > index 2d139b724bc8..ed9cad20c039 100644 > --- a/arch/riscv/kernel/machine_kexec.c > +++ b/arch/riscv/kernel/machine_kexec.c > @@ -18,30 +18,6 @@ > #include <linux/interrupt.h> > #include <linux/irq.h> > > -/* > - * kexec_image_info - Print received image details > - */ > -static void > -kexec_image_info(const struct kimage *image) > -{ > - unsigned long i; > - > - pr_debug("Kexec image info:\n"); > - pr_debug("\ttype: %d\n", image->type); > - pr_debug("\tstart: %lx\n", image->start); > - pr_debug("\thead: %lx\n", image->head); > - pr_debug("\tnr_segments: %lu\n", image->nr_segments); > - > - for (i = 0; i < image->nr_segments; i++) { > - pr_debug("\t segment[%lu]: %016lx - %016lx", i, > - image->segment[i].mem, > - image->segment[i].mem + image->segment[i].memsz); > - pr_debug("\t\t0x%lx bytes, %lu pages\n", > - (unsigned long) image->segment[i].memsz, > - (unsigned long) image->segment[i].memsz / PAGE_SIZE); > - } > -} > - > /* > * machine_kexec_prepare - Initialize kexec > * > @@ -60,8 +36,6 @@ machine_kexec_prepare(struct kimage *image) > unsigned int control_code_buffer_sz = 0; > int i = 0; > > - kexec_image_info(image); > - > /* Find the Flattened Device Tree and save its physical address */ > for (i = 0; i < image->nr_segments; i++) { > if (image->segment[i].memsz <= sizeof(fdt)) > -- > 2.41.0 >
On 12/01/23 at 10:38am, Conor Dooley wrote: > On Thu, Nov 30, 2023 at 10:39:53AM +0800, Baoquan He wrote: > > $subject has a typo in the arch bit :) Indeed, will fix if need report. Thanks for careful checking. > > > Replace pr_debug() with the newly added kexec_dprintk() in kexec_file > > loading related codes. > > Commit messages should be understandable in isolation, but this only > explains (part of) what is obvious in the diff. Why is this change > being made? The purpose has been detailedly described in cover letter and patch 1 log. Andrew has picked these patches into his tree and grabbed the cover letter log into the relevant commit for people's later checking. All these seven patches will be present in mainline together. This is common way when posting patch series? Please let me know if I misunderstand anything. > > > > > And also remove kexec_image_info() because the content has been printed > > out in generic code. > > > > Signed-off-by: Baoquan He <bhe@redhat.com> > > --- > > arch/riscv/kernel/elf_kexec.c | 11 ++++++----- > > arch/riscv/kernel/machine_kexec.c | 26 -------------------------- > > 2 files changed, 6 insertions(+), 31 deletions(-) > > > > diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c > > index e60fbd8660c4..5bd1ec3341fe 100644 > > --- a/arch/riscv/kernel/elf_kexec.c > > +++ b/arch/riscv/kernel/elf_kexec.c > > @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > if (ret) > > goto out; > > kernel_start = image->start; > > - pr_notice("The entry point of kernel at 0x%lx\n", image->start); > > > > /* Add the kernel binary to the image */ > > ret = riscv_kexec_elf_load(image, &ehdr, &elf_info, > > @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > image->elf_load_addr = kbuf.mem; > > image->elf_headers_sz = headers_sz; > > > > - pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n", > > - image->elf_load_addr, kbuf.bufsz, kbuf.memsz); > > + kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n", > > + image->elf_load_addr, kbuf.bufsz, kbuf.memsz); > > > > /* Setup cmdline for kdump kernel case */ > > modified_cmdline = setup_kdump_cmdline(image, cmdline, > > @@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > pr_err("Error loading purgatory ret=%d\n", ret); > > goto out; > > } > > + kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem); > > + > > ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry", > > &kernel_start, > > sizeof(kernel_start), 0); > > @@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > if (ret) > > goto out; > > initrd_pbase = kbuf.mem; > > > - pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase); > > + kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase); > > This is not a pr_debug(). > > > } > > > > /* Add the DTB to the image */ > > @@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > } > > /* Cache the fdt buffer address for memory cleanup */ > > image->arch.fdt = fdt; > > > - pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem); > > + kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem); > > Neither is this. Why are they being moved from pr_notice()? You are right. While always printing out the loaded location of purgatory and device tree doesn't make sense. It will be confusing when users see these even when they do normal kexec/kdump loading. It should be changed to pr_debug(). Which way do you suggest? 1) change it back to pr_debug(), fix it in another patch; 2) keep it as is in the patch; Thanks Baoquan
On Mon, Dec 04, 2023 at 11:38:05PM +0800, Baoquan He wrote: > On 12/01/23 at 10:38am, Conor Dooley wrote: > > On Thu, Nov 30, 2023 at 10:39:53AM +0800, Baoquan He wrote: > > > > $subject has a typo in the arch bit :) > > Indeed, will fix if need report. Thanks for careful checking. > > > > > > Replace pr_debug() with the newly added kexec_dprintk() in kexec_file > > > loading related codes. > > > > Commit messages should be understandable in isolation, but this only > > explains (part of) what is obvious in the diff. Why is this change > > being made? > > The purpose has been detailedly described in cover letter and patch 1 > log. Andrew has picked these patches into his tree and grabbed the cover > letter log into the relevant commit for people's later checking. All > these seven patches will be present in mainline together. This is common > way when posting patch series? Please let me know if I misunderstand > anything. Each patch having a commit message that explains why a change is being made is the expectation. It is especially useful to explain the why here, since it is not just a mechanical conversion of pr_debug()s as the commit message suggests. > > > > > > > > And also remove kexec_image_info() because the content has been printed > > > out in generic code. > > > > > > Signed-off-by: Baoquan He <bhe@redhat.com> > > > --- > > > arch/riscv/kernel/elf_kexec.c | 11 ++++++----- > > > arch/riscv/kernel/machine_kexec.c | 26 -------------------------- > > > 2 files changed, 6 insertions(+), 31 deletions(-) > > > > > > diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c > > > index e60fbd8660c4..5bd1ec3341fe 100644 > > > --- a/arch/riscv/kernel/elf_kexec.c > > > +++ b/arch/riscv/kernel/elf_kexec.c > > > @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > if (ret) > > > goto out; > > > kernel_start = image->start; > > > - pr_notice("The entry point of kernel at 0x%lx\n", image->start); > > > > > > /* Add the kernel binary to the image */ > > > ret = riscv_kexec_elf_load(image, &ehdr, &elf_info, > > > @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > image->elf_load_addr = kbuf.mem; > > > image->elf_headers_sz = headers_sz; > > > > > > - pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n", > > > - image->elf_load_addr, kbuf.bufsz, kbuf.memsz); > > > + kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n", > > > + image->elf_load_addr, kbuf.bufsz, kbuf.memsz); > > > > > > /* Setup cmdline for kdump kernel case */ > > > modified_cmdline = setup_kdump_cmdline(image, cmdline, > > > @@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > pr_err("Error loading purgatory ret=%d\n", ret); > > > goto out; > > > } > > > + kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem); > > > + > > > ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry", > > > &kernel_start, > > > sizeof(kernel_start), 0); > > > @@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > if (ret) > > > goto out; > > > initrd_pbase = kbuf.mem; > > > > > - pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase); > > > + kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase); > > > > This is not a pr_debug(). > > > > > } > > > > > > /* Add the DTB to the image */ > > > @@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > } > > > /* Cache the fdt buffer address for memory cleanup */ > > > image->arch.fdt = fdt; > > > > > - pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem); > > > + kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem); > > > > Neither is this. Why are they being moved from pr_notice()? > > You are right. > > While always printing out the loaded location of purgatory and > device tree doesn't make sense. It will be confusing when users > see these even when they do normal kexec/kdump loading. It should be > changed to pr_debug(). > > Which way do you suggest? > 1) change it back to pr_debug(), fix it in another patch; > 2) keep it as is in the patch; Personally I think it is fine to change them all in one patch, but the rationale for converting pr_notice() to your new debug infrastructure needs to be in the commit message. Thanks, Conor.
On 12/04/23 at 04:14pm, Conor Dooley wrote: > On Mon, Dec 04, 2023 at 11:38:05PM +0800, Baoquan He wrote: > > On 12/01/23 at 10:38am, Conor Dooley wrote: > > > On Thu, Nov 30, 2023 at 10:39:53AM +0800, Baoquan He wrote: > > > > > > $subject has a typo in the arch bit :) > > > > Indeed, will fix if need report. Thanks for careful checking. > > > > > > > > > Replace pr_debug() with the newly added kexec_dprintk() in kexec_file > > > > loading related codes. > > > > > > Commit messages should be understandable in isolation, but this only > > > explains (part of) what is obvious in the diff. Why is this change > > > being made? > > > > The purpose has been detailedly described in cover letter and patch 1 > > log. Andrew has picked these patches into his tree and grabbed the cover > > letter log into the relevant commit for people's later checking. All > > these seven patches will be present in mainline together. This is common > > way when posting patch series? Please let me know if I misunderstand > > anything. > > Each patch having a commit message that explains why a change is being > made is the expectation. It is especially useful to explain the why > here, since it is not just a mechanical conversion of pr_debug()s as the > commit message suggests. Sounds reasonable. I rephrase the patch 3 log as below, do you think it's OK to you? I will also adjust patch logs on other ARCH once this one is done. Thanks. ============================= Subject: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required Then when specifying '-d' for kexec_file_load interface, loaded locations of kernel/initrd/cmdline etc can be printed out to help debug. Here replace pr_debug() with the newly added kexec_dprintk() in kexec_file loading related codes. And also replace pr_notice() with kexec_dprintk() in elf_kexec_load() because it's make sense to always print out loaded location of purgatory and device tree even though users don't expect the message. And also remove kexec_image_info() because the content has been printed out in generic code. ============================ > > > > > > > > > > > > And also remove kexec_image_info() because the content has been printed > > > > out in generic code. > > > > > > > > Signed-off-by: Baoquan He <bhe@redhat.com> > > > > --- > > > > arch/riscv/kernel/elf_kexec.c | 11 ++++++----- > > > > arch/riscv/kernel/machine_kexec.c | 26 -------------------------- > > > > 2 files changed, 6 insertions(+), 31 deletions(-) > > > > > > > > diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c > > > > index e60fbd8660c4..5bd1ec3341fe 100644 > > > > --- a/arch/riscv/kernel/elf_kexec.c > > > > +++ b/arch/riscv/kernel/elf_kexec.c > > > > @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > if (ret) > > > > goto out; > > > > kernel_start = image->start; > > > > - pr_notice("The entry point of kernel at 0x%lx\n", image->start); > > > > > > > > /* Add the kernel binary to the image */ > > > > ret = riscv_kexec_elf_load(image, &ehdr, &elf_info, > > > > @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > image->elf_load_addr = kbuf.mem; > > > > image->elf_headers_sz = headers_sz; > > > > > > > > - pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n", > > > > - image->elf_load_addr, kbuf.bufsz, kbuf.memsz); > > > > + kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n", > > > > + image->elf_load_addr, kbuf.bufsz, kbuf.memsz); > > > > > > > > /* Setup cmdline for kdump kernel case */ > > > > modified_cmdline = setup_kdump_cmdline(image, cmdline, > > > > @@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > pr_err("Error loading purgatory ret=%d\n", ret); > > > > goto out; > > > > } > > > > + kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem); > > > > + > > > > ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry", > > > > &kernel_start, > > > > sizeof(kernel_start), 0); > > > > @@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > if (ret) > > > > goto out; > > > > initrd_pbase = kbuf.mem; > > > > > > > - pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase); > > > > + kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase); > > > > > > This is not a pr_debug(). > > > > > > > } > > > > > > > > /* Add the DTB to the image */ > > > > @@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > } > > > > /* Cache the fdt buffer address for memory cleanup */ > > > > image->arch.fdt = fdt; > > > > > > > - pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem); > > > > + kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem); > > > > > > Neither is this. Why are they being moved from pr_notice()? > > > > You are right. > > > > While always printing out the loaded location of purgatory and > > device tree doesn't make sense. It will be confusing when users > > see these even when they do normal kexec/kdump loading. It should be > > changed to pr_debug(). > > > > Which way do you suggest? > > 1) change it back to pr_debug(), fix it in another patch; > > 2) keep it as is in the patch; > > Personally I think it is fine to change them all in one patch, but the > rationale for converting pr_notice() to your new debug infrastructure > needs to be in the commit message. Sure, sounds good to me. I have changed the patch log to reflect this as you suggested, please help check. Thanks again.
On Wed, Dec 06, 2023 at 11:37:52PM +0800, Baoquan He wrote: > On 12/04/23 at 04:14pm, Conor Dooley wrote: > > On Mon, Dec 04, 2023 at 11:38:05PM +0800, Baoquan He wrote: > > > On 12/01/23 at 10:38am, Conor Dooley wrote: > > > > On Thu, Nov 30, 2023 at 10:39:53AM +0800, Baoquan He wrote: > > > > > > > > $subject has a typo in the arch bit :) > > > > > > Indeed, will fix if need report. Thanks for careful checking. > > > > > > > > > > > > Replace pr_debug() with the newly added kexec_dprintk() in kexec_file > > > > > loading related codes. > > > > > > > > Commit messages should be understandable in isolation, but this only > > > > explains (part of) what is obvious in the diff. Why is this change > > > > being made? > > > > > > The purpose has been detailedly described in cover letter and patch 1 > > > log. Andrew has picked these patches into his tree and grabbed the cover > > > letter log into the relevant commit for people's later checking. All > > > these seven patches will be present in mainline together. This is common > > > way when posting patch series? Please let me know if I misunderstand > > > anything. > > > > Each patch having a commit message that explains why a change is being > > made is the expectation. It is especially useful to explain the why > > here, since it is not just a mechanical conversion of pr_debug()s as the > > commit message suggests. > > Sounds reasonable. I rephrase the patch 3 log as below, do you think > it's OK to you? Yes, but with one comment. > > I will also adjust patch logs on other ARCH once this one is done. > Thanks. > > ============================= > Subject: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required > > Then when specifying '-d' for kexec_file_load interface, loaded > locations of kernel/initrd/cmdline etc can be printed out to help debug. > > Here replace pr_debug() with the newly added kexec_dprintk() in kexec_file > loading related codes. > > And also replace pr_notice() with kexec_dprintk() in elf_kexec_load() > because it's make sense to always print out loaded location of purgatory > and device tree even though users don't expect the message. This seems to contradict what you said in your earlier mail, about moving these from notice to debug. I think you missed a negation in your new version of the commit message. What you said in response to me seems like a more complete explanation anyway: always printing out the loaded location of purgatory and device tree doesn't make sense. It will be confusing when users see these even when they do normal kexec/kdump loading. Thanks, Conor. > And also remove kexec_image_info() because the content has been printed > out in generic code. > > ============================ > > > > > > > > > > > > > > > > > And also remove kexec_image_info() because the content has been printed > > > > > out in generic code. > > > > > > > > > > Signed-off-by: Baoquan He <bhe@redhat.com> > > > > > --- > > > > > arch/riscv/kernel/elf_kexec.c | 11 ++++++----- > > > > > arch/riscv/kernel/machine_kexec.c | 26 -------------------------- > > > > > 2 files changed, 6 insertions(+), 31 deletions(-) > > > > > > > > > > diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c > > > > > index e60fbd8660c4..5bd1ec3341fe 100644 > > > > > --- a/arch/riscv/kernel/elf_kexec.c > > > > > +++ b/arch/riscv/kernel/elf_kexec.c > > > > > @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > > if (ret) > > > > > goto out; > > > > > kernel_start = image->start; > > > > > - pr_notice("The entry point of kernel at 0x%lx\n", image->start); > > > > > > > > > > /* Add the kernel binary to the image */ > > > > > ret = riscv_kexec_elf_load(image, &ehdr, &elf_info, > > > > > @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > > image->elf_load_addr = kbuf.mem; > > > > > image->elf_headers_sz = headers_sz; > > > > > > > > > > - pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n", > > > > > - image->elf_load_addr, kbuf.bufsz, kbuf.memsz); > > > > > + kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n", > > > > > + image->elf_load_addr, kbuf.bufsz, kbuf.memsz); > > > > > > > > > > /* Setup cmdline for kdump kernel case */ > > > > > modified_cmdline = setup_kdump_cmdline(image, cmdline, > > > > > @@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > > pr_err("Error loading purgatory ret=%d\n", ret); > > > > > goto out; > > > > > } > > > > > + kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem); > > > > > + > > > > > ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry", > > > > > &kernel_start, > > > > > sizeof(kernel_start), 0); > > > > > @@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > > if (ret) > > > > > goto out; > > > > > initrd_pbase = kbuf.mem; > > > > > > > > > - pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase); > > > > > + kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase); > > > > > > > > This is not a pr_debug(). > > > > > > > > > } > > > > > > > > > > /* Add the DTB to the image */ > > > > > @@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > > } > > > > > /* Cache the fdt buffer address for memory cleanup */ > > > > > image->arch.fdt = fdt; > > > > > > > > > - pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem); > > > > > + kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem); > > > > > > > > Neither is this. Why are they being moved from pr_notice()? > > > > > > You are right. > > > > > > While always printing out the loaded location of purgatory and > > > device tree doesn't make sense. It will be confusing when users > > > see these even when they do normal kexec/kdump loading. It should be > > > changed to pr_debug(). > > > > > > Which way do you suggest? > > > 1) change it back to pr_debug(), fix it in another patch; > > > 2) keep it as is in the patch; > > > > Personally I think it is fine to change them all in one patch, but the > > rationale for converting pr_notice() to your new debug infrastructure > > needs to be in the commit message. > > Sure, sounds good to me. I have changed the patch log to reflect this as > you suggested, please help check. Thanks again. >
On 12/06/23 at 04:54pm, Conor Dooley wrote: > On Wed, Dec 06, 2023 at 11:37:52PM +0800, Baoquan He wrote: > > On 12/04/23 at 04:14pm, Conor Dooley wrote: > > > On Mon, Dec 04, 2023 at 11:38:05PM +0800, Baoquan He wrote: > > > > On 12/01/23 at 10:38am, Conor Dooley wrote: > > > > > On Thu, Nov 30, 2023 at 10:39:53AM +0800, Baoquan He wrote: > > > > > > > > > > $subject has a typo in the arch bit :) > > > > > > > > Indeed, will fix if need report. Thanks for careful checking. > > > > > > > > > > > > > > > Replace pr_debug() with the newly added kexec_dprintk() in kexec_file > > > > > > loading related codes. > > > > > > > > > > Commit messages should be understandable in isolation, but this only > > > > > explains (part of) what is obvious in the diff. Why is this change > > > > > being made? > > > > > > > > The purpose has been detailedly described in cover letter and patch 1 > > > > log. Andrew has picked these patches into his tree and grabbed the cover > > > > letter log into the relevant commit for people's later checking. All > > > > these seven patches will be present in mainline together. This is common > > > > way when posting patch series? Please let me know if I misunderstand > > > > anything. > > > > > > Each patch having a commit message that explains why a change is being > > > made is the expectation. It is especially useful to explain the why > > > here, since it is not just a mechanical conversion of pr_debug()s as the > > > commit message suggests. > > > > Sounds reasonable. I rephrase the patch 3 log as below, do you think > > it's OK to you? > > Yes, but with one comment. > > > > > I will also adjust patch logs on other ARCH once this one is done. > > Thanks. > > > > ============================= > > Subject: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required > > > > Then when specifying '-d' for kexec_file_load interface, loaded > > locations of kernel/initrd/cmdline etc can be printed out to help debug. > > > > Here replace pr_debug() with the newly added kexec_dprintk() in kexec_file > > loading related codes. > > > > > And also replace pr_notice() with kexec_dprintk() in elf_kexec_load() > > because it's make sense to always print out loaded location of purgatory ~ > > and device tree even though users don't expect the message. Fixed typo: ========== And also replace pr_notice() with kexec_dprintk() in elf_kexec_load() because it doesn't make sense to always print out loaded location of purgatory and device tree even though users don't expect the message. > > This seems to contradict what you said in your earlier mail, about > moving these from notice to debug. I think you missed a negation in your > new version of the commit message. What you said in response to me seems > like a more complete explanation anyway: Ah, I made mistake when typing, these printing is only for debugging, so always printing out them is not suggested. > always printing out the loaded location of purgatory and > device tree doesn't make sense. It will be confusing when users > see these even when they do normal kexec/kdump loading. > > Thanks, > Conor. > > > And also remove kexec_image_info() because the content has been printed > > out in generic code. > > > > ============================ > > > > > > > > > > > > > > > > > > > > > > And also remove kexec_image_info() because the content has been printed > > > > > > out in generic code. > > > > > > > > > > > > Signed-off-by: Baoquan He <bhe@redhat.com> > > > > > > --- > > > > > > arch/riscv/kernel/elf_kexec.c | 11 ++++++----- > > > > > > arch/riscv/kernel/machine_kexec.c | 26 -------------------------- > > > > > > 2 files changed, 6 insertions(+), 31 deletions(-) > > > > > > > > > > > > diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c > > > > > > index e60fbd8660c4..5bd1ec3341fe 100644 > > > > > > --- a/arch/riscv/kernel/elf_kexec.c > > > > > > +++ b/arch/riscv/kernel/elf_kexec.c > > > > > > @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > > > if (ret) > > > > > > goto out; > > > > > > kernel_start = image->start; > > > > > > - pr_notice("The entry point of kernel at 0x%lx\n", image->start); > > > > > > > > > > > > /* Add the kernel binary to the image */ > > > > > > ret = riscv_kexec_elf_load(image, &ehdr, &elf_info, > > > > > > @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > > > image->elf_load_addr = kbuf.mem; > > > > > > image->elf_headers_sz = headers_sz; > > > > > > > > > > > > - pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n", > > > > > > - image->elf_load_addr, kbuf.bufsz, kbuf.memsz); > > > > > > + kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n", > > > > > > + image->elf_load_addr, kbuf.bufsz, kbuf.memsz); > > > > > > > > > > > > /* Setup cmdline for kdump kernel case */ > > > > > > modified_cmdline = setup_kdump_cmdline(image, cmdline, > > > > > > @@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > > > pr_err("Error loading purgatory ret=%d\n", ret); > > > > > > goto out; > > > > > > } > > > > > > + kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem); > > > > > > + > > > > > > ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry", > > > > > > &kernel_start, > > > > > > sizeof(kernel_start), 0); > > > > > > @@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > > > if (ret) > > > > > > goto out; > > > > > > initrd_pbase = kbuf.mem; > > > > > > > > > > > - pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase); > > > > > > + kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase); > > > > > > > > > > This is not a pr_debug(). > > > > > > > > > > > } > > > > > > > > > > > > /* Add the DTB to the image */ > > > > > > @@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > > > } > > > > > > /* Cache the fdt buffer address for memory cleanup */ > > > > > > image->arch.fdt = fdt; > > > > > > > > > > > - pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem); > > > > > > + kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem); > > > > > > > > > > Neither is this. Why are they being moved from pr_notice()? > > > > > > > > You are right. > > > > > > > > While always printing out the loaded location of purgatory and > > > > device tree doesn't make sense. It will be confusing when users > > > > see these even when they do normal kexec/kdump loading. It should be > > > > changed to pr_debug(). > > > > > > > > Which way do you suggest? > > > > 1) change it back to pr_debug(), fix it in another patch; > > > > 2) keep it as is in the patch; > > > > > > Personally I think it is fine to change them all in one patch, but the > > > rationale for converting pr_notice() to your new debug infrastructure > > > needs to be in the commit message. > > > > Sure, sounds good to me. I have changed the patch log to reflect this as > > you suggested, please help check. Thanks again. > >
On 12/07/23 at 07:22am, Baoquan He wrote: > On 12/06/23 at 04:54pm, Conor Dooley wrote: > > On Wed, Dec 06, 2023 at 11:37:52PM +0800, Baoquan He wrote: > > > On 12/04/23 at 04:14pm, Conor Dooley wrote: > > > > On Mon, Dec 04, 2023 at 11:38:05PM +0800, Baoquan He wrote: > > > > > On 12/01/23 at 10:38am, Conor Dooley wrote: > > > > > > On Thu, Nov 30, 2023 at 10:39:53AM +0800, Baoquan He wrote: > > > > > > > > > > > > $subject has a typo in the arch bit :) > > > > > > > > > > Indeed, will fix if need report. Thanks for careful checking. > > > > > > > > > > > > > > > > > > Replace pr_debug() with the newly added kexec_dprintk() in kexec_file > > > > > > > loading related codes. > > > > > > > > > > > > Commit messages should be understandable in isolation, but this only > > > > > > explains (part of) what is obvious in the diff. Why is this change > > > > > > being made? > > > > > > > > > > The purpose has been detailedly described in cover letter and patch 1 > > > > > log. Andrew has picked these patches into his tree and grabbed the cover > > > > > letter log into the relevant commit for people's later checking. All > > > > > these seven patches will be present in mainline together. This is common > > > > > way when posting patch series? Please let me know if I misunderstand > > > > > anything. > > > > > > > > Each patch having a commit message that explains why a change is being > > > > made is the expectation. It is especially useful to explain the why > > > > here, since it is not just a mechanical conversion of pr_debug()s as the > > > > commit message suggests. > > > > > > Sounds reasonable. I rephrase the patch 3 log as below, do you think > > > it's OK to you? > > > > Yes, but with one comment. > > > > > > > > I will also adjust patch logs on other ARCH once this one is done. > > > Thanks. > > > > > > ============================= > > > Subject: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required > > > > > > Then when specifying '-d' for kexec_file_load interface, loaded > > > locations of kernel/initrd/cmdline etc can be printed out to help debug. > > > > > > Here replace pr_debug() with the newly added kexec_dprintk() in kexec_file > > > loading related codes. > > > > > > > > And also replace pr_notice() with kexec_dprintk() in elf_kexec_load() > > > because it's make sense to always print out loaded location of purgatory > ~ > > > and device tree even though users don't expect the message. > > Fixed typo: > ========== > > And also replace pr_notice() with kexec_dprintk() in elf_kexec_load() > because it doesn't make sense to always print out loaded location of > purgatory and device tree even though users don't expect the message. I will post v4 to include these suggested changes, please add comments if there's any concern. Thanks for reviewing. > > > > > This seems to contradict what you said in your earlier mail, about > > moving these from notice to debug. I think you missed a negation in your > > new version of the commit message. What you said in response to me seems > > like a more complete explanation anyway: > > Ah, I made mistake when typing, these printing is only for debugging, > so always printing out them is not suggested. > > > always printing out the loaded location of purgatory and > > device tree doesn't make sense. It will be confusing when users > > see these even when they do normal kexec/kdump loading. > > > > Thanks, > > Conor. > > > > > And also remove kexec_image_info() because the content has been printed > > > out in generic code. > > > > > > ============================ > > > > > > > > > > > > > > > > > > > > > > > > > > > And also remove kexec_image_info() because the content has been printed > > > > > > > out in generic code. > > > > > > > > > > > > > > Signed-off-by: Baoquan He <bhe@redhat.com> > > > > > > > --- > > > > > > > arch/riscv/kernel/elf_kexec.c | 11 ++++++----- > > > > > > > arch/riscv/kernel/machine_kexec.c | 26 -------------------------- > > > > > > > 2 files changed, 6 insertions(+), 31 deletions(-) > > > > > > > > > > > > > > diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c > > > > > > > index e60fbd8660c4..5bd1ec3341fe 100644 > > > > > > > --- a/arch/riscv/kernel/elf_kexec.c > > > > > > > +++ b/arch/riscv/kernel/elf_kexec.c > > > > > > > @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > > > > if (ret) > > > > > > > goto out; > > > > > > > kernel_start = image->start; > > > > > > > - pr_notice("The entry point of kernel at 0x%lx\n", image->start); > > > > > > > > > > > > > > /* Add the kernel binary to the image */ > > > > > > > ret = riscv_kexec_elf_load(image, &ehdr, &elf_info, > > > > > > > @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > > > > image->elf_load_addr = kbuf.mem; > > > > > > > image->elf_headers_sz = headers_sz; > > > > > > > > > > > > > > - pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n", > > > > > > > - image->elf_load_addr, kbuf.bufsz, kbuf.memsz); > > > > > > > + kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n", > > > > > > > + image->elf_load_addr, kbuf.bufsz, kbuf.memsz); > > > > > > > > > > > > > > /* Setup cmdline for kdump kernel case */ > > > > > > > modified_cmdline = setup_kdump_cmdline(image, cmdline, > > > > > > > @@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > > > > pr_err("Error loading purgatory ret=%d\n", ret); > > > > > > > goto out; > > > > > > > } > > > > > > > + kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem); > > > > > > > + > > > > > > > ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry", > > > > > > > &kernel_start, > > > > > > > sizeof(kernel_start), 0); > > > > > > > @@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > > > > if (ret) > > > > > > > goto out; > > > > > > > initrd_pbase = kbuf.mem; > > > > > > > > > > > > > - pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase); > > > > > > > + kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase); > > > > > > > > > > > > This is not a pr_debug(). > > > > > > > > > > > > > } > > > > > > > > > > > > > > /* Add the DTB to the image */ > > > > > > > @@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, > > > > > > > } > > > > > > > /* Cache the fdt buffer address for memory cleanup */ > > > > > > > image->arch.fdt = fdt; > > > > > > > > > > > > > - pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem); > > > > > > > + kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem); > > > > > > > > > > > > Neither is this. Why are they being moved from pr_notice()? > > > > > > > > > > You are right. > > > > > > > > > > While always printing out the loaded location of purgatory and > > > > > device tree doesn't make sense. It will be confusing when users > > > > > see these even when they do normal kexec/kdump loading. It should be > > > > > changed to pr_debug(). > > > > > > > > > > Which way do you suggest? > > > > > 1) change it back to pr_debug(), fix it in another patch; > > > > > 2) keep it as is in the patch; > > > > > > > > Personally I think it is fine to change them all in one patch, but the > > > > rationale for converting pr_notice() to your new debug infrastructure > > > > needs to be in the commit message. > > > > > > Sure, sounds good to me. I have changed the patch log to reflect this as > > > you suggested, please help check. Thanks again. > > > > >
diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c index e60fbd8660c4..5bd1ec3341fe 100644 --- a/arch/riscv/kernel/elf_kexec.c +++ b/arch/riscv/kernel/elf_kexec.c @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, if (ret) goto out; kernel_start = image->start; - pr_notice("The entry point of kernel at 0x%lx\n", image->start); /* Add the kernel binary to the image */ ret = riscv_kexec_elf_load(image, &ehdr, &elf_info, @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, image->elf_load_addr = kbuf.mem; image->elf_headers_sz = headers_sz; - pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n", - image->elf_load_addr, kbuf.bufsz, kbuf.memsz); + kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n", + image->elf_load_addr, kbuf.bufsz, kbuf.memsz); /* Setup cmdline for kdump kernel case */ modified_cmdline = setup_kdump_cmdline(image, cmdline, @@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, pr_err("Error loading purgatory ret=%d\n", ret); goto out; } + kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem); + ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry", &kernel_start, sizeof(kernel_start), 0); @@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, if (ret) goto out; initrd_pbase = kbuf.mem; - pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase); + kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase); } /* Add the DTB to the image */ @@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, } /* Cache the fdt buffer address for memory cleanup */ image->arch.fdt = fdt; - pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem); + kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem); goto out; out_free_fdt: diff --git a/arch/riscv/kernel/machine_kexec.c b/arch/riscv/kernel/machine_kexec.c index 2d139b724bc8..ed9cad20c039 100644 --- a/arch/riscv/kernel/machine_kexec.c +++ b/arch/riscv/kernel/machine_kexec.c @@ -18,30 +18,6 @@ #include <linux/interrupt.h> #include <linux/irq.h> -/* - * kexec_image_info - Print received image details - */ -static void -kexec_image_info(const struct kimage *image) -{ - unsigned long i; - - pr_debug("Kexec image info:\n"); - pr_debug("\ttype: %d\n", image->type); - pr_debug("\tstart: %lx\n", image->start); - pr_debug("\thead: %lx\n", image->head); - pr_debug("\tnr_segments: %lu\n", image->nr_segments); - - for (i = 0; i < image->nr_segments; i++) { - pr_debug("\t segment[%lu]: %016lx - %016lx", i, - image->segment[i].mem, - image->segment[i].mem + image->segment[i].memsz); - pr_debug("\t\t0x%lx bytes, %lu pages\n", - (unsigned long) image->segment[i].memsz, - (unsigned long) image->segment[i].memsz / PAGE_SIZE); - } -} - /* * machine_kexec_prepare - Initialize kexec * @@ -60,8 +36,6 @@ machine_kexec_prepare(struct kimage *image) unsigned int control_code_buffer_sz = 0; int i = 0; - kexec_image_info(image); - /* Find the Flattened Device Tree and save its physical address */ for (i = 0; i < image->nr_segments; i++) { if (image->segment[i].memsz <= sizeof(fdt))
Replace pr_debug() with the newly added kexec_dprintk() in kexec_file loading related codes. And also remove kexec_image_info() because the content has been printed out in generic code. Signed-off-by: Baoquan He <bhe@redhat.com> --- arch/riscv/kernel/elf_kexec.c | 11 ++++++----- arch/riscv/kernel/machine_kexec.c | 26 -------------------------- 2 files changed, 6 insertions(+), 31 deletions(-)