Not a functional change.

Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
---
 xen/arch/x86/include/asm/pci.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
index bed99437cc..2e67cba8b9 100644
--- a/xen/arch/x86/include/asm/pci.h
+++ b/xen/arch/x86/include/asm/pci.h
@@ -13,6 +13,8 @@
                         || (id) == 0x01128086 || (id) == 0x01228086 \
                         || (id) == 0x010A8086 )
 
+struct pci_dev;
+
 struct arch_pci_dev {
     vmask_t used_vectors;
     /*
-- 
2.43.0

On 30.05.2025 14:02, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/include/asm/pci.h
> +++ b/xen/arch/x86/include/asm/pci.h
> @@ -13,6 +13,8 @@
>                          || (id) == 0x01128086 || (id) == 0x01228086 \
>                          || (id) == 0x010A8086 )
>  
> +struct pci_dev;
> +
>  struct arch_pci_dev {
>      vmask_t used_vectors;
>      /*

Would it perhaps be better to put this ahead of xen/pci.h's #include, thus
helping all architectures?

Jan

On Mon Jun 2, 2025 at 9:48 AM CEST, Jan Beulich wrote:
> On 30.05.2025 14:02, Alejandro Vallejo wrote:
>> --- a/xen/arch/x86/include/asm/pci.h
>> +++ b/xen/arch/x86/include/asm/pci.h
>> @@ -13,6 +13,8 @@
>>                          || (id) == 0x01128086 || (id) == 0x01228086 \
>>                          || (id) == 0x010A8086 )
>>  
>> +struct pci_dev;
>> +
>>  struct arch_pci_dev {
>>      vmask_t used_vectors;
>>      /*
>
> Would it perhaps be better to put this ahead of xen/pci.h's #include, thus
> helping all architectures?
>
> Jan

You mean include asm/pci.h from xen/pci.h ? Seeing how arch_pci_dev is here it
must already be transitively included. I could replace all asm/pci.h with xen/
pci.h instead, but this seems strictly better so you can include asm/pci.h when
that's all you need.

Or did you mean something else?

Cheers,
Alejandro

On 02.06.2025 16:01, Alejandro Vallejo wrote:
> On Mon Jun 2, 2025 at 9:48 AM CEST, Jan Beulich wrote:
>> On 30.05.2025 14:02, Alejandro Vallejo wrote:
>>> --- a/xen/arch/x86/include/asm/pci.h
>>> +++ b/xen/arch/x86/include/asm/pci.h
>>> @@ -13,6 +13,8 @@
>>>                          || (id) == 0x01128086 || (id) == 0x01228086 \
>>>                          || (id) == 0x010A8086 )
>>>  
>>> +struct pci_dev;
>>> +
>>>  struct arch_pci_dev {
>>>      vmask_t used_vectors;
>>>      /*
>>
>> Would it perhaps be better to put this ahead of xen/pci.h's #include, thus
>> helping all architectures?
> 
> You mean include asm/pci.h from xen/pci.h ? Seeing how arch_pci_dev is here it
> must already be transitively included. I could replace all asm/pci.h with xen/
> pci.h instead, but this seems strictly better so you can include asm/pci.h when
> that's all you need.

Hmm, I didn't take into account that asm/pci.h is indeed being included all on
its own in a few places. IOW your patch is fine by me as-is.

Jan

On Fri, 30 May 2025, Alejandro Vallejo wrote:
> Not a functional change.
> 
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

> ---
>  xen/arch/x86/include/asm/pci.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
> index bed99437cc..2e67cba8b9 100644
> --- a/xen/arch/x86/include/asm/pci.h
> +++ b/xen/arch/x86/include/asm/pci.h
> @@ -13,6 +13,8 @@
>                          || (id) == 0x01128086 || (id) == 0x01228086 \
>                          || (id) == 0x010A8086 )
>  
> +struct pci_dev;
> +
>  struct arch_pci_dev {
>      vmask_t used_vectors;
>      /*
> -- 
> 2.43.0
> 
>

On 2025-05-30 08:02, Alejandro Vallejo wrote:
> Not a functional change.
> 
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>

Some sort of reason would be good in the commit message.

"struct pci_dev is used in function prototypes within the header.  This 
is in preparation for including (transitively) in device tree"?

... I'm guessing that is why.  Stating  it would be better.

With a suitable reason:

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Regards,
Jason

On Fri May 30, 2025 at 11:04 PM CEST, Jason Andryuk wrote:
> On 2025-05-30 08:02, Alejandro Vallejo wrote:
>> Not a functional change.
>> 
>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
>
> Some sort of reason would be good in the commit message.
>
> "struct pci_dev is used in function prototypes within the header.  This 
> is in preparation for including (transitively) in device tree"?

>
> ... I'm guessing that is why.  Stating  it would be better.

Yes, but I'm not sure the second part of that explanation is relevant. Unless
specifically noted in the header, they are meant to stand by themselves when
possible and not require preinclusion of something else (in this case, xen/pci.h).

This patch would still be relevant (imo) even if I wasn't using the header for
anything.

>
> With a suitable reason:
>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Thanks,

Does this sound reasonable?

  struct pci_dev is used in function prototypes within the header, so it must
  be forward declared for asm/pci.h not to depend on xen/pci.h being included
  first.

>
> Regards,
> Jason

Cheers,
Alejandro

On 2025-06-02 09:45, Alejandro Vallejo wrote:
> On Fri May 30, 2025 at 11:04 PM CEST, Jason Andryuk wrote:
>> On 2025-05-30 08:02, Alejandro Vallejo wrote:
>>> Not a functional change.
>>>
>>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
>>
>> Some sort of reason would be good in the commit message.
>>
>> "struct pci_dev is used in function prototypes within the header.  This
>> is in preparation for including (transitively) in device tree"?
> 
>>
>> ... I'm guessing that is why.  Stating  it would be better.
> 
> Yes, but I'm not sure the second part of that explanation is relevant. Unless
> specifically noted in the header, they are meant to stand by themselves when
> possible and not require preinclusion of something else (in this case, xen/pci.h).
> 
> This patch would still be relevant (imo) even if I wasn't using the header for
> anything.

Yes, this could be made as a totally independent clean up, and that 
would be fine.  If this is a prerequisite for some later change, I think 
it is useful to highlight it.  It would not otherwise be evident when 
looking at the history.  Because I haven't gotten farther into the 
series, I don't know if this patch is an independent cleanup or a 
prerequisite.

>>
>> With a suitable reason:
>>
>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> 
> Thanks,
> 
> Does this sound reasonable?
> 
>    struct pci_dev is used in function prototypes within the header, so it must
>    be forward declared for asm/pci.h not to depend on xen/pci.h being included
>    first.

Sure.  Or just:
struct pci_dev is used in function prototypes within the header, so this 
forward declaration makes it standalone.

My point is: we should explicitly state what the patch is trying to 
achieve.  It helps reviewers and future git history readers.

Regards,
Jason