Re: [XEN][PATCH v7 13/19] asm/smp.h: Fix circular dependency for device_tree.h and rwlock.h

[Julien Grall <julien@xxxxxxx>]

Hi,

On 02/06/2023 01:48, Vikram Garhwal wrote:
> Dynamic programming ops will modify the dt_host and there might be other
> function which are browsing the dt_host at the same time. To avoid the race
> conditions, adding rwlock for browsing the dt_host.
Reading this sentence, it sounds like you are adding the rwlock in this 
patch. However, this is not the case. Also, the rwlock is not only for 
browsing but also add new node. So how about ", we will need to add a 
rwlock to protect access to the dt_host".
> But adding rwlock in
> device_tree.h causes following circular dependency:
>      device_tree.h->rwlock.h->smp.h->asm/smp.h->device_tree.h
>
> To fix this, removed the "#include <xen/device_tree.h> and forward declared
> "struct dt_device_node".
>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
> Reviewed-by: Henry Wang <Henry.Wang@xxxxxxx>
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
> ---
>   xen/arch/arm/include/asm/smp.h | 3 ++-
>   xen/arch/arm/smpboot.c         | 1 +
>   2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/include/asm/smp.h b/xen/arch/arm/include/asm/smp.h
> index a37ca55bff..b12949ba8a 100644
> --- a/xen/arch/arm/include/asm/smp.h
> +++ b/xen/arch/arm/include/asm/smp.h
> @@ -3,13 +3,14 @@
>   
>   #ifndef __ASSEMBLY__
>   #include <xen/cpumask.h>
> -#include <xen/device_tree.h>
>   #include <asm/current.h>
>   #endif
>   
>   DECLARE_PER_CPU(cpumask_var_t, cpu_sibling_mask);
>   DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask);
>   
> +struct dt_device_node;
Can you add the declaration just above arch_cpu_init()? This will make 
clearer why the forward declaration is necessary.
> +
>   #define cpu_is_offline(cpu) unlikely(!cpu_online(cpu))
>   
>   #define smp_processor_id() get_processor_id()
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index e107b86b7b..eeb76cd551 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -10,6 +10,7 @@
>   #include <xen/cpu.h>
>   #include <xen/cpumask.h>
>   #include <xen/delay.h>
> +#include <xen/device_tree.h>
>   #include <xen/domain_page.h>
>   #include <xen/errno.h>
>   #include <xen/init.h>
Cheers,

--
Julien Grall