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

[Rob Herring]

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);
> > >  }
> > >
> > > -
> > > +#ifdef CONFIG_ACPI
> > >  /*
> > >   * VMBUS is an acpi enumerated device. Get the information we
> > >   * need from DSDT.
> > > @@ -2262,6 +2262,7 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
> > >
> > >         return AE_OK;
> > >  }
> > > +#endif
> > >
> > >  static void vmbus_mmio_remove(void)
> > >  {
> > > @@ -2282,7 +2283,7 @@ static void vmbus_mmio_remove(void)
> > >         }
> > >  }
> > >
> > > -static void vmbus_reserve_fb(void)
> > > +static void __maybe_unused vmbus_reserve_fb(void)
> > >  {
> > >         resource_size_t start = 0, size;
> > >         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.

> 
> > 
> > >  static int vmbus_acpi_add(struct platform_device *pdev)
> > >  {
> > >         acpi_status result;
> > > @@ -2496,10 +2498,68 @@ static int vmbus_acpi_add(struct platform_device *pdev)
> > >                 vmbus_mmio_remove();
> > >         return ret_val;
> > >  }
> > > +#else
> > > +
> > > +static int vmbus_device_add(struct platform_device *pdev)
> > > +{
> > > +       struct resource **cur_res = &hyperv_mmio;
> > > +       struct device_node *np;
> > > +       u32 *ranges, len;
> > > +       u64 start;
> > > +       int nr_ranges, child_cells = 2, cur_cell = 0, ret = 0;
> > > +
> > > +       hv_dev = pdev;
> > > +       np = pdev->dev.of_node;
> > > +
> > > +       nr_ranges = device_property_count_u32(&pdev->dev, "ranges");
> > 
> > Parsing ranges yourself is a bad sign. It's a standard property and we
> > have functions which handle it. If those don't work, then something is
> > wrong with your DT or they need to be fixed/expanded.
> 
> I find all the  standard functions which parse "ranges" property are doing
> much more then I need. Our requirement is to only pass the mmio memory range
> and size, I couldn't find any standard API doing this.

You can't just change how standard properties work to suit your needs.

We shouldn't even be having this discussion because we have tools to 
check all this now. dtc does some and dtschema does a lot more.

> I see some of the drivers are using these APIs to parse ranges property hence
> I follwed those examples. I will be happy to improve it if I get any better
> alternative.

You can always find bad examples to follow...

> > > +       if (nr_ranges < 0)
> > > +               return nr_ranges;
> > > +       ranges = kcalloc(nr_ranges, sizeof(u32), GFP_KERNEL);
> > > +       if (!ranges)
> > > +               return -ENOMEM;
> > > +
> > > +       if (device_property_read_u32_array(&pdev->dev, "ranges", ranges, nr_ranges)) {
> > > +               ret =  -EINVAL;
> > > +               goto free_ranges;
> > > +       }
> > > +
> > > +       while (cur_cell < nr_ranges) {
> > > +               struct resource *res;
> > > +
> > > +               /* The first u64 in the ranges description isn't used currently. */
> > > +               cur_cell = cur_cell + child_cells;
> > > +               start = ranges[cur_cell++];
> > > +               start = (start << 32) | ranges[cur_cell++];
> > > +               len = ranges[cur_cell++];
> > 
> > To expand my last point, the format of ranges is <child_addr
> > parent_addr length>. That's not what your 'ranges' has. You've also
> > just ignored '#address-cells' and '#size-cells'.
> 
> Got it. However I need to check if there is any standard API which can
> give me these values, otherwise I may have to parse these as well :(

for_each_of_range()

That is not how linux works. When the core code doesn't do what you 
want, you adapt it to your needs. You don't work around it. Read 
this[1].

Rob

[1] https://lwn.net/Articles/443531/