Bhupesh,
Thank you for submitting a patch. One comment:
On Fri, Nov 30, 2018 at 11:02:45AM +0530, Bhupesh Sharma wrote:
> While creating the 2nd dtb to be passed to the secondary kernel
> for arm64, we need to handle the return values from helper fdt
> functions properly, to make sure that we can handle various command-line
> options being passed to 'kexec' both for kexec load and kdump
> purposes.
>
> This will provide proper error reporting to the calling
> function in case something goes wrong.
>
> Without this patch, we get misleading error FDT_ERR_BADOFFSET reported
> when we pass a .dtb to 'kexec -p' using '--dtb' option, which doesn't
> contain the '/chosen' node (for e.g. the rockchip sapphire board
> dtb -> rk3399-sapphire.dtb).
>
> # kexec -d -p Image --reuse-cmdline --dtb rk3399-sapphire.dtb
> <..snip..>
> load_crashdump_segments: elfcorehdr 0xbfff0000-0xbfff33ff
> get_cells_size: #address-cells:2 #size-cells:2
> cells_size_fitted: bfff0000-bfff33ff
> cells_size_fitted: 8ee00000-bfffffff
> setup_2nd_dtb: no kaslr-seed found
> setup_2nd_dtb: fdt_setprop failed: FDT_ERR_BADOFFSET
> kexec: setup_2nd_dtb failed.
> kexec: load failed.
>
> Now after the fix, we get the correct error FDT_ERR_NOTFOUND reported
> to the calling function:
>
> # kexec -d -l Image --append 'debug' --dtb rk3399-sapphire.dtb
> <..snip..>
> load_crashdump_segments: elfcorehdr 0xbfff0000-0xbfff33ff
> get_cells_size: #address-cells:2 #size-cells:2
> cells_size_fitted: bfff0000-bfff33ff
> cells_size_fitted: 8ee00000-bfffffff
> setup_2nd_dtb: /chosen node not found - can't create dtb for dump kernel: FDT_ERR_NOTFOUND
> kexec: setup_2nd_dtb failed.
> kexec: load failed.
>
>
> Cc: Simon Horman <horms@xxxxxxxxxxxx>
> Cc: AKASHI Takahiro <takahiro.akashi@xxxxxxxxxx>
> Signed-off-by: Bhupesh Sharma <bhsharma@xxxxxxxxxx>
> ---
> kexec/arch/arm64/kexec-arm64.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/kexec/arch/arm64/kexec-arm64.c b/kexec/arch/arm64/kexec-arm64.c
> index f4913b2e9480..560d83455f95 100644
> --- a/kexec/arch/arm64/kexec-arm64.c
> +++ b/kexec/arch/arm64/kexec-arm64.c
> @@ -515,6 +515,11 @@ static int setup_2nd_dtb(struct dtb *dtb, char *command_line, int on_crash)
> }
>
> nodeoffset = fdt_path_offset(new_buf, "/chosen");
> + if (nodeoffset < 0) {
> + result = nodeoffset;
> + goto on_error;
> + }
> +
I think that the issue should be fixed much earlier:
| /* fixup 'kaslr-seed' with a random value, if supported */
| nodeoffset = fdt_path_offset(new_buf, "/chosen");
| prop = fdt_getprop_w(new_buf, nodeoffset,
| "kaslr-seed", &len);
When we fail to locate "/chosen" here, we'd be better off to create it
(if necessary). So, *logically*, we won't have to worry about it later on.
-Takahiro Akashi
> result = fdt_setprop_inplace(new_buf,
> nodeoffset, "kaslr-seed",
> &fdt_val64, sizeof(fdt_val64));
> @@ -529,6 +534,14 @@ static int setup_2nd_dtb(struct dtb *dtb, char *command_line, int on_crash)
> if (on_crash) {
> /* add linux,elfcorehdr */
> nodeoffset = fdt_path_offset(new_buf, "/chosen");
> + if (nodeoffset < 0) {
> + result = nodeoffset;
> + dbgprintf("%s: /chosen node not found - can't create "
> + "dtb for dump kernel: %s\n", __func__,
> + fdt_strerror(result));
> + goto on_error;
> + }
> +
> result = fdt_setprop_range(new_buf, nodeoffset,
> PROP_ELFCOREHDR, &elfcorehdr_mem,
> address_cells, size_cells);
> @@ -541,6 +554,14 @@ static int setup_2nd_dtb(struct dtb *dtb, char *command_line, int on_crash)
>
> /* add linux,usable-memory-range */
> nodeoffset = fdt_path_offset(new_buf, "/chosen");
> + if (nodeoffset < 0) {
> + result = nodeoffset;
> + dbgprintf("%s: /chosen node not found - can't create "
> + "dtb for dump kernel: %s\n", __func__,
> + fdt_strerror(result));
> + goto on_error;
> + }
> +
> result = fdt_setprop_range(new_buf, nodeoffset,
> PROP_USABLE_MEM_RANGE, &crash_reserved_mem,
> address_cells, size_cells);
> --
> 2.7.4
>
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
