Re: [PATCH v4 02/14] xen/asm-generic: introduce generic device.h

From: Shawn Anastasio <sanastasio@raptorengineering.com>
To: Oleksii <oleksii.kurochko@gmail.com>, Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	Michal Orzel <michal.orzel@amd.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>, Wei Liu <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v4 02/14] xen/asm-generic: introduce generic device.h
Date: Tue, 28 Nov 2023 15:36:01 -0600	[thread overview]
Message-ID: <68213190-4d6f-4d5a-9807-7e2cabb80ed5@raptorengineering.com> (raw)
In-Reply-To: <0e948dcb22fe553053431083f19f058245016e41.camel@gmail.com>

Hi Oleksii,

On 11/27/23 1:46 PM, Oleksii wrote:
> On Mon, 2023-11-27 at 15:31 +0100, Jan Beulich wrote:
>> On 27.11.2023 15:13, Oleksii Kurochko wrote:
>>> --- a/xen/arch/ppc/include/asm/irq.h
>>> +++ b/xen/arch/ppc/include/asm/irq.h
>>> @@ -3,7 +3,9 @@
>>>  #define __ASM_PPC_IRQ_H__
>>>  
>>>  #include <xen/lib.h>
>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>>  #include <xen/device_tree.h>
>>> +#endif
>>>  #include <public/device_tree_defs.h>
>>
>> Why would this #ifdef not cover the public header as well? (Otherwise
>> I'd
>> be inclined to ask that the conditional be moved inside that header.)
> In that header is defined only consts without additional header
> inclusion. At that moment it looked to me pretty save to ifdef only
> xen/device_tree.h but you are right we can move incluion of the public
> header inside #ifdef OR just remove as xen/device_tree.h already
> includes it.
>

Removing the include of <xen/device_tree.h> from ppc's asm/irq.h breaks
the build, because of platform_get_irq's usage of the `struct
dt_device_node` type:

  CC      common/cpu.o
In file included from ./include/xen/irq.h:80,
                 from ./include/xen/pci.h:13,
                 from ./include/xen/iommu.h:25,
                 from ./include/xen/sched.h:12,
                 from ./include/xen/event.h:12,
                 from common/cpu.c:3:
./arch/ppc/include/asm/irq.h:28:49: error: ‘struct dt_device_node’
declared inside parameter list will not be visible outside of this
definition or declaration [-Werror]
   28 | static inline int platform_get_irq(const struct dt_device_node
*device, int index)

I wouldn't be opposed to a forward declaration of that type, but as I
indicated in my other reply to this patch, I'd like to avoid the
addition of these essentially always-dead CONFIG_HAS_DEVICE_TREE checks
to PPC code anyways, so dropping this hunk entirely from the patch might
be the way forward.

> ~ Oleksii

Thanks,
Shawn