[Skiboot] [PATCH v9 08/22] core/fdt: OPAL API opal_get_device_tree()
Daniel Axtens
dja at axtens.net
Tue Dec 1 09:46:50 AEDT 2015
Hi Gavin,
> This implements OPAL API opal_get_device_tree(), which will be used
> to retrieve the device sub-tree introduced by newly hot plugged PCI
> devices.
Could you please tell us a bit more about the semantics of the API in
the commit message?
Could you please also explain what changes it requires in the functions?
So for example you've added an 'inc' parameter to flatten_dt_node. It's
not clear what 'inc' means: I think it means an incremental change, but
it would be nice to have that spelled out.
There are some more comments in line.
>
> Signed-off-by: Gavin Shan <gwshan at linux.vnet.ibm.com>
> ---
> core/fdt.c | 109 ++++++++++++++++++++++++++++++++++-------------------
> include/opal-api.h | 3 +-
> 2 files changed, 73 insertions(+), 39 deletions(-)
>
> diff --git a/core/fdt.c b/core/fdt.c
> index a9392b1..3a85618 100644
> --- a/core/fdt.c
> +++ b/core/fdt.c
> @@ -126,19 +126,23 @@ static void flatten_dt_properties(void *fdt, const struct dt_node *dn)
> }
> }
>
> -static void flatten_dt_node(void *fdt, const struct dt_node *root)
> +static void flatten_dt_node(void *fdt, const struct dt_node *root, bool inc)
> {
> const struct dt_node *i;
>
> + if (inc) {
> #ifdef DEBUG_FDT
> - printf("FDT: node: %s\n", root->name);
> + printf("FDT: node: %s\n", root->name);
> #endif
> - flatten_dt_properties(fdt, root);
> - list_for_each(&root->children, i, list) {
> - dt_begin_node(fdt, i);
> - flatten_dt_node(fdt, i);
> - dt_end_node(fdt);
> + dt_begin_node(fdt, root);
> + flatten_dt_properties(fdt, root);
> }
> +
> + list_for_each(&root->children, i, list)
> + flatten_dt_node(fdt, i, true);
OK, so I really don't think I fully understand `inc'. What exactly does
it mean? I thought it meant incremental, but then passing down true here
unconditionally is confusing me.
> +
> + if (inc)
> + dt_end_node(fdt);
> }
>
> static void create_dtb_reservemap(void *fdt, const struct dt_node *root)
> @@ -163,51 +167,80 @@ static void create_dtb_reservemap(void *fdt, const struct dt_node *root)
> save_err(fdt_finish_reservemap(fdt));
> }
>
> -void *create_dtb(const struct dt_node *root)
> +static int __create_dtb(void *fdt, size_t len,
> + const struct dt_node *root,
> + bool inc)
> {
> - void *fdt = NULL;
> - size_t len = DEVICE_TREE_MAX_SIZE;
> uint32_t old_last_phandle = last_phandle;
>
> - do {
> - if (fdt)
> - free(fdt);
> + fdt_create(fdt, len);
> +
> + if (root == dt_root && inc)
> + create_dtb_reservemap(fdt, root);
> +
> + /* Unflatten our live tree */
> + flatten_dt_node(fdt, root, inc);
> +
> + save_err(fdt_finish(fdt));
> + if (fdt_error) {
> last_phandle = old_last_phandle;
> - fdt_error = 0;
> - fdt = malloc(len);
> - if (!fdt) {
> - prerror("dtb: could not malloc %lu\n", (long)len);
> - return NULL;
> - }
> + prerror("dtb: error %s\n", fdt_strerror(fdt_error));
> + return fdt_error;
> + }
> +
> +#ifdef DEBUG_FDT
> + dump_fdt(fdt);
> +#endif
> + return 0;
> +}
>
> - fdt_create(fdt, len);
> +static int64_t opal_get_device_tree(uint32_t phandle,
> + uint64_t buf,
> + uint64_t len)
> +{
> + struct dt_node *root;
> + void *fdt = (void *)buf;
> + int ret;
>
> - create_dtb_reservemap(fdt, root);
> + if (!fdt || !len)
> + return OPAL_PARAMETER;
>
> - /* Open root node */
> - dt_begin_node(fdt, root);
> + root = dt_find_by_phandle(dt_root, phandle);
> + if (!root)
> + return OPAL_CLOSED;
The use of OPAL_CLOSED caught my eye here. If I understand correctly,
this is returned if you cannot find the device pointed to by phandle in
skiboot's device tree.
It looks like OPAL_CLOSED is used elsewhere for:
- console devices that can be closed
- PHB slots that are empty
Is there a reason you picked OPAL_CLOSED over, e.g. OPAL_PARAMETER?
As I understand it providing a phandle that is missing is providing an
invalid parameter, but I might not understand the semantics fully.
>
> - /* Unflatten our live tree */
> - flatten_dt_node(fdt, root);
> + ret = __create_dtb(fdt, len, root, false);
> + if (ret == -FDT_ERR_NOSPACE)
> + return OPAL_NO_MEM;
> + else if (ret)
> + return OPAL_EMPTY;
>
> - /* Close root node */
> - dt_end_node(fdt);
> + return OPAL_SUCCESS;
> +}
> +opal_call(OPAL_GET_DEVICE_TREE, opal_get_device_tree, 3);
>
> - save_err(fdt_finish(fdt));
> +void *create_dtb(const struct dt_node *root)
> +{
> + void *fdt = NULL;
> + size_t len = DEVICE_TREE_MAX_SIZE;
> + int ret;
>
> - if (!fdt_error)
> + do {
> + fdt = malloc(len);
> + if (!fdt) {
> + prerror("dtb: cannot allocate FDT blob (%lu bytes)\n",
> + (long)len);
> break;
> + }
>
> - len *= 2;
> - } while (fdt_error == -FDT_ERR_NOSPACE);
> + ret = __create_dtb(fdt, len, root, true);
> + if (ret) {
> + free(fdt);
> + fdt = NULL;
> + }
>
> -#ifdef DEBUG_FDT
> - dump_fdt(fdt);
> -#endif
> + len *= 2;
> + } while (ret == -FDT_ERR_NOSPACE);
>
I think this is trying to create the smallest fdt blob that:
- is a power of 2 and
- fits the entire fdt.
Is that right?
I understand you're not changing the algorithm, I'm just trying to make
sense of it to make sure its functionality is preserved.
Regards,
Daniel
> - if (fdt_error) {
> - prerror("dtb: error %s\n", fdt_strerror(fdt_error));
> - return NULL;
> - }
> return fdt;
> }
> diff --git a/include/opal-api.h b/include/opal-api.h
> index 7a11fe8..fff9148 100644
> --- a/include/opal-api.h
> +++ b/include/opal-api.h
> @@ -162,7 +162,8 @@
> #define OPAL_LEDS_GET_INDICATOR 114
> #define OPAL_LEDS_SET_INDICATOR 115
> #define OPAL_CEC_REBOOT2 116
> -#define OPAL_LAST 116
> +#define OPAL_GET_DEVICE_TREE 117
> +#define OPAL_LAST 117
>
> /* Device tree flags */
>
> --
> 2.1.0
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 859 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/skiboot/attachments/20151201/b2ab7171/attachment-0001.sig>
More information about the Skiboot
mailing list