[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC v2 3/8] xen/arm: Export host device-tree to hypfs



Hi,

On 09/02/2022 18:51, Oleksii Moisieiev wrote:
On Wed, Feb 09, 2022 at 12:17:17PM +0000, Julien Grall wrote:
+static HYPFS_DIR_INIT_FUNC(host_dt_dir, HOST_DT_DIR, &host_dt_dir_funcs);
+
+static int __init host_dtb_export_init(void)
+{
+    ASSERT(dt_host && (dt_host->sibling == NULL));

dt_host can be NULL when booting on ACPI platform. So I think this wants to
be turned to a normal check and return directly.


I will replace if with
if ( !acpi_disabled )
      return -ENODEV;

Also could you explain why you need to check dt_host->sibling?


This is my way to check if dt_host points to the top of the device-tree.
In any case I will replace it with !acpi_disabled as I mentioned
earlier.

dt_host will always points to the root of the host device-tree. I don't
think it is the job of hypfs to enforce it unless you expect the code to be
buggy if this happens. But then I would argue the code should be hardened.


Hi Julien,

Unfortunatelly I can't use acpi_disabled in host_dtb_export_init because
I've already moved host_dtb_export.c to the common folder.

I am sorry, but I don't understand why moving the code to common code prevents you to use !acpi_disabled. Can you clarify?


As for the host->sibling - I took the whole assert:
ASSERT(dt_host && (dt_host->sibling == NULL));
from the prepare_dtb_hwdom function. And this assertion was added by the
commit b8f1c5e7039efbe1103ed3fe4caedf8c34affe13 authored by you.

I am not sure what's your point... Yes I wrote the same ASSERT() 9 years time. But people view evolves over the time.

There are some code I wished I had written differently (How about you? ;)). However, I don't have the time to rewrite everything I ever wrote. That said, I can at least make sure they are not spread.


What do you think if I omit dt_host->sibling check and make it:

if ( !dt_host )
     return -ENODEV;

We used to set dt_host even when booting with ACPI but that shouldn't be the case anymore. So I think this check should be fine.

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.