On Fri, Feb 10, 2017 at 04:07:38PM -0600, Eric DeVolder wrote:
> From: Daniel Kiper <daniel.kiper at oracle.com>
>
> Implement get_crash_kernel_load_range() in support of
> print crash kernel region size option.
>
> Signed-off-by: Daniel Kiper <daniel.kiper at oracle.com>
> Signed-off-by: Eric DeVolder <eric.devolder at oracle.com>
> ---
> kexec/arch/ppc/crashdump-powerpc.c | 22 +++++++++++++++++++++-
> kexec/arch/ppc/kexec-ppc.c | 27 +++++++++++++++++++++++++++
> kexec/arch/ppc/kexec-ppc.h | 1 +
> 3 files changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/kexec/arch/ppc/crashdump-powerpc.c b/kexec/arch/ppc/crashdump-powerpc.c
> index 3dc35eb..fc5d70c 100644
> --- a/kexec/arch/ppc/crashdump-powerpc.c
> +++ b/kexec/arch/ppc/crashdump-powerpc.c
> @@ -397,6 +397,27 @@ void add_usable_mem_rgns(unsigned long long base, unsigned long long size)
> usablemem_rgns.size, base, size);
> }
>
> +int get_crash_kernel_load_range(uint64_t *start, uint64_t *end)
> +{
> + unsigned long long value;
> + int ret = 0;
> +
> + if (get_devtree_value("/proc/device-tree/chosen/linux,crashkernel-base", &value))
Could not you define this path as a constant somewhere? It is used in a
few places, so, it would be nice.
> + *start = value;
> + else {
> + *start = 0;
> + ret = -1;
> + }
I would add empty line here.
> + if (get_devtree_value("/proc/device-tree/chosen/linux,crashkernel-size", &value))
Ditto.
> + *end = *start + value - 1;
> + else {
> + *end = 0;
> + ret = -1;
> + }
> +
> + return ret;
> +}
> +
> int is_crashkernel_mem_reserved(void)
> {
> int fd;
> @@ -407,4 +428,3 @@ int is_crashkernel_mem_reserved(void)
> close(fd);
> return 1;
> }
> -
Nice but this cleanup should be a separate patch if you wish.
Otherwise it is a bit confusing.
> diff --git a/kexec/arch/ppc/kexec-ppc.c b/kexec/arch/ppc/kexec-ppc.c
> index d046110..d9cdb9c 100644
> --- a/kexec/arch/ppc/kexec-ppc.c
> +++ b/kexec/arch/ppc/kexec-ppc.c
> @@ -423,6 +423,33 @@ err_out:
> return -1;
> }
>
> +int get_devtree_value(const char *fname, unsigned long long *pvalue)
> +{
> + /* Return 1 if fname/value valid, 0 otherwise */
Most if not all functions in kexec/arch/ppc/kexec-ppc.c return -1 in case
of error and 0 if everything went OK. Please follow this convention.
> + FILE *file;
> + char buf[MAXBYTES];
> + int ret = 1, n = -1;
> + unsigned long long value = 0;
> +
> + if ((file = fopen(fname, "r"))) {
> + n = fread(buf, 1, MAXBYTES, file);
> + fclose(file);
> + }
> +
> + if (n == sizeof(uint32_t))
> + value = ((uint32_t *)buf)[0];
I would prefer *((uint32_t *)buf) but as I can see this is a convention
in this file, so, let's leave it as is.
> + else if (n == sizeof(uint64_t)) {
> + value = ((uint64_t *)buf)[0];
Ditto.
> + else {
> + fprintf(stderr, "%s node has invalid size: %d\n", fname, n);
> + ret = 0;
> + }
> +
> + if (pvalue)
> + *pvalue = value;
Add empty line here.
> + return ret;
> +}
> +
> /* Get devtree details and create exclude_range array
> * Also create usablemem_ranges for KEXEC_ON_CRASH
> */
> diff --git a/kexec/arch/ppc/kexec-ppc.h b/kexec/arch/ppc/kexec-ppc.h
> index 904cf48..0e52d18 100644
> --- a/kexec/arch/ppc/kexec-ppc.h
> +++ b/kexec/arch/ppc/kexec-ppc.h
> @@ -75,6 +75,7 @@ extern unsigned long dt_address_cells, dt_size_cells;
> extern int init_memory_region_info(void);
> extern int read_memory_region_limits(int fd, unsigned long long *start,
> unsigned long long *end);
> +extern int get_devtree_value (const char *fname, unsigned long long *pvalue);
Remove extra space between function name and parentheses.
Daniel
