Re: [PATCH v2 6/6] Driver: VMBus: Add device tree support

[Rob Herring]

On Fri, Feb 3, 2023 at 11:36 AM Saurabh Singh Sengar
<ssengar@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Feb 01, 2023 at 11:46:38AM -0600, Rob Herring wrote:
> > On Wed, Feb 01, 2023 at 08:51:33AM -0800, Saurabh Singh Sengar wrote:
> > > On Tue, Jan 31, 2023 at 02:12:53PM -0600, Rob Herring wrote:
> > > > On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar
> > > > <ssengar@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > Update the driver to support device tree boot as well along with ACPI.
> > > > > At present the device tree parsing only provides the mmio region info
> > > > > and is not the exact copy of ACPI parsing. This is sufficient to cater
> > > > > all the current device tree usecases for VMBus.
> > > > >
> > > > > Signed-off-by: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx>
> > > > > ---
> > > > >  drivers/hv/vmbus_drv.c | 75 ++++++++++++++++++++++++++++++++++++++++--
> > > > >  1 file changed, 73 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > > > > index 49030e756b9f..1741f1348f9f 100644
> > > > > --- a/drivers/hv/vmbus_drv.c
> > > > > +++ b/drivers/hv/vmbus_drv.c
> > > > > @@ -2152,7 +2152,7 @@ void vmbus_device_unregister(struct hv_device *device_obj)
> > > > >         device_unregister(&device_obj->device);
> > > > >  }
> (...)
> > > > >         struct pci_dev *pdev;
> > > > > @@ -2442,6 +2443,7 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size)
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(vmbus_free_mmio);
> > > > >
> > > > > +#ifdef CONFIG_ACPI
> > > >
> > > > It's better to put C 'if (!IS_ENABLED(CONFIG_ACPI)' code in the
> > >
> > > I wanted to have separate function for ACPI and device tree flow, which
> > > can be easily maintained with #ifdef. Please let me know if its fine.
> >
> > Yes, you can have separate functions:
> >
> > static int vmbus_acpi_add(struct platform_device *pdev)
> > {
> >       if (!IS_ENABLED(CONFIG_ACPI))
> >               return -ENODEV;
> >
> >       ...
> > }
> >
> > The compiler will throw away the function in the end if CONFIG_ACPI is
> > not enabled.
> >
> > That is easier for us to maintain because it reduces the combinations to
> > build.
> >
>
> I tried removing #ifdef CONFIG_ACPI and use C's if(!IS_ENABLED(CONFIG_ACPI)) but looks
> compiler is not optimizing out the rest of function, it still throwing errors
> for acpi functions. This doesn't look 1:1 replacement to me.
> Please let me know if I have missunderstood any of your suggestion.
>
> drivers/hv/vmbus_drv.c:2175:8: error: implicit declaration of function ‘acpi_dev_resource_interrupt’ [-Werror=implicit-function-

That's a failure of the ACPI headers not having empty function
declarations. The DT functions do...

Also, this is just a broken assumption:

#ifdef CONFIG_ACPI

#else
// Assume DT
#endif

Both ACPI and DT can be enabled at the same time. They may be mutually
exclusive for a platform, but not the kernel. For distro kernels, both
will be enabled typically if the arch supports both. On arm64, DT is
never disabled because the boot interface is always DT.

Furthermore, this makes compile testing your code difficult. The arm64
defconfig, allmodconfig and allyesconfig all will not build the DT
code. The same for x86. This means all the CI builds that happen can't
build test this.

Rob