From: Saurabh Singh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx> Sent: Sunday, March 12, 2023 10:40 AM
>
> On Sun, Mar 12, 2023 at 01:08:02PM +0000, Michael Kelley (LINUX) wrote:
> > From: Saurabh Singh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx> Sent: Thursday, March 9,
> 2023 9:35 PM
> > >
> > > On Thu, Mar 09, 2023 at 09:16:25PM +0000, Wei Liu wrote:
> > > > On Thu, Feb 23, 2023 at 03:29:05AM -0800, Saurabh Sengar wrote:
> >
> > [snip]
> >
> > > > >
> > > > > static int vmbus_platform_driver_probe(struct platform_device *pdev)
> > > > > {
> > > > > +#ifdef CONFIG_ACPI
> > > > > return vmbus_acpi_add(pdev);
> > > > > +#endif
> > > >
> > > > Please use #else here.
> > > >
> > > > > + return vmbus_device_add(pdev);
> > > >
> > > > Is there going to be a configuration that ACPI and OF are available at
> > > > the same time? I don't see they are marked as mutually exclusive in the
> > > > proposed KConfig.
> > >
> > > Initially, the device tree functions was included in "#else" section after
> > > the "#ifdef CONFIG_ACPI" section. However, it was subsequently removed to
> > > increase the coverage for CI builds.
> > >
> > > Ref: https://lkml.org/lkml/2023/2/7/910
> > >
> >
> > I think the point here is that it is possible (and even likely on ARM64?) to
> > build a kernel where CONFIG_ACPI and CONFIG_OF are both "Y". So the
> > code for ACPI and OF is compiled and present in the kernel image. However,
> > for a particular Linux boot on a particular hardware or virtual platform,
> > only one of the two will be enabled. I specifically mention a particular Linux
> > kernel boot because there's a kernel boot line option that can force disabling
> > ACPI. Ideally, the VMBus code should work if both CONFIG_ACPI and
> > CONFIG_OF are enabled in the kernel image, and it would determine at
> > runtime which to use. This approach meets the goals Rob spells out.
> >
> > There's an exported global variable "acpi_disabled" that is set correctly
> > depending on CONFIG_ACPI and the kernel boot line option (and perhaps if
> > ACPI is not detected at runtime during boot -- I didn't check all the details).
> > So the above could be written as:
> >
> > if (!acpi_disabled)
> > return vmbus_acpi_add(pdev);
> > else
> > return vmbus_device_add(pdev);
> >
> > This avoids the weird "two return statements in a row" while preferring
> > ACPI over OF if ACPI is enabled for a particular boot of Linux.
> >
> > I'm not sure if you'll need a stub for vmbus_acpi_add() when CONFIG_ACPI=n.
> > In that case, acpi_disabled is #defined to be 1, so the compiler should just
> > drop the call to vmbus_acpi_add() entirely and no stub will be needed. But
> > you'll need to confirm.
>
> Thanks for suggesting acpi_disabled, definitely this looks better. However,
> we need a dummy stub for vmbus_acpi_add in case of CONFIG_ACPI=n, as compiler
> doesn't take out vmbus_acpi_add reference completely for CONFIG_ACPI=n.
Fair enough. I wasn't sure ....
>
> >
> > Also just confirming, it looks like vmbus_device_add() compiles correctly if
> > CONFIG_OF=n. There are enough stubs in places so that you don't need an
> > #ifdef CONFIG_OF around vmbus_device_add() like is needed for
> > vmbus_acpi_add().
>
> Yes, I tested this scenario.
>
> >
> > > > >
> > > > > +static const __maybe_unused struct of_device_id vmbus_of_match[] = {
> > > > > + {
> > > > > + .compatible = "microsoft,vmbus",
> > > > > + },
> > > > > + {
> > > > > + /* sentinel */
> > > > > + },
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(of, vmbus_of_match);
> > > > > +
> > > > > +#ifdef CONFIG_ACPI
> > > > > static const struct acpi_device_id vmbus_acpi_device_ids[] = {
> > > > > {"VMBUS", 0},
> > > > > {"VMBus", 0},
> > > > > {"", 0},
> > > > > };
> > > > > MODULE_DEVICE_TABLE(acpi, vmbus_acpi_device_ids);
> > > > > +#endif
> >
> > Couldn't the bracketing #ifdef be dropped and add __maybe_unused, just
> > as you've done with vmbus_of_match? ACPI_PTR() is defined to return NULL
> > if CONFIG_ACPI=n, just like with of_match_ptr() and CONFIG_OF.
>
> I kept #ifdef so that all the acpi code is treated equally. However, I am
> fine to use __maybe_unused, will fix this in next version.
OK, I see your point about a different consistency, so this could go either way. :-)
I tend to prefer getting rid of #ifdef's whenever possible. Since there's a valid
argument either way, let's prefer the approach that eliminates the #ifdef.
Michael
>
> Regards,
> Saurabh
>
> >
> > > > >
> > > > > /*
> > > > > * Note: we must use the "no_irq" ops, otherwise hibernation can not work with
> > > > > @@ -2677,6 +2729,7 @@ static struct platform_driver vmbus_platform_driver = {
> > > > > .driver = {
> > > > > .name = "vmbus",
> > > > > .acpi_match_table = ACPI_PTR(vmbus_acpi_device_ids),
> > > > > + .of_match_table = of_match_ptr(vmbus_of_match),
> > > > > .pm = &vmbus_bus_pm,
> > > > > .probe_type = PROBE_FORCE_SYNCHRONOUS,
> > > > > }
> > > > > --
> > > > > 2.34.1
> > > > >
