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

From: Rob Herring
Date: Tue Jan 31 2023 - 15:13:14 EST


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

> 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.

> + 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'.

> +
> + res = kzalloc(sizeof(*res), GFP_ATOMIC);
> + if (!res) {
> + ret = -ENOMEM;
> + goto free_ranges;
> + }
> +
> + res->name = "hyperv mmio";
> + res->flags = IORESOURCE_MEM | IORESOURCE_MEM_64;
> + res->start = start;
> + res->end = start + len;
> +
> + *cur_res = res;
> + cur_res = &res->sibling;
> + }
> +
> +free_ranges:
> + kfree(ranges);
> + return ret;
> +}
> +#endif
>
> static int vmbus_platform_driver_probe(struct platform_device *pdev)
> {
> +#ifdef CONFIG_ACPI
> return vmbus_acpi_add(pdev);
> +#else
> + return vmbus_device_add(pdev);
> +#endif
> }
>
> static int vmbus_platform_driver_remove(struct platform_device *pdev)
> @@ -2645,6 +2705,16 @@ static int vmbus_bus_resume(struct device *dev)
> #define vmbus_bus_resume NULL
> #endif /* CONFIG_PM_SLEEP */
>
> +static const struct of_device_id vmbus_of_match[] = {
> + {
> + .compatible = "msft,vmbus",
> + },
> + {
> + /* sentinel */
> + },
> +};
> +MODULE_DEVICE_TABLE(of, vmbus_of_match);
> +
> static const struct acpi_device_id vmbus_acpi_device_ids[] = {
> {"VMBUS", 0},
> {"VMBus", 0},
> @@ -2679,6 +2749,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.25.1
>