Re: [PATCH v3] xen: Remove dependency between pciback and privcmd

[Juergen Gross <jgross@xxxxxxxx>]

On 11.10.24 12:10, Jan Beulich wrote:
> On 11.10.2024 11:33, Chen, Jiqian wrote:
> > On 2024/10/11 17:20, Chen, Jiqian wrote:
> > > On 2024/10/11 16:54, Jan Beulich wrote:
> > > > On 11.10.2024 05:42, Jiqian Chen wrote:
> > > > > @@ -1757,11 +1756,19 @@ static int __init xen_pcibk_init(void)
> > > > >                 bus_register_notifier(&pci_bus_type, &pci_stub_nb);
> > > > >   #endif
> > > > >   
> > > > > +#ifdef CONFIG_XEN_ACPI
> > > > > +       xen_acpi_register_get_gsi_func(pcistub_get_gsi_from_sbdf);
> > > > > +#endif
> > > > > +
> > > > >         return err;
> > > > >   }
> > > > >   
> > > > >   static void __exit xen_pcibk_cleanup(void)
> > > > >   {
> > > > > +#ifdef CONFIG_XEN_ACPI
> > > > > +       xen_acpi_register_get_gsi_func(NULL);
> > > > > +#endif
> > > > Just wondering - instead of these two #ifdef-s, ...
> > > >
> > > > > --- a/include/xen/acpi.h
> > > > > +++ b/include/xen/acpi.h
> > > > > @@ -91,13 +91,9 @@ static inline int xen_acpi_get_gsi_info(struct pci_dev *dev,
> > > > >   }
> > > > >   #endif
> > > > >   
> > > > > -#ifdef CONFIG_XEN_PCI_STUB
> > > > > -int pcistub_get_gsi_from_sbdf(unsigned int sbdf);
> > > > > -#else
> > > > > -static inline int pcistub_get_gsi_from_sbdf(unsigned int sbdf)
> > > > > -{
> > > > > -       return -1;
> > > > > -}
> > > > > -#endif
> > > > > +typedef int (*get_gsi_from_sbdf_t)(u32 sbdf);
> > > > > +
> > > > > +void xen_acpi_register_get_gsi_func(get_gsi_from_sbdf_t func);
> > > > > +int xen_acpi_get_gsi_from_sbdf(u32 sbdf);
> > > > ... wouldn't a static inline stub (for the !XEN_ACPI case) aid overall 
> > > > readability?
> > > I'm not sure if other files do this. But for me, it feels a little strange to use 
> > > "#ifdef CONFIG_XEN_ACPI #else" in apci.h, like self-containment.
> > > And "#include apci.h" in pic_stub.c is also wraped with CONFIG_XEN_ACPI.
> > OK, I saw other files also do this.
> > If you insist, I will make modifications in the next version.
> Well, I'm not a maintainer of this code, so I can't very well insist.
In this case I'm in favor of having the #ifdefs at the calling site.

The amount of code isn't larger this way, while it is more clear when
reading the code that the calls are only done in the CONFIG_XEN_ACPI
case.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature