Hari Bathini <hbath...@linux.ibm.com> writes:
> Kdump kernel, used for capturing the kernel core image, is supposed
> to use only specific memory regions to avoid corrupting the image to
> be captured. The regions are crashkernel range - the memory reserved
> explicitly for kdump kernel, memory used for the tce-table, the OPAL
> region and RTAS region as applicable. Restrict kdump kernel memory
> to use only these regions by setting up usable-memory DT property.
> Also, tell the kdump kernel to run at the loaded address by setting
> the magic word at 0x5c.
>
> Signed-off-by: Hari Bathini <hbath...@linux.ibm.com>
> Tested-by: Pingfan Liu <pi...@redhat.com>
> ---
>
> v3 -> v4:
> * Updated get_node_path() to be an iterative function instead of a
> recursive one.
> * Added comment explaining why low memory is added to kdump kernel's
> usable memory ranges though it doesn't fall in crashkernel region.
> * For correctness, added fdt_add_mem_rsv() for the low memory being
> added to kdump kernel's usable memory ranges.
Good idea.
> * Fixed prop pointer update in add_usable_mem_property() and changed
> duple to tuple as suggested by Thiago.
<snip>
> +/**
> + * get_node_pathlen - Get the full path length of the given node.
> + * @dn: Node.
> + *
> + * Also, counts '/' at the end of the path.
> + * For example, /memory@0 will be "/memory@0/\0" => 11 bytes.
Wouldn't this function return 10 in the case of /memory@0?
Are you saying that it should count the \0 at the end too? it's not
doing that, AFAICS.
> + *
> + * Returns the string length of the node's full path.
> + */
Maybe it's me (by analogy with strlen()), but I would expect "string
length" to not include the terminating \0. I suggest renaming the
function to something like get_node_path_size() and do s/length/size/ in
the comment above if it's supposed to count the terminating \0.
> +static int get_node_pathlen(struct device_node *dn)
> +{
> + int len = 0;
> +
> + if (!dn)
> + return 0;
> +
> + while (dn) {
> + len += strlen(dn->full_name) + 1;
> + dn = dn->parent;
> + }
> +
> + return len + 1;
> +}
> +
> +/**
> + * get_node_path - Get the full path of the given node.
> + * @node: Device node.
> + *
> + * Allocates buffer for node path. The caller must free the buffer
> + * after use.
> + *
> + * Returns buffer with path on success, NULL otherwise.
> + */
> +static char *get_node_path(struct device_node *node)
> +{
> + struct device_node *dn;
> + int len, idx, nlen;
> + char *path = NULL;
> + char end_char;
> +
> + if (!node)
> + goto err;
> +
> + /*
> + * Get the path length first and use it to iteratively build the path
> + * from node to root.
> + */
> + len = get_node_pathlen(node);
> +
> + /* Allocate memory for node path */
> + path = kzalloc(ALIGN(len, 8), GFP_KERNEL);
> + if (!path)
> + goto err;
> +
> + /*
> + * Iteratively update path from node to root by decrementing
> + * index appropriately.
> + *
> + * Also, add %NUL at the end of node & '/' at the end of all its
> + * parent nodes.
> + */
> + dn = node;
> + path[0] = '/';
> + idx = len - 1;
Here, idx is pointing to the supposed '/' at the end of the node
path ...
> + end_char = '\0';
> + while (dn->parent) {
> + path[--idx] = end_char;
.. and in the first ireation, this is writing '\0' at a place which will be
overwritten by the memcpy() below with the last character of
dn->full_name. You need to start idx with len, not len - 1.
> + end_char = '/';
> +
> + nlen = strlen(dn->full_name);
> + idx -= nlen;
> + memcpy(path + idx, dn->full_name, nlen);
> +
> + dn = dn->parent;
> + }
> +
> + return path;
> +err:
> + kfree(path);
> + return NULL;
> +}
--
Thiago Jung Bauermann
IBM Linux Technology Center
