[PATCH 0/4] Device tree support for Hyper-V VMBus driver

[PATCH 0/4] Device tree support for Hyper-V VMBus driver

[Saurabh Sengar]

This patch set expands the functionality of the VMBus driver by adding
support for device tree on x86/x64 architectures.

The first two patches enable Hyper-V builds for non-ACPI systems, while
the third patch adds device tree support into the VMBus driver, in
addition to its pre-existing support for ACPI. The fourth patch includes
the necessary device tree bindings for the VMBus driver.

Saurabh Sengar (4):
  drivers/clocksource/hyper-v: non ACPI support in hyperv clock
  Drivers: hv: allow non ACPI compilation for
    hv_is_hibernation_supported
  Drivers: hv: vmbus: Device Tree support
  dt-bindings: hv: Add dt-bindings for VMBus

 .../devicetree/bindings/hv/msft,vmbus.yaml         |  34 ++++
 drivers/clocksource/hyperv_timer.c                 |  15 +-
 drivers/hv/hv_common.c                             |   4 +
 drivers/hv/vmbus_drv.c                             | 190 +++++++++++++++++----
 4 files changed, 206 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hv/msft,vmbus.yaml

-- 
1.8.3.1

[PATCH 1/4] drivers/clocksource/hyper-v: non ACPI support in hyperv clock

[Saurabh Sengar]

Add a placeholder function for the hv_setup_stimer0_irq API to accommodate
systems without ACPI support. Since this function is not utilized on
x86/x64 systems and non-ACPI support is only intended for x86/x64 systems,
a placeholder function is sufficient for now and can be improved upon if
necessary in the future

This change will make it easier to add device tree support for VMBus in
subsequent commits.

Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
 drivers/clocksource/hyperv_timer.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
index c0cef92..f32948c 100644
--- a/drivers/clocksource/hyperv_timer.c
+++ b/drivers/clocksource/hyperv_timer.c
@@ -49,7 +49,7 @@
 
 static int stimer0_irq = -1;
 static int stimer0_message_sint;
-static DEFINE_PER_CPU(long, stimer0_evt);
+static __maybe_unused DEFINE_PER_CPU(long, stimer0_evt);
 
 /*
  * Common code for stimer0 interrupts coming via Direct Mode or
@@ -68,7 +68,7 @@ void hv_stimer0_isr(void)
  * stimer0 interrupt handler for architectures that support
  * per-cpu interrupts, which also implies Direct Mode.
  */
-static irqreturn_t hv_stimer0_percpu_isr(int irq, void *dev_id)
+static irqreturn_t __maybe_unused hv_stimer0_percpu_isr(int irq, void *dev_id)
 {
 	hv_stimer0_isr();
 	return IRQ_HANDLED;
@@ -196,6 +196,7 @@ void __weak hv_remove_stimer0_handler(void)
 {
 };
 
+#ifdef CONFIG_ACPI
 /* Called only on architectures with per-cpu IRQs (i.e., not x86/x64) */
 static int hv_setup_stimer0_irq(void)
 {
@@ -230,6 +231,16 @@ static void hv_remove_stimer0_irq(void)
 		stimer0_irq = -1;
 	}
 }
+#else
+static int hv_setup_stimer0_irq(void)
+{
+	return 0;
+}
+
+static void hv_remove_stimer0_irq(void)
+{
+}
+#endif
 
 /* hv_stimer_alloc - Global initialization of the clockevent and stimer0 */
 int hv_stimer_alloc(bool have_percpu_irqs)
-- 
1.8.3.1

[PATCH 2/4] Drivers: hv: allow non ACPI compilation for hv_is_hibernation_supported

[Saurabh Sengar]

acpi_sleep_state_supported API is only define for CONFIG_ACPI flag and
thus it can't be used for non-ACPI builds. Initaly there won't be
hibernate support for non ACPI builds.

This change will help adding device tree support in subsequent commits.

Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
 drivers/hv/hv_common.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
index 52a6f89..370ec20 100644
--- a/drivers/hv/hv_common.c
+++ b/drivers/hv/hv_common.c
@@ -234,7 +234,11 @@ void hv_setup_dma_ops(struct device *dev, bool coherent)
 
 bool hv_is_hibernation_supported(void)
 {
+#ifdef CONFIG_ACPI
 	return !hv_root_partition && acpi_sleep_state_supported(ACPI_STATE_S4);
+#else
+	return false;
+#endif
 }
 EXPORT_SYMBOL_GPL(hv_is_hibernation_supported);
 
-- 
1.8.3.1

[PATCH 3/4] Drivers: hv: vmbus: Device Tree support

[Saurabh Sengar]

Update the driver to use vmbus_root_dev device instead of acpi_device,
which can be assigned to either ACPI or OF device, making VMBus agnostic
to whether the device is using ACPI or device tree.

When built for OF, the driver registers as a platform driver and includes
a 'of' probe function that is alternate to the existing ACPI registration
and probe when built for ACPI. The VMBus driver maintains its registration
as the bus that other VMBus devices plug into, regardless of whether it is
initially probed and initialized via ACPI or OF.

This change also introduce vmbus_remove_mmio function, which helps removing
the duplicate code for mmio cleanup between APIC and OF driver.

Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
 drivers/hv/vmbus_drv.c | 190 ++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 155 insertions(+), 35 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 1901556..894b360 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -11,11 +11,13 @@
 
 #include <linux/init.h>
 #include <linux/module.h>
-#include <linux/device.h>
+#include <linux/platform_device.h>
 #include <linux/interrupt.h>
 #include <linux/sysctl.h>
 #include <linux/slab.h>
 #include <linux/acpi.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/completion.h>
 #include <linux/hyperv.h>
 #include <linux/kernel_stat.h>
@@ -44,14 +46,14 @@ struct vmbus_dynid {
 	struct hv_vmbus_device_id id;
 };
 
-static struct acpi_device  *hv_acpi_dev;
-
 static int hyperv_cpuhp_online;
 
 static void *hv_panic_page;
 
 static long __percpu *vmbus_evt;
 
+static struct device *vmbus_root_dev;
+
 /* Values parsed from ACPI DSDT */
 int vmbus_irq;
 int vmbus_interrupt;
@@ -136,17 +138,13 @@ static int hv_die_panic_notify_crash(struct notifier_block *self,
 	return NOTIFY_DONE;
 }
 
-static const char *fb_mmio_name = "fb_range";
 static struct resource *fb_mmio;
 static struct resource *hyperv_mmio;
 static DEFINE_MUTEX(hyperv_mmio_lock);
 
 static int vmbus_exists(void)
 {
-	if (hv_acpi_dev == NULL)
-		return -ENODEV;
-
-	return 0;
+	return vmbus_root_dev ? 0 : -ENODEV;
 }
 
 static u8 channel_monitor_group(const struct vmbus_channel *channel)
@@ -932,7 +930,7 @@ static int vmbus_dma_configure(struct device *child_device)
 	 * On x86/x64 coherence is assumed and these calls have no effect.
 	 */
 	hv_setup_dma_ops(child_device,
-		device_get_dma_attr(&hv_acpi_dev->dev) == DEV_DMA_COHERENT);
+		device_get_dma_attr(vmbus_root_dev) == DEV_DMA_COHERENT);
 	return 0;
 }
 
@@ -2090,7 +2088,7 @@ int vmbus_device_register(struct hv_device *child_device_obj)
 		     &child_device_obj->channel->offermsg.offer.if_instance);
 
 	child_device_obj->device.bus = &hv_bus;
-	child_device_obj->device.parent = &hv_acpi_dev->dev;
+	child_device_obj->device.parent = vmbus_root_dev;
 	child_device_obj->device.release = vmbus_device_release;
 
 	child_device_obj->device.dma_parms = &child_device_obj->dma_parms;
@@ -2151,7 +2149,26 @@ void vmbus_device_unregister(struct hv_device *device_obj)
 	device_unregister(&device_obj->device);
 }
 
+static void vmbus_remove_mmio(void)
+{
+	struct resource *cur_res;
+	struct resource *next_res;
+
+	if (hyperv_mmio) {
+		if (fb_mmio) {
+			__release_region(hyperv_mmio, fb_mmio->start,
+					 resource_size(fb_mmio));
+			fb_mmio = NULL;
+		}
+
+		for (cur_res = hyperv_mmio; cur_res; cur_res = next_res) {
+			next_res = cur_res->sibling;
+			kfree(cur_res);
+		}
+	}
+}
 
+#ifdef CONFIG_ACPI
 /*
  * VMBUS is an acpi enumerated device. Get the information we
  * need from DSDT.
@@ -2264,21 +2281,7 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
 
 static void vmbus_acpi_remove(struct acpi_device *device)
 {
-	struct resource *cur_res;
-	struct resource *next_res;
-
-	if (hyperv_mmio) {
-		if (fb_mmio) {
-			__release_region(hyperv_mmio, fb_mmio->start,
-					 resource_size(fb_mmio));
-			fb_mmio = NULL;
-		}
-
-		for (cur_res = hyperv_mmio; cur_res; cur_res = next_res) {
-			next_res = cur_res->sibling;
-			kfree(cur_res);
-		}
-	}
+	vmbus_remove_mmio();
 }
 
 static void vmbus_reserve_fb(void)
@@ -2319,8 +2322,9 @@ static void vmbus_reserve_fb(void)
 	 * reserving a larger area and make it smaller until it succeeds.
 	 */
 	for (; !fb_mmio && (size >= 0x100000); size >>= 1)
-		fb_mmio = __request_region(hyperv_mmio, start, size, fb_mmio_name, 0);
+		fb_mmio = __request_region(hyperv_mmio, start, size, "fb_range", 0);
 }
+#endif /* CONFIG_ACPI */
 
 /**
  * vmbus_allocate_mmio() - Pick a memory-mapped I/O range.
@@ -2441,13 +2445,14 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size)
 }
 EXPORT_SYMBOL_GPL(vmbus_free_mmio);
 
+#ifdef CONFIG_ACPI
 static int vmbus_acpi_add(struct acpi_device *device)
 {
 	acpi_status result;
 	int ret_val = -ENODEV;
 	struct acpi_device *ancestor;
 
-	hv_acpi_dev = device;
+	vmbus_root_dev = &device->dev;
 
 	/*
 	 * Older versions of Hyper-V for ARM64 fail to include the _CCA
@@ -2492,6 +2497,72 @@ static int vmbus_acpi_add(struct acpi_device *device)
 		vmbus_acpi_remove(device);
 	return ret_val;
 }
+#endif
+
+#ifdef CONFIG_OF
+static int vmbus_of_driver_probe(struct platform_device *dev)
+{
+	struct resource **cur_res = &hyperv_mmio;
+	struct device_node *np;
+	const __be32 *ranges;
+	u32 nr_addr, nr_size, nr_parent_addr_cells, nr_ranges;
+	u32 range_len, range_size;
+	int i;
+
+	vmbus_root_dev = &dev->dev;
+	np = vmbus_root_dev->of_node;
+
+	if (of_property_read_u32(np, "#address-cells", &nr_addr))
+		return -ENOENT;
+	if (of_property_read_u32(np, "#size-cells", &nr_size))
+		return -ENOENT;
+	nr_parent_addr_cells = of_n_addr_cells(np);
+
+	if (nr_parent_addr_cells != 2 || nr_addr != 2 || nr_size != 1) {
+		pr_err("Address format is not supported\n");
+		return -EINVAL;
+	}
+
+	ranges = of_get_property(np, "ranges", &range_len);
+	if (!ranges)
+		return -ENOENT;
+
+	range_size = nr_parent_addr_cells + nr_addr + nr_size; // in cells
+	nr_ranges = range_len / sizeof(__be32) / range_size;
+
+	for (i = 0; i < nr_ranges; ++i, ranges += range_size) {
+		struct resource *res;
+		/*
+		 * The first u64 in the ranges description isn't used currently.
+		 * u64 _ = of_read_number(ranges, nr_parent_addr_cells);
+		 */
+		u64 start = of_read_number(ranges + nr_parent_addr_cells, nr_addr);
+		u32 len = of_read_number(ranges + nr_parent_addr_cells + nr_addr, nr_size);
+
+		pr_debug("VMBUS DeviceTree MMIO region start %#llx, %#x\n", start, len);
+
+		res = kzalloc(sizeof(*res), GFP_ATOMIC);
+		if (!res)
+			return -ENOMEM;
+
+		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;
+	}
+
+	return 0;
+}
+
+static int vmbus_of_driver_remove(struct platform_device *dev)
+{
+	vmbus_remove_mmio();
+	return 0;
+}
+#endif
 
 #ifdef CONFIG_PM_SLEEP
 static int vmbus_bus_suspend(struct device *dev)
@@ -2630,6 +2701,9 @@ static int vmbus_bus_resume(struct device *dev)
 #define vmbus_bus_resume NULL
 #endif /* CONFIG_PM_SLEEP */
 
+#define DRV_NAME "vmbus"
+
+#ifdef CONFIG_ACPI
 static const struct acpi_device_id vmbus_acpi_device_ids[] = {
 	{"VMBUS", 0},
 	{"VMBus", 0},
@@ -2659,7 +2733,7 @@ static int vmbus_bus_resume(struct device *dev)
 };
 
 static struct acpi_driver vmbus_acpi_driver = {
-	.name = "vmbus",
+	.name = DRV_NAME,
 	.ids = vmbus_acpi_device_ids,
 	.ops = {
 		.add = vmbus_acpi_add,
@@ -2669,6 +2743,7 @@ static int vmbus_bus_resume(struct device *dev)
 	.drv.probe_type = PROBE_FORCE_SYNCHRONOUS,
 };
 
+#endif
 static void hv_kexec_handler(void)
 {
 	hv_stimer_global_cleanup();
@@ -2737,7 +2812,32 @@ static void hv_synic_resume(void)
 	.resume = hv_synic_resume,
 };
 
-static int __init hv_acpi_init(void)
+#ifdef CONFIG_OF
+static const struct of_device_id vmbus_of_match[] = {
+	{
+		.name = "msft,vmbus",
+		.compatible = "msft,vmbus",
+		.data = NULL
+	},
+	{
+		/* sentinel */
+	},
+};
+MODULE_DEVICE_TABLE(of, vmbus_of_match);
+
+static struct platform_driver vmbus_platform_driver = {
+	.probe = vmbus_of_driver_probe,
+	.remove = vmbus_of_driver_remove,
+	.driver = {
+		.name = DRV_NAME,
+		.of_match_table = of_match_ptr(vmbus_of_match),
+		.pm = &vmbus_pm,
+		.bus = &hv_bus,
+	}
+};
+#endif
+
+static int __init vmbus_init(void)
 {
 	int ret;
 
@@ -2747,18 +2847,27 @@ static int __init hv_acpi_init(void)
 	if (hv_root_partition && !hv_nested)
 		return 0;
 
+#ifdef CONFIG_ACPI
 	/*
-	 * Get ACPI resources first.
+	 * Request ACPI resources and wait for the completion
 	 */
 	ret = acpi_bus_register_driver(&vmbus_acpi_driver);
 
 	if (ret)
 		return ret;
 
-	if (!hv_acpi_dev) {
-		ret = -ENODEV;
+	if (!vmbus_root_dev) {
+		ret = -ETIMEDOUT;
 		goto cleanup;
 	}
+#endif
+#ifdef CONFIG_OF
+	ret = platform_driver_register(&vmbus_platform_driver);
+	if (ret) {
+		pr_err("Error registering platform resources: %d\n", ret);
+		goto cleanup;
+	}
+#endif
 
 	/*
 	 * If we're on an architecture with a hardcoded hypervisor
@@ -2785,8 +2894,14 @@ static int __init hv_acpi_init(void)
 	return 0;
 
 cleanup:
+#ifdef CONFIG_ACPI
 	acpi_bus_unregister_driver(&vmbus_acpi_driver);
-	hv_acpi_dev = NULL;
+#endif
+#ifdef CONFIG_OF
+	platform_driver_unregister(&vmbus_platform_driver);
+#endif
+	vmbus_root_dev = NULL;
+
 	return ret;
 }
 
@@ -2839,12 +2954,17 @@ static void __exit vmbus_exit(void)
 
 	cpuhp_remove_state(hyperv_cpuhp_online);
 	hv_synic_free();
+#ifdef CONFIG_ACPI
 	acpi_bus_unregister_driver(&vmbus_acpi_driver);
+#endif
+#ifdef CONFIG_OF
+	platform_driver_unregister(&vmbus_platform_driver);
+#endif
+	vmbus_root_dev = NULL;
 }
 
-
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Microsoft Hyper-V VMBus Driver");
 
-subsys_initcall(hv_acpi_init);
+subsys_initcall(vmbus_init);
 module_exit(vmbus_exit);
-- 
1.8.3.1

[PATCH 4/4] dt-bindings: hv: Add dt-bindings for VMBus

[Saurabh Sengar]

Add dt-bindings for Hyper-V VMBus

Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
 .../devicetree/bindings/hv/msft,vmbus.yaml         | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hv/msft,vmbus.yaml

diff --git a/Documentation/devicetree/bindings/hv/msft,vmbus.yaml b/Documentation/devicetree/bindings/hv/msft,vmbus.yaml
new file mode 100644
index 0000000..66cb426
--- /dev/null
+++ b/Documentation/devicetree/bindings/hv/msft,vmbus.yaml
@@ -0,0 +1,34 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/hv/msft,vmbus.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microsoft Hyper-V VMBus device tree bindings
+
+maintainers:
+  - Saurabh Sengar <ssengar@linux.microsoft.com>
+
+description:
+  VMBus is a software bus that implement the protocols for communication
+  between the root or host OS and guest OSs (virtual machines).
+
+properties:
+  compatible:
+    const: msft,vmbus
+
+  ranges :
+    const: <0x00 0x00 0x0f 0xf0000000 0x10000000>
+
+required:
+  - compatible
+  - ranges
+
+examples:
+  - |
+        vmbus {
+		#address-cells = <0x02>;
+		#size-cells = <0x01>;
+		compatible = "msft,vmbus";
+		ranges = <0x00 0x00 0x0f 0xf0000000 0x10000000>;
+	};
-- 
1.8.3.1

Re: [PATCH 3/4] Drivers: hv: vmbus: Device Tree support

[Krzysztof Kozlowski]

On 16/01/2023 17:48, Saurabh Sengar wrote:
> Update the driver to use vmbus_root_dev device instead of acpi_device,
> which can be assigned to either ACPI or OF device, making VMBus agnostic
> to whether the device is using ACPI or device tree.
> 

(...)

>  
>  static void vmbus_reserve_fb(void)
> @@ -2319,8 +2322,9 @@ static void vmbus_reserve_fb(void)
>  	 * reserving a larger area and make it smaller until it succeeds.
>  	 */
>  	for (; !fb_mmio && (size >= 0x100000); size >>= 1)
> -		fb_mmio = __request_region(hyperv_mmio, start, size, fb_mmio_name, 0);
> +		fb_mmio = __request_region(hyperv_mmio, start, size, "fb_range", 0);

Your patch is doing much more than just adding OF. Adding OF is usually
just few lines, so this means you are refactoring driver and all this
work should be split to self-contained patches.

>  }
> +#endif /* CONFIG_ACPI */
>  
>  /**
>   * vmbus_allocate_mmio() - Pick a memory-mapped I/O range.
> @@ -2441,13 +2445,14 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size)
>  }
>  EXPORT_SYMBOL_GPL(vmbus_free_mmio);
>  
> +#ifdef CONFIG_ACPI
>  static int vmbus_acpi_add(struct acpi_device *device)
>  {
>  	acpi_status result;
>  	int ret_val = -ENODEV;
>  	struct acpi_device *ancestor;
>  
> -	hv_acpi_dev = device;
> +	vmbus_root_dev = &device->dev;
>  
>  	/*
>  	 * Older versions of Hyper-V for ARM64 fail to include the _CCA
> @@ -2492,6 +2497,72 @@ static int vmbus_acpi_add(struct acpi_device *device)
>  		vmbus_acpi_remove(device);
>  	return ret_val;
>  }
> +#endif
> +
> +#ifdef CONFIG_OF
> +static int vmbus_of_driver_probe(struct platform_device *dev)
> +{
> +	struct resource **cur_res = &hyperv_mmio;
> +	struct device_node *np;
> +	const __be32 *ranges;
> +	u32 nr_addr, nr_size, nr_parent_addr_cells, nr_ranges;
> +	u32 range_len, range_size;
> +	int i;
> +
> +	vmbus_root_dev = &dev->dev;
> +	np = vmbus_root_dev->of_node;
> +
> +	if (of_property_read_u32(np, "#address-cells", &nr_addr))
> +		return -ENOENT;
> +	if (of_property_read_u32(np, "#size-cells", &nr_size))
> +		return -ENOENT;
> +	nr_parent_addr_cells = of_n_addr_cells(np);
> +
> +	if (nr_parent_addr_cells != 2 || nr_addr != 2 || nr_size != 1) {
> +		pr_err("Address format is not supported\n");
> +		return -EINVAL;
> +	}
> +
> +	ranges = of_get_property(np, "ranges", &range_len);
> +	if (!ranges)
> +		return -ENOENT;
> +
> +	range_size = nr_parent_addr_cells + nr_addr + nr_size; // in cells
> +	nr_ranges = range_len / sizeof(__be32) / range_size;
> +
> +	for (i = 0; i < nr_ranges; ++i, ranges += range_size) {
> +		struct resource *res;
> +		/*
> +		 * The first u64 in the ranges description isn't used currently.
> +		 * u64 _ = of_read_number(ranges, nr_parent_addr_cells);
> +		 */
> +		u64 start = of_read_number(ranges + nr_parent_addr_cells, nr_addr);
> +		u32 len = of_read_number(ranges + nr_parent_addr_cells + nr_addr, nr_size);
> +
> +		pr_debug("VMBUS DeviceTree MMIO region start %#llx, %#x\n", start, len);

You must not print kernel or IO space addresses. You could use some
printk formats to hide the address, if this is really needed.

> +
> +		res = kzalloc(sizeof(*res), GFP_ATOMIC);
> +		if (!res)
> +			return -ENOMEM;
> +
> +		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;
> +	}
> +
> +	return 0;
> +}
> +
> +static int vmbus_of_driver_remove(struct platform_device *dev)
> +{
> +	vmbus_remove_mmio();
> +	return 0;
> +}
> +#endif
>  
>  #ifdef CONFIG_PM_SLEEP
>  static int vmbus_bus_suspend(struct device *dev)
> @@ -2630,6 +2701,9 @@ static int vmbus_bus_resume(struct device *dev)
>  #define vmbus_bus_resume NULL
>  #endif /* CONFIG_PM_SLEEP */
>  
> +#define DRV_NAME "vmbus"
> +
> +#ifdef CONFIG_ACPI
>  static const struct acpi_device_id vmbus_acpi_device_ids[] = {
>  	{"VMBUS", 0},
>  	{"VMBus", 0},
> @@ -2659,7 +2733,7 @@ static int vmbus_bus_resume(struct device *dev)
>  };
>  
>  static struct acpi_driver vmbus_acpi_driver = {
> -	.name = "vmbus",
> +	.name = DRV_NAME,

How this is related?

>  	.ids = vmbus_acpi_device_ids,
>  	.ops = {
>  		.add = vmbus_acpi_add,
> @@ -2669,6 +2743,7 @@ static int vmbus_bus_resume(struct device *dev)
>  	.drv.probe_type = PROBE_FORCE_SYNCHRONOUS,
>  };
>  
> +#endif
>  static void hv_kexec_handler(void)
>  {
>  	hv_stimer_global_cleanup();
> @@ -2737,7 +2812,32 @@ static void hv_synic_resume(void)
>  	.resume = hv_synic_resume,
>  };
>  
> -static int __init hv_acpi_init(void)
> +#ifdef CONFIG_OF
> +static const struct of_device_id vmbus_of_match[] = {
> +	{
> +		.name = "msft,vmbus",

Why do you need name?

> +		.compatible = "msft,vmbus",
> +		.data = NULL

Why do you need data field?

> +	},
> +	{
> +		/* sentinel */
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, vmbus_of_match);
> +
> +static struct platform_driver vmbus_platform_driver = {
> +	.probe = vmbus_of_driver_probe,
> +	.remove = vmbus_of_driver_remove,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.of_match_table = of_match_ptr(vmbus_of_match),
> +		.pm = &vmbus_pm,
> +		.bus = &hv_bus,
> +	}
> +};
> +#endif

Why platform driver is hidden by CONFIG_OF? It should not be the case.
The interface - ACPI or OF - should not differ for driver
infrastructure. Even one probe could be used - just drop all of_...
methods and use generic device_property_
> +
> +static int __init vmbus_init(void)
>  {
>  	int ret;
>  
> @@ -2747,18 +2847,27 @@ static int __init hv_acpi_init(void)
>  	if (hv_root_partition && !hv_nested)
>  		return 0;
>  
> +#ifdef CONFIG_ACPI
>  	/*
> -	 * Get ACPI resources first.
> +	 * Request ACPI resources and wait for the completion
>  	 */
>  	ret = acpi_bus_register_driver(&vmbus_acpi_driver);
>  
>  	if (ret)
>  		return ret;
>  
> -	if (!hv_acpi_dev) {
> -		ret = -ENODEV;
> +	if (!vmbus_root_dev) {
> +		ret = -ETIMEDOUT;
>  		goto cleanup;
>  	}
> +#endif
> +#ifdef CONFIG_OF
> +	ret = platform_driver_register(&vmbus_platform_driver);
> +	if (ret) {
> +		pr_err("Error registering platform resources: %d\n", ret);
> +		goto cleanup;
> +	}
> +#endif
>  
>  	/*
>  	 * If we're on an architecture with a hardcoded hypervisor
> @@ -2785,8 +2894,14 @@ static int __init hv_acpi_init(void)
>  	return 0;
>  
>  cleanup:
> +#ifdef CONFIG_ACPI
>  	acpi_bus_unregister_driver(&vmbus_acpi_driver);
> -	hv_acpi_dev = NULL;
> +#endif
> +#ifdef CONFIG_OF
> +	platform_driver_unregister(&vmbus_platform_driver);
> +#endif
> +	vmbus_root_dev = NULL;
> +
>  	return ret;
>  }
>  
> @@ -2839,12 +2954,17 @@ static void __exit vmbus_exit(void)
>  
>  	cpuhp_remove_state(hyperv_cpuhp_online);
>  	hv_synic_free();
> +#ifdef CONFIG_ACPI
>  	acpi_bus_unregister_driver(&vmbus_acpi_driver);
> +#endif
> +#ifdef CONFIG_OF
> +	platform_driver_unregister(&vmbus_platform_driver);
> +#endif
> +	vmbus_root_dev = NULL;
>  }
>  
> -

This is really a messy patch...

>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("Microsoft Hyper-V VMBus Driver");
>  
> -subsys_initcall(hv_acpi_init);
> +subsys_initcall(vmbus_init);
>  module_exit(vmbus_exit);

Best regards,
Krzysztof

Re: [PATCH 4/4] dt-bindings: hv: Add dt-bindings for VMBus

[Krzysztof Kozlowski]

On 16/01/2023 17:48, Saurabh Sengar wrote:
> Add dt-bindings for Hyper-V VMBus

Missing full stop.

Subject: drop second/last, redundant "dt-bindings for". The
"dt-bindings" prefix is already stating that these are bindings.

> 
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
>  .../devicetree/bindings/hv/msft,vmbus.yaml         | 34 ++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hv/msft,vmbus.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hv/msft,vmbus.yaml b/Documentation/devicetree/bindings/hv/msft,vmbus.yaml
> new file mode 100644
> index 0000000..66cb426
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hv/msft,vmbus.yaml
> @@ -0,0 +1,34 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/hv/msft,vmbus.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microsoft Hyper-V VMBus device tree bindings

Drop "device tree bindings"

> +
> +maintainers:
> +  - Saurabh Sengar <ssengar@linux.microsoft.com>
> +
> +description:
> +  VMBus is a software bus that implement the protocols for communication
> +  between the root or host OS and guest OSs (virtual machines).

Why this cannot be auto-discoverable? Why do you need OF for this?

> +
> +properties:
> +  compatible:
> +    const: msft,vmbus
> +
> +  ranges :
> +    const: <0x00 0x00 0x0f 0xf0000000 0x10000000>

Did you test the bindings?

This property does not look correct. If you have static addresses, you
do not need OF. What do you want to discover here?

> +
> +required:
> +  - compatible
> +  - ranges
> +
> +examples:
> +  - |
> +        vmbus {

Use 4 spaces for example indentation.

> +		#address-cells = <0x02>;
> +		#size-cells = <0x01>;

That's not correct style. Drop hex notation. Drop leading zeros.

But anyway you did not test the bindings. This cannot work. Try.

> +		compatible = "msft,vmbus";

compatible is a first property.

> +		ranges = <0x00 0x00 0x0f 0xf0000000 0x10000000>;

What do you translate? There is no reg, no unit address.

> +	};

Best regards,
Krzysztof

Re: [PATCH 4/4] dt-bindings: hv: Add dt-bindings for VMBus

[Krzysztof Kozlowski]

On 16/01/2023 17:48, Saurabh Sengar wrote:
> Add dt-bindings for Hyper-V VMBus
> 
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
>  .../devicetree/bindings/hv/msft,vmbus.yaml         | 34 ++++++++++++++++++++++

Also, there is no "hv" hardware, so that's not correct location. If your
bindings describe firmware, this should go to firmware. Otherwise, this
does not look like suitable for DT. We do not describe software stuff in DT.

Best regards,
Krzysztof

Re: [PATCH 4/4] dt-bindings: hv: Add dt-bindings for VMBus

[Rob Herring]

On Mon, 16 Jan 2023 08:48:08 -0800, Saurabh Sengar wrote:
> Add dt-bindings for Hyper-V VMBus
> 
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
>  .../devicetree/bindings/hv/msft,vmbus.yaml         | 34 ++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hv/msft,vmbus.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/hv/msft,vmbus.yaml:20:9: [warning] too many spaces before colon (colons)
./Documentation/devicetree/bindings/hv/msft,vmbus.yaml:30:1: [error] syntax error: found character '\t' that cannot start any token (syntax)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/hv/msft,vmbus.example.dts'
Documentation/devicetree/bindings/hv/msft,vmbus.yaml:30:1: found a tab character where an indentation space is expected
make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/hv/msft,vmbus.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/hv/msft,vmbus.yaml:30:1: found a tab character where an indentation space is expected
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hv/msft,vmbus.yaml: ignoring, error parsing file
make: *** [Makefile:1508: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1673887688-19151-5-git-send-email-ssengar@linux.microsoft.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 3/4] Drivers: hv: vmbus: Device Tree support

[Rob Herring]

On Mon, Jan 16, 2023 at 08:48:07AM -0800, Saurabh Sengar wrote:
> Update the driver to use vmbus_root_dev device instead of acpi_device,
> which can be assigned to either ACPI or OF device, making VMBus agnostic
> to whether the device is using ACPI or device tree.
> 
> When built for OF, the driver registers as a platform driver and includes
> a 'of' probe function that is alternate to the existing ACPI registration
> and probe when built for ACPI. The VMBus driver maintains its registration
> as the bus that other VMBus devices plug into, regardless of whether it is
> initially probed and initialized via ACPI or OF.
> 
> This change also introduce vmbus_remove_mmio function, which helps removing
> the duplicate code for mmio cleanup between APIC and OF driver.
> 
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
>  drivers/hv/vmbus_drv.c | 190 ++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 155 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 1901556..894b360 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -11,11 +11,13 @@
>  
>  #include <linux/init.h>
>  #include <linux/module.h>
> -#include <linux/device.h>
> +#include <linux/platform_device.h>
>  #include <linux/interrupt.h>
>  #include <linux/sysctl.h>
>  #include <linux/slab.h>
>  #include <linux/acpi.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
>  #include <linux/completion.h>
>  #include <linux/hyperv.h>
>  #include <linux/kernel_stat.h>
> @@ -44,14 +46,14 @@ struct vmbus_dynid {
>  	struct hv_vmbus_device_id id;
>  };
>  
> -static struct acpi_device  *hv_acpi_dev;
> -
>  static int hyperv_cpuhp_online;
>  
>  static void *hv_panic_page;
>  
>  static long __percpu *vmbus_evt;
>  
> +static struct device *vmbus_root_dev;
> +
>  /* Values parsed from ACPI DSDT */
>  int vmbus_irq;
>  int vmbus_interrupt;
> @@ -136,17 +138,13 @@ static int hv_die_panic_notify_crash(struct notifier_block *self,
>  	return NOTIFY_DONE;
>  }
>  
> -static const char *fb_mmio_name = "fb_range";
>  static struct resource *fb_mmio;
>  static struct resource *hyperv_mmio;
>  static DEFINE_MUTEX(hyperv_mmio_lock);
>  
>  static int vmbus_exists(void)
>  {
> -	if (hv_acpi_dev == NULL)
> -		return -ENODEV;
> -
> -	return 0;
> +	return vmbus_root_dev ? 0 : -ENODEV;
>  }
>  
>  static u8 channel_monitor_group(const struct vmbus_channel *channel)
> @@ -932,7 +930,7 @@ static int vmbus_dma_configure(struct device *child_device)
>  	 * On x86/x64 coherence is assumed and these calls have no effect.
>  	 */
>  	hv_setup_dma_ops(child_device,
> -		device_get_dma_attr(&hv_acpi_dev->dev) == DEV_DMA_COHERENT);
> +		device_get_dma_attr(vmbus_root_dev) == DEV_DMA_COHERENT);
>  	return 0;
>  }
>  
> @@ -2090,7 +2088,7 @@ int vmbus_device_register(struct hv_device *child_device_obj)
>  		     &child_device_obj->channel->offermsg.offer.if_instance);
>  
>  	child_device_obj->device.bus = &hv_bus;
> -	child_device_obj->device.parent = &hv_acpi_dev->dev;
> +	child_device_obj->device.parent = vmbus_root_dev;
>  	child_device_obj->device.release = vmbus_device_release;
>  
>  	child_device_obj->device.dma_parms = &child_device_obj->dma_parms;
> @@ -2151,7 +2149,26 @@ void vmbus_device_unregister(struct hv_device *device_obj)
>  	device_unregister(&device_obj->device);
>  }
>  
> +static void vmbus_remove_mmio(void)
> +{
> +	struct resource *cur_res;
> +	struct resource *next_res;
> +
> +	if (hyperv_mmio) {
> +		if (fb_mmio) {
> +			__release_region(hyperv_mmio, fb_mmio->start,
> +					 resource_size(fb_mmio));
> +			fb_mmio = NULL;
> +		}
> +
> +		for (cur_res = hyperv_mmio; cur_res; cur_res = next_res) {
> +			next_res = cur_res->sibling;
> +			kfree(cur_res);
> +		}
> +	}
> +}
>  
> +#ifdef CONFIG_ACPI
>  /*
>   * VMBUS is an acpi enumerated device. Get the information we
>   * need from DSDT.
> @@ -2264,21 +2281,7 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
>  
>  static void vmbus_acpi_remove(struct acpi_device *device)
>  {
> -	struct resource *cur_res;
> -	struct resource *next_res;
> -
> -	if (hyperv_mmio) {
> -		if (fb_mmio) {
> -			__release_region(hyperv_mmio, fb_mmio->start,
> -					 resource_size(fb_mmio));
> -			fb_mmio = NULL;
> -		}
> -
> -		for (cur_res = hyperv_mmio; cur_res; cur_res = next_res) {
> -			next_res = cur_res->sibling;
> -			kfree(cur_res);
> -		}
> -	}
> +	vmbus_remove_mmio();
>  }
>  
>  static void vmbus_reserve_fb(void)
> @@ -2319,8 +2322,9 @@ static void vmbus_reserve_fb(void)
>  	 * reserving a larger area and make it smaller until it succeeds.
>  	 */
>  	for (; !fb_mmio && (size >= 0x100000); size >>= 1)
> -		fb_mmio = __request_region(hyperv_mmio, start, size, fb_mmio_name, 0);
> +		fb_mmio = __request_region(hyperv_mmio, start, size, "fb_range", 0);
>  }
> +#endif /* CONFIG_ACPI */
>  
>  /**
>   * vmbus_allocate_mmio() - Pick a memory-mapped I/O range.
> @@ -2441,13 +2445,14 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size)
>  }
>  EXPORT_SYMBOL_GPL(vmbus_free_mmio);
>  
> +#ifdef CONFIG_ACPI
>  static int vmbus_acpi_add(struct acpi_device *device)
>  {
>  	acpi_status result;
>  	int ret_val = -ENODEV;
>  	struct acpi_device *ancestor;
>  
> -	hv_acpi_dev = device;
> +	vmbus_root_dev = &device->dev;
>  
>  	/*
>  	 * Older versions of Hyper-V for ARM64 fail to include the _CCA
> @@ -2492,6 +2497,72 @@ static int vmbus_acpi_add(struct acpi_device *device)
>  		vmbus_acpi_remove(device);
>  	return ret_val;
>  }
> +#endif
> +
> +#ifdef CONFIG_OF
> +static int vmbus_of_driver_probe(struct platform_device *dev)
> +{
> +	struct resource **cur_res = &hyperv_mmio;
> +	struct device_node *np;
> +	const __be32 *ranges;
> +	u32 nr_addr, nr_size, nr_parent_addr_cells, nr_ranges;
> +	u32 range_len, range_size;
> +	int i;
> +
> +	vmbus_root_dev = &dev->dev;
> +	np = vmbus_root_dev->of_node;
> +
> +	if (of_property_read_u32(np, "#address-cells", &nr_addr))
> +		return -ENOENT;
> +	if (of_property_read_u32(np, "#size-cells", &nr_size))
> +		return -ENOENT;
> +	nr_parent_addr_cells = of_n_addr_cells(np);
> +
> +	if (nr_parent_addr_cells != 2 || nr_addr != 2 || nr_size != 1) {
> +		pr_err("Address format is not supported\n");
> +		return -EINVAL;
> +	}
> +
> +	ranges = of_get_property(np, "ranges", &range_len);
> +	if (!ranges)
> +		return -ENOENT;
> +
> +	range_size = nr_parent_addr_cells + nr_addr + nr_size; // in cells
> +	nr_ranges = range_len / sizeof(__be32) / range_size;
> +
> +	for (i = 0; i < nr_ranges; ++i, ranges += range_size) {
> +		struct resource *res;
> +		/*
> +		 * The first u64 in the ranges description isn't used currently.
> +		 * u64 _ = of_read_number(ranges, nr_parent_addr_cells);
> +		 */
> +		u64 start = of_read_number(ranges + nr_parent_addr_cells, nr_addr);
> +		u32 len = of_read_number(ranges + nr_parent_addr_cells + nr_addr, nr_size);

These are all standard properties, so you shouldn't be parsing them 
yourself. Likely you are doing it wrong. For example, where do you 
handle address translations in parent nodes?

Use or add to what is in drivers/of/address.c.


> +		pr_debug("VMBUS DeviceTree MMIO region start %#llx, %#x\n", start, len);
> +
> +		res = kzalloc(sizeof(*res), GFP_ATOMIC);
> +		if (!res)
> +			return -ENOMEM;
> +
> +		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;
> +	}
> +
> +	return 0;
> +}
> +
> +static int vmbus_of_driver_remove(struct platform_device *dev)
> +{
> +	vmbus_remove_mmio();
> +	return 0;
> +}
> +#endif
>  
>  #ifdef CONFIG_PM_SLEEP
>  static int vmbus_bus_suspend(struct device *dev)
> @@ -2630,6 +2701,9 @@ static int vmbus_bus_resume(struct device *dev)
>  #define vmbus_bus_resume NULL
>  #endif /* CONFIG_PM_SLEEP */
>  
> +#define DRV_NAME "vmbus"
> +
> +#ifdef CONFIG_ACPI
>  static const struct acpi_device_id vmbus_acpi_device_ids[] = {
>  	{"VMBUS", 0},
>  	{"VMBus", 0},
> @@ -2659,7 +2733,7 @@ static int vmbus_bus_resume(struct device *dev)
>  };
>  
>  static struct acpi_driver vmbus_acpi_driver = {
> -	.name = "vmbus",
> +	.name = DRV_NAME,
>  	.ids = vmbus_acpi_device_ids,
>  	.ops = {
>  		.add = vmbus_acpi_add,
> @@ -2669,6 +2743,7 @@ static int vmbus_bus_resume(struct device *dev)
>  	.drv.probe_type = PROBE_FORCE_SYNCHRONOUS,
>  };
>  
> +#endif
>  static void hv_kexec_handler(void)
>  {
>  	hv_stimer_global_cleanup();
> @@ -2737,7 +2812,32 @@ static void hv_synic_resume(void)
>  	.resume = hv_synic_resume,
>  };
>  
> -static int __init hv_acpi_init(void)
> +#ifdef CONFIG_OF
> +static const struct of_device_id vmbus_of_match[] = {
> +	{
> +		.name = "msft,vmbus",
> +		.compatible = "msft,vmbus",
> +		.data = NULL
> +	},
> +	{
> +		/* sentinel */
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, vmbus_of_match);
> +
> +static struct platform_driver vmbus_platform_driver = {
> +	.probe = vmbus_of_driver_probe,
> +	.remove = vmbus_of_driver_remove,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.of_match_table = of_match_ptr(vmbus_of_match),
> +		.pm = &vmbus_pm,
> +		.bus = &hv_bus,
> +	}
> +};
> +#endif
> +
> +static int __init vmbus_init(void)
>  {
>  	int ret;
>  
> @@ -2747,18 +2847,27 @@ static int __init hv_acpi_init(void)
>  	if (hv_root_partition && !hv_nested)
>  		return 0;
>  
> +#ifdef CONFIG_ACPI

Use 'if (IS_ENABLED(CONFIG_ACPI))' here and anywhere else you can.

Rob

Re: [PATCH 0/4] Device tree support for Hyper-V VMBus driver

[Rob Herring]

On Mon, Jan 16, 2023 at 08:48:04AM -0800, Saurabh Sengar wrote:
> This patch set expands the functionality of the VMBus driver by adding
> support for device tree on x86/x64 architectures.

Humm, interesting. Currently we support OLPC and CE4100 for x86 DT based 
systems. Adding a new platform implies numerous new bindings yet there 
is only 1 here. I'm guessing your DT is generated by the hypervisor, but 
an example would be nice to see and run thru dtschema validation. We've 
unfortunately been trying to fix KVM/QEMU DT years after it was created.


> The first two patches enable Hyper-V builds for non-ACPI systems, while
> the third patch adds device tree support into the VMBus driver, in
> addition to its pre-existing support for ACPI. The fourth patch includes
> the necessary device tree bindings for the VMBus driver.

Bindings come before using them...

> 
> Saurabh Sengar (4):
>   drivers/clocksource/hyper-v: non ACPI support in hyperv clock
>   Drivers: hv: allow non ACPI compilation for
>     hv_is_hibernation_supported
>   Drivers: hv: vmbus: Device Tree support
>   dt-bindings: hv: Add dt-bindings for VMBus
> 
>  .../devicetree/bindings/hv/msft,vmbus.yaml         |  34 ++++
>  drivers/clocksource/hyperv_timer.c                 |  15 +-
>  drivers/hv/hv_common.c                             |   4 +
>  drivers/hv/vmbus_drv.c                             | 190 +++++++++++++++++----
>  4 files changed, 206 insertions(+), 37 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/hv/msft,vmbus.yaml
> 
> -- 
> 1.8.3.1
> 

Re: [PATCH 4/4] dt-bindings: hv: Add dt-bindings for VMBus

[Saurabh Singh Sengar]

On Mon, Jan 16, 2023 at 07:55:13PM +0100, Krzysztof Kozlowski wrote:
> On 16/01/2023 17:48, Saurabh Sengar wrote:
> > Add dt-bindings for Hyper-V VMBus
> > 
> > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > ---
> >  .../devicetree/bindings/hv/msft,vmbus.yaml         | 34 ++++++++++++++++++++++
> 
> Also, there is no "hv" hardware, so that's not correct location. If your
> bindings describe firmware, this should go to firmware. Otherwise, this
> does not look like suitable for DT. We do not describe software stuff in DT.

VMBus is a virtual device this is simmilar to virtio. I can rename this folder to vmbus.

> 
> Best regards,
> Krzysztof

Re: [PATCH 3/4] Drivers: hv: vmbus: Device Tree support

[Saurabh Singh Sengar]

On Mon, Jan 16, 2023 at 07:48:25PM +0100, Krzysztof Kozlowski wrote:
> On 16/01/2023 17:48, Saurabh Sengar wrote:
> > Update the driver to use vmbus_root_dev device instead of acpi_device,
> > which can be assigned to either ACPI or OF device, making VMBus agnostic
> > to whether the device is using ACPI or device tree.
> > 
> 
> (...)
> 
> >  
> >  static void vmbus_reserve_fb(void)
> > @@ -2319,8 +2322,9 @@ static void vmbus_reserve_fb(void)
> >  	 * reserving a larger area and make it smaller until it succeeds.
> >  	 */
> >  	for (; !fb_mmio && (size >= 0x100000); size >>= 1)
> > -		fb_mmio = __request_region(hyperv_mmio, start, size, fb_mmio_name, 0);
> > +		fb_mmio = __request_region(hyperv_mmio, start, size, "fb_range", 0);
> 
> Your patch is doing much more than just adding OF. Adding OF is usually
> just few lines, so this means you are refactoring driver and all this
> work should be split to self-contained patches.
> 
> >  }
> > +#endif /* CONFIG_ACPI */
> >  
> >  /**
> >   * vmbus_allocate_mmio() - Pick a memory-mapped I/O range.
> > @@ -2441,13 +2445,14 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size)
> >  }
> >  EXPORT_SYMBOL_GPL(vmbus_free_mmio);
> >  
> > +#ifdef CONFIG_ACPI
> >  static int vmbus_acpi_add(struct acpi_device *device)
> >  {
> >  	acpi_status result;
> >  	int ret_val = -ENODEV;
> >  	struct acpi_device *ancestor;
> >  
> > -	hv_acpi_dev = device;
> > +	vmbus_root_dev = &device->dev;
> >  
> >  	/*
> >  	 * Older versions of Hyper-V for ARM64 fail to include the _CCA
> > @@ -2492,6 +2497,72 @@ static int vmbus_acpi_add(struct acpi_device *device)
> >  		vmbus_acpi_remove(device);
> >  	return ret_val;
> >  }
> > +#endif
> > +
> > +#ifdef CONFIG_OF
> > +static int vmbus_of_driver_probe(struct platform_device *dev)
> > +{
> > +	struct resource **cur_res = &hyperv_mmio;
> > +	struct device_node *np;
> > +	const __be32 *ranges;
> > +	u32 nr_addr, nr_size, nr_parent_addr_cells, nr_ranges;
> > +	u32 range_len, range_size;
> > +	int i;
> > +
> > +	vmbus_root_dev = &dev->dev;
> > +	np = vmbus_root_dev->of_node;
> > +
> > +	if (of_property_read_u32(np, "#address-cells", &nr_addr))
> > +		return -ENOENT;
> > +	if (of_property_read_u32(np, "#size-cells", &nr_size))
> > +		return -ENOENT;
> > +	nr_parent_addr_cells = of_n_addr_cells(np);
> > +
> > +	if (nr_parent_addr_cells != 2 || nr_addr != 2 || nr_size != 1) {
> > +		pr_err("Address format is not supported\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	ranges = of_get_property(np, "ranges", &range_len);
> > +	if (!ranges)
> > +		return -ENOENT;
> > +
> > +	range_size = nr_parent_addr_cells + nr_addr + nr_size; // in cells
> > +	nr_ranges = range_len / sizeof(__be32) / range_size;
> > +
> > +	for (i = 0; i < nr_ranges; ++i, ranges += range_size) {
> > +		struct resource *res;
> > +		/*
> > +		 * The first u64 in the ranges description isn't used currently.
> > +		 * u64 _ = of_read_number(ranges, nr_parent_addr_cells);
> > +		 */
> > +		u64 start = of_read_number(ranges + nr_parent_addr_cells, nr_addr);
> > +		u32 len = of_read_number(ranges + nr_parent_addr_cells + nr_addr, nr_size);
> > +
> > +		pr_debug("VMBUS DeviceTree MMIO region start %#llx, %#x\n", start, len);
> 
> You must not print kernel or IO space addresses. You could use some
> printk formats to hide the address, if this is really needed.
> 
> > +
> > +		res = kzalloc(sizeof(*res), GFP_ATOMIC);
> > +		if (!res)
> > +			return -ENOMEM;
> > +
> > +		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;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int vmbus_of_driver_remove(struct platform_device *dev)
> > +{
> > +	vmbus_remove_mmio();
> > +	return 0;
> > +}
> > +#endif
> >  
> >  #ifdef CONFIG_PM_SLEEP
> >  static int vmbus_bus_suspend(struct device *dev)
> > @@ -2630,6 +2701,9 @@ static int vmbus_bus_resume(struct device *dev)
> >  #define vmbus_bus_resume NULL
> >  #endif /* CONFIG_PM_SLEEP */
> >  
> > +#define DRV_NAME "vmbus"
> > +
> > +#ifdef CONFIG_ACPI
> >  static const struct acpi_device_id vmbus_acpi_device_ids[] = {
> >  	{"VMBUS", 0},
> >  	{"VMBus", 0},
> > @@ -2659,7 +2733,7 @@ static int vmbus_bus_resume(struct device *dev)
> >  };
> >  
> >  static struct acpi_driver vmbus_acpi_driver = {
> > -	.name = "vmbus",
> > +	.name = DRV_NAME,
> 
> How this is related?
> 
> >  	.ids = vmbus_acpi_device_ids,
> >  	.ops = {
> >  		.add = vmbus_acpi_add,
> > @@ -2669,6 +2743,7 @@ static int vmbus_bus_resume(struct device *dev)
> >  	.drv.probe_type = PROBE_FORCE_SYNCHRONOUS,
> >  };
> >  
> > +#endif
> >  static void hv_kexec_handler(void)
> >  {
> >  	hv_stimer_global_cleanup();
> > @@ -2737,7 +2812,32 @@ static void hv_synic_resume(void)
> >  	.resume = hv_synic_resume,
> >  };
> >  
> > -static int __init hv_acpi_init(void)
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id vmbus_of_match[] = {
> > +	{
> > +		.name = "msft,vmbus",
> 
> Why do you need name?
> 
> > +		.compatible = "msft,vmbus",
> > +		.data = NULL
> 
> Why do you need data field?
> 
> > +	},
> > +	{
> > +		/* sentinel */
> > +	},
> > +};
> > +MODULE_DEVICE_TABLE(of, vmbus_of_match);
> > +
> > +static struct platform_driver vmbus_platform_driver = {
> > +	.probe = vmbus_of_driver_probe,
> > +	.remove = vmbus_of_driver_remove,
> > +	.driver = {
> > +		.name = DRV_NAME,
> > +		.of_match_table = of_match_ptr(vmbus_of_match),
> > +		.pm = &vmbus_pm,
> > +		.bus = &hv_bus,
> > +	}
> > +};
> > +#endif
> 
> Why platform driver is hidden by CONFIG_OF? It should not be the case.
> The interface - ACPI or OF - should not differ for driver
> infrastructure. Even one probe could be used - just drop all of_...
> methods and use generic device_property_

This is a very heavily used driver with multiple drivers dependent on it
and I was reluctant to change what is working for so many years. Thus added
the OF support as minimal disturbance to existing systems. However, if this
needs to be changed I will work on evaluating what impact it will bring to
change acpi driver to platform for ACPI systems. If it all works-out, I
understand I have to send a patch to change existing acpi driver to platform
and then extend the device tree support in separate patch.

I can use device_propert_ but I am not clear how will that help parsing in
ACPI (DTSDS) flow, it will be lot simpler if we can have two separate parsing
functions for ACPI and OF. Hope having two separate parsing functions in single
probe is acceptable?

> > +
> > +static int __init vmbus_init(void)
> >  {
> >  	int ret;
> >  
> > @@ -2747,18 +2847,27 @@ static int __init hv_acpi_init(void)
> >  	if (hv_root_partition && !hv_nested)
> >  		return 0;
> >  
> > +#ifdef CONFIG_ACPI
> >  	/*
> > -	 * Get ACPI resources first.
> > +	 * Request ACPI resources and wait for the completion
> >  	 */
> >  	ret = acpi_bus_register_driver(&vmbus_acpi_driver);
> >  
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (!hv_acpi_dev) {
> > -		ret = -ENODEV;
> > +	if (!vmbus_root_dev) {
> > +		ret = -ETIMEDOUT;
> >  		goto cleanup;
> >  	}
> > +#endif
> > +#ifdef CONFIG_OF
> > +	ret = platform_driver_register(&vmbus_platform_driver);
> > +	if (ret) {
> > +		pr_err("Error registering platform resources: %d\n", ret);
> > +		goto cleanup;
> > +	}
> > +#endif
> >  
> >  	/*
> >  	 * If we're on an architecture with a hardcoded hypervisor
> > @@ -2785,8 +2894,14 @@ static int __init hv_acpi_init(void)
> >  	return 0;
> >  
> >  cleanup:
> > +#ifdef CONFIG_ACPI
> >  	acpi_bus_unregister_driver(&vmbus_acpi_driver);
> > -	hv_acpi_dev = NULL;
> > +#endif
> > +#ifdef CONFIG_OF
> > +	platform_driver_unregister(&vmbus_platform_driver);
> > +#endif
> > +	vmbus_root_dev = NULL;
> > +
> >  	return ret;
> >  }
> >  
> > @@ -2839,12 +2954,17 @@ static void __exit vmbus_exit(void)
> >  
> >  	cpuhp_remove_state(hyperv_cpuhp_online);
> >  	hv_synic_free();
> > +#ifdef CONFIG_ACPI
> >  	acpi_bus_unregister_driver(&vmbus_acpi_driver);
> > +#endif
> > +#ifdef CONFIG_OF
> > +	platform_driver_unregister(&vmbus_platform_driver);
> > +#endif
> > +	vmbus_root_dev = NULL;
> >  }
> >  
> > -
> 
> This is really a messy patch...
> 
> >  MODULE_LICENSE("GPL");
> >  MODULE_DESCRIPTION("Microsoft Hyper-V VMBus Driver");
> >  
> > -subsys_initcall(hv_acpi_init);
> > +subsys_initcall(vmbus_init);
> >  module_exit(vmbus_exit);
> 
> Best regards,
> Krzysztof

Re: [PATCH 4/4] dt-bindings: hv: Add dt-bindings for VMBus

[Krzysztof Kozlowski]

On 17/01/2023 16:13, Saurabh Singh Sengar wrote:
> On Mon, Jan 16, 2023 at 07:55:13PM +0100, Krzysztof Kozlowski wrote:
>> On 16/01/2023 17:48, Saurabh Sengar wrote:
>>> Add dt-bindings for Hyper-V VMBus
>>>
>>> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
>>> ---
>>>  .../devicetree/bindings/hv/msft,vmbus.yaml         | 34 ++++++++++++++++++++++
>>
>> Also, there is no "hv" hardware, so that's not correct location. If your
>> bindings describe firmware, this should go to firmware. Otherwise, this
>> does not look like suitable for DT. We do not describe software stuff in DT.
> 
> VMBus is a virtual device this is simmilar to virtio. I can rename this folder to vmbus.
>

Then virtio directory. The directories are per subsystems (hardware
classes).

Best regards,
Krzysztof

Re: [PATCH 4/4] dt-bindings: hv: Add dt-bindings for VMBus

[Saurabh Singh Sengar]

On Tue, Jan 17, 2023 at 04:41:22PM +0100, Krzysztof Kozlowski wrote:
> On 17/01/2023 16:13, Saurabh Singh Sengar wrote:
> > On Mon, Jan 16, 2023 at 07:55:13PM +0100, Krzysztof Kozlowski wrote:
> >> On 16/01/2023 17:48, Saurabh Sengar wrote:
> >>> Add dt-bindings for Hyper-V VMBus
> >>>
> >>> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> >>> ---
> >>>  .../devicetree/bindings/hv/msft,vmbus.yaml         | 34 ++++++++++++++++++++++
> >>
> >> Also, there is no "hv" hardware, so that's not correct location. If your
> >> bindings describe firmware, this should go to firmware. Otherwise, this
> >> does not look like suitable for DT. We do not describe software stuff in DT.
> > 
> > VMBus is a virtual device this is simmilar to virtio. I can rename this folder to vmbus.
> >
> 
> Then virtio directory. The directories are per subsystems (hardware
> classes).

Apologies if I was not clear, I meant to say this is a device conceptually
similar to virtio. But this driver has nothing to do with virtio, we should
be creating a new folder for it OR I am fine moving it under bus if that's
okay.

> 
> Best regards,
> Krzysztof

Re: [PATCH 4/4] dt-bindings: hv: Add dt-bindings for VMBus

[Krzysztof Kozlowski]

On 17/01/2023 16:52, Saurabh Singh Sengar wrote:
> On Tue, Jan 17, 2023 at 04:41:22PM +0100, Krzysztof Kozlowski wrote:
>> On 17/01/2023 16:13, Saurabh Singh Sengar wrote:
>>> On Mon, Jan 16, 2023 at 07:55:13PM +0100, Krzysztof Kozlowski wrote:
>>>> On 16/01/2023 17:48, Saurabh Sengar wrote:
>>>>> Add dt-bindings for Hyper-V VMBus
>>>>>
>>>>> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
>>>>> ---
>>>>>  .../devicetree/bindings/hv/msft,vmbus.yaml         | 34 ++++++++++++++++++++++
>>>>
>>>> Also, there is no "hv" hardware, so that's not correct location. If your
>>>> bindings describe firmware, this should go to firmware. Otherwise, this
>>>> does not look like suitable for DT. We do not describe software stuff in DT.
>>>
>>> VMBus is a virtual device this is simmilar to virtio. I can rename this folder to vmbus.
>>>
>>
>> Then virtio directory. The directories are per subsystems (hardware
>> classes).
> 
> Apologies if I was not clear, I meant to say this is a device conceptually
> similar to virtio. But this driver has nothing to do with virtio, we should

Bindings are for hardware, not drivers, so if the device serves the same
purpose, it's driver differences do not matter.

> be creating a new folder for it OR I am fine moving it under bus if that's
> okay.

Since you do not have children here, it's not really a bus to fit under
bus directory...

Probably this should go together with virtio bindings to dedicated
hypervisor interfaces directory. We do not create directories for
specific solutions (implementations) with only one or few bindings.
Directories are for entire classes.

Best regards,
Krzysztof

Re: [PATCH 4/4] dt-bindings: hv: Add dt-bindings for VMBus

[Saurabh Singh Sengar]

On Fri, Jan 20, 2023 at 12:43:40PM +0100, Krzysztof Kozlowski wrote:
> On 17/01/2023 16:52, Saurabh Singh Sengar wrote:
> > On Tue, Jan 17, 2023 at 04:41:22PM +0100, Krzysztof Kozlowski wrote:
> >> On 17/01/2023 16:13, Saurabh Singh Sengar wrote:
> >>> On Mon, Jan 16, 2023 at 07:55:13PM +0100, Krzysztof Kozlowski wrote:
> >>>> On 16/01/2023 17:48, Saurabh Sengar wrote:
> >>>>> Add dt-bindings for Hyper-V VMBus
> >>>>>
> >>>>> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> >>>>> ---
> >>>>>  .../devicetree/bindings/hv/msft,vmbus.yaml         | 34 ++++++++++++++++++++++
> >>>>
> >>>> Also, there is no "hv" hardware, so that's not correct location. If your
> >>>> bindings describe firmware, this should go to firmware. Otherwise, this
> >>>> does not look like suitable for DT. We do not describe software stuff in DT.
> >>>
> >>> VMBus is a virtual device this is simmilar to virtio. I can rename this folder to vmbus.
> >>>
> >>
> >> Then virtio directory. The directories are per subsystems (hardware
> >> classes).
> > 
> > Apologies if I was not clear, I meant to say this is a device conceptually
> > similar to virtio. But this driver has nothing to do with virtio, we should
> 
> Bindings are for hardware, not drivers, so if the device serves the same
> purpose, it's driver differences do not matter.
> 
> > be creating a new folder for it OR I am fine moving it under bus if that's
> > okay.
> 
> Since you do not have children here, it's not really a bus to fit under
> bus directory...
> 
> Probably this should go together with virtio bindings to dedicated
> hypervisor interfaces directory. We do not create directories for
> specific solutions (implementations) with only one or few bindings.
> Directories are for entire classes.

I am OK to keep it anywhere, but I believe virtio is not its correct place. I am also
concern how will the virtio maintainers will perceive it. Ideally we should be renaming
virtio to virtualization OR hypervisor OR something more generic where both virtio and
VMBus can co-exist. Please let me know if renaming virtio is acceptable.

> 
> Best regards,
> Krzysztof

Re: [PATCH 4/4] dt-bindings: hv: Add dt-bindings for VMBus

[Krzysztof Kozlowski]

On 20/01/2023 13:51, Saurabh Singh Sengar wrote:
> On Fri, Jan 20, 2023 at 12:43:40PM +0100, Krzysztof Kozlowski wrote:
>> On 17/01/2023 16:52, Saurabh Singh Sengar wrote:
>>> On Tue, Jan 17, 2023 at 04:41:22PM +0100, Krzysztof Kozlowski wrote:
>>>> On 17/01/2023 16:13, Saurabh Singh Sengar wrote:
>>>>> On Mon, Jan 16, 2023 at 07:55:13PM +0100, Krzysztof Kozlowski wrote:
>>>>>> On 16/01/2023 17:48, Saurabh Sengar wrote:
>>>>>>> Add dt-bindings for Hyper-V VMBus
>>>>>>>
>>>>>>> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/hv/msft,vmbus.yaml         | 34 ++++++++++++++++++++++
>>>>>>
>>>>>> Also, there is no "hv" hardware, so that's not correct location. If your
>>>>>> bindings describe firmware, this should go to firmware. Otherwise, this
>>>>>> does not look like suitable for DT. We do not describe software stuff in DT.
>>>>>
>>>>> VMBus is a virtual device this is simmilar to virtio. I can rename this folder to vmbus.
>>>>>
>>>>
>>>> Then virtio directory. The directories are per subsystems (hardware
>>>> classes).
>>>
>>> Apologies if I was not clear, I meant to say this is a device conceptually
>>> similar to virtio. But this driver has nothing to do with virtio, we should
>>
>> Bindings are for hardware, not drivers, so if the device serves the same
>> purpose, it's driver differences do not matter.
>>
>>> be creating a new folder for it OR I am fine moving it under bus if that's
>>> okay.
>>
>> Since you do not have children here, it's not really a bus to fit under
>> bus directory...
>>
>> Probably this should go together with virtio bindings to dedicated
>> hypervisor interfaces directory. We do not create directories for
>> specific solutions (implementations) with only one or few bindings.
>> Directories are for entire classes.
> 
> I am OK to keep it anywhere, but I believe virtio is not its correct place. I am also
> concern how will the virtio maintainers will perceive it. Ideally we should be renaming
> virtio to virtualization OR hypervisor OR something more generic where both virtio and
> VMBus can co-exist. Please let me know if renaming virtio is acceptable.

Yes, that's what I was thinking about. I think all of these should be in
one place, but named differently (with updates to MAINTAINERS place).

Best regards,
Krzysztof

Re: [PATCH 3/4] Drivers: hv: vmbus: Device Tree support

[Saurabh Singh Sengar]

On Mon, Jan 16, 2023 at 07:48:25PM +0100, Krzysztof Kozlowski wrote:
> On 16/01/2023 17:48, Saurabh Sengar wrote:
> > Update the driver to use vmbus_root_dev device instead of acpi_device,
> > which can be assigned to either ACPI or OF device, making VMBus agnostic
> > to whether the device is using ACPI or device tree.
> > 
> 
> (...)
> 
> >  
> >  static void vmbus_reserve_fb(void)
> > @@ -2319,8 +2322,9 @@ static void vmbus_reserve_fb(void)
> >  	 * reserving a larger area and make it smaller until it succeeds.
> >  	 */
> >  	for (; !fb_mmio && (size >= 0x100000); size >>= 1)
> > -		fb_mmio = __request_region(hyperv_mmio, start, size, fb_mmio_name, 0);
> > +		fb_mmio = __request_region(hyperv_mmio, start, size, "fb_range", 0);
> 
> Your patch is doing much more than just adding OF. Adding OF is usually
> just few lines, so this means you are refactoring driver and all this
> work should be split to self-contained patches.

did a split in v2.

> 
> >  }
> > +#endif /* CONFIG_ACPI */
> >  
> >  /**
> >   * vmbus_allocate_mmio() - Pick a memory-mapped I/O range.
> > @@ -2441,13 +2445,14 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size)
> >  }
> >  EXPORT_SYMBOL_GPL(vmbus_free_mmio);
> >  
> > +#ifdef CONFIG_ACPI
> >  static int vmbus_acpi_add(struct acpi_device *device)
> >  {
> >  	acpi_status result;
> >  	int ret_val = -ENODEV;
> >  	struct acpi_device *ancestor;
> >  
> > -	hv_acpi_dev = device;
> > +	vmbus_root_dev = &device->dev;
> >  
> >  	/*
> >  	 * Older versions of Hyper-V for ARM64 fail to include the _CCA
> > @@ -2492,6 +2497,72 @@ static int vmbus_acpi_add(struct acpi_device *device)
> >  		vmbus_acpi_remove(device);
> >  	return ret_val;
> >  }
> > +#endif
> > +
> > +#ifdef CONFIG_OF
> > +static int vmbus_of_driver_probe(struct platform_device *dev)
> > +{
> > +	struct resource **cur_res = &hyperv_mmio;
> > +	struct device_node *np;
> > +	const __be32 *ranges;
> > +	u32 nr_addr, nr_size, nr_parent_addr_cells, nr_ranges;
> > +	u32 range_len, range_size;
> > +	int i;
> > +
> > +	vmbus_root_dev = &dev->dev;
> > +	np = vmbus_root_dev->of_node;
> > +
> > +	if (of_property_read_u32(np, "#address-cells", &nr_addr))
> > +		return -ENOENT;
> > +	if (of_property_read_u32(np, "#size-cells", &nr_size))
> > +		return -ENOENT;
> > +	nr_parent_addr_cells = of_n_addr_cells(np);
> > +
> > +	if (nr_parent_addr_cells != 2 || nr_addr != 2 || nr_size != 1) {
> > +		pr_err("Address format is not supported\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	ranges = of_get_property(np, "ranges", &range_len);
> > +	if (!ranges)
> > +		return -ENOENT;
> > +
> > +	range_size = nr_parent_addr_cells + nr_addr + nr_size; // in cells
> > +	nr_ranges = range_len / sizeof(__be32) / range_size;
> > +
> > +	for (i = 0; i < nr_ranges; ++i, ranges += range_size) {
> > +		struct resource *res;
> > +		/*
> > +		 * The first u64 in the ranges description isn't used currently.
> > +		 * u64 _ = of_read_number(ranges, nr_parent_addr_cells);
> > +		 */
> > +		u64 start = of_read_number(ranges + nr_parent_addr_cells, nr_addr);
> > +		u32 len = of_read_number(ranges + nr_parent_addr_cells + nr_addr, nr_size);
> > +
> > +		pr_debug("VMBUS DeviceTree MMIO region start %#llx, %#x\n", start, len);
> 
> You must not print kernel or IO space addresses. You could use some
> printk formats to hide the address, if this is really needed.

removed in v2

> 
> > +
> > +		res = kzalloc(sizeof(*res), GFP_ATOMIC);
> > +		if (!res)
> > +			return -ENOMEM;
> > +
> > +		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;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int vmbus_of_driver_remove(struct platform_device *dev)
> > +{
> > +	vmbus_remove_mmio();
> > +	return 0;
> > +}
> > +#endif
> >  
> >  #ifdef CONFIG_PM_SLEEP
> >  static int vmbus_bus_suspend(struct device *dev)
> > @@ -2630,6 +2701,9 @@ static int vmbus_bus_resume(struct device *dev)
> >  #define vmbus_bus_resume NULL
> >  #endif /* CONFIG_PM_SLEEP */
> >  
> > +#define DRV_NAME "vmbus"
> > +
> > +#ifdef CONFIG_ACPI
> >  static const struct acpi_device_id vmbus_acpi_device_ids[] = {
> >  	{"VMBUS", 0},
> >  	{"VMBus", 0},
> > @@ -2659,7 +2733,7 @@ static int vmbus_bus_resume(struct device *dev)
> >  };
> >  
> >  static struct acpi_driver vmbus_acpi_driver = {
> > -	.name = "vmbus",
> > +	.name = DRV_NAME,
> 
> How this is related?

removed in v2

> 
> >  	.ids = vmbus_acpi_device_ids,
> >  	.ops = {
> >  		.add = vmbus_acpi_add,
> > @@ -2669,6 +2743,7 @@ static int vmbus_bus_resume(struct device *dev)
> >  	.drv.probe_type = PROBE_FORCE_SYNCHRONOUS,
> >  };
> >  
> > +#endif
> >  static void hv_kexec_handler(void)
> >  {
> >  	hv_stimer_global_cleanup();
> > @@ -2737,7 +2812,32 @@ static void hv_synic_resume(void)
> >  	.resume = hv_synic_resume,
> >  };
> >  
> > -static int __init hv_acpi_init(void)
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id vmbus_of_match[] = {
> > +	{
> > +		.name = "msft,vmbus",
> 
> Why do you need name?

removed in v2.

> 
> > +		.compatible = "msft,vmbus",
> > +		.data = NULL
> 
> Why do you need data field?

removed in v2

> 
> > +	},
> > +	{
> > +		/* sentinel */
> > +	},
> > +};
> > +MODULE_DEVICE_TABLE(of, vmbus_of_match);
> > +
> > +static struct platform_driver vmbus_platform_driver = {
> > +	.probe = vmbus_of_driver_probe,
> > +	.remove = vmbus_of_driver_remove,
> > +	.driver = {
> > +		.name = DRV_NAME,
> > +		.of_match_table = of_match_ptr(vmbus_of_match),
> > +		.pm = &vmbus_pm,
> > +		.bus = &hv_bus,
> > +	}
> > +};
> > +#endif
> 
> Why platform driver is hidden by CONFIG_OF? It should not be the case.
> The interface - ACPI or OF - should not differ for driver
> infrastructure. Even one probe could be used - just drop all of_...
> methods and use generic device_property_

Changed the driver to platform driver as suggested and used common probe
in v2. Also using device_property_ instead of of_property.

> > +
> > +static int __init vmbus_init(void)
> >  {
> >  	int ret;
> >  
> > @@ -2747,18 +2847,27 @@ static int __init hv_acpi_init(void)
> >  	if (hv_root_partition && !hv_nested)
> >  		return 0;
> >  
> > +#ifdef CONFIG_ACPI
> >  	/*
> > -	 * Get ACPI resources first.
> > +	 * Request ACPI resources and wait for the completion
> >  	 */
> >  	ret = acpi_bus_register_driver(&vmbus_acpi_driver);
> >  
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (!hv_acpi_dev) {
> > -		ret = -ENODEV;
> > +	if (!vmbus_root_dev) {
> > +		ret = -ETIMEDOUT;
> >  		goto cleanup;
> >  	}
> > +#endif
> > +#ifdef CONFIG_OF
> > +	ret = platform_driver_register(&vmbus_platform_driver);
> > +	if (ret) {
> > +		pr_err("Error registering platform resources: %d\n", ret);
> > +		goto cleanup;
> > +	}
> > +#endif
> >  
> >  	/*
> >  	 * If we're on an architecture with a hardcoded hypervisor
> > @@ -2785,8 +2894,14 @@ static int __init hv_acpi_init(void)
> >  	return 0;
> >  
> >  cleanup:
> > +#ifdef CONFIG_ACPI
> >  	acpi_bus_unregister_driver(&vmbus_acpi_driver);
> > -	hv_acpi_dev = NULL;
> > +#endif
> > +#ifdef CONFIG_OF
> > +	platform_driver_unregister(&vmbus_platform_driver);
> > +#endif
> > +	vmbus_root_dev = NULL;
> > +
> >  	return ret;
> >  }
> >  
> > @@ -2839,12 +2954,17 @@ static void __exit vmbus_exit(void)
> >  
> >  	cpuhp_remove_state(hyperv_cpuhp_online);
> >  	hv_synic_free();
> > +#ifdef CONFIG_ACPI
> >  	acpi_bus_unregister_driver(&vmbus_acpi_driver);
> > +#endif
> > +#ifdef CONFIG_OF
> > +	platform_driver_unregister(&vmbus_platform_driver);
> > +#endif
> > +	vmbus_root_dev = NULL;
> >  }
> >  
> > -
> 
> This is really a messy patch...

fixed in v2 with changing this driver to platform.

> 
> >  MODULE_LICENSE("GPL");
> >  MODULE_DESCRIPTION("Microsoft Hyper-V VMBus Driver");
> >  
> > -subsys_initcall(hv_acpi_init);
> > +subsys_initcall(vmbus_init);
> >  module_exit(vmbus_exit);
> 
> Best regards,
> Krzysztof

Re: [PATCH 4/4] dt-bindings: hv: Add dt-bindings for VMBus

[Saurabh Singh Sengar]

On Mon, Jan 16, 2023 at 07:53:07PM +0100, Krzysztof Kozlowski wrote:
> On 16/01/2023 17:48, Saurabh Sengar wrote:
> > Add dt-bindings for Hyper-V VMBus
> 
> Missing full stop.
> 
> Subject: drop second/last, redundant "dt-bindings for". The
> "dt-bindings" prefix is already stating that these are bindings.

Will fix in v3.

> 
> > 
> > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > ---
> >  .../devicetree/bindings/hv/msft,vmbus.yaml         | 34 ++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/hv/msft,vmbus.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/hv/msft,vmbus.yaml b/Documentation/devicetree/bindings/hv/msft,vmbus.yaml
> > new file mode 100644
> > index 0000000..66cb426
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hv/msft,vmbus.yaml
> > @@ -0,0 +1,34 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/hv/msft,vmbus.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Microsoft Hyper-V VMBus device tree bindings
> 
> Drop "device tree bindings"

Will fix in v3

> 
> > +
> > +maintainers:
> > +  - Saurabh Sengar <ssengar@linux.microsoft.com>
> > +
> > +description:
> > +  VMBus is a software bus that implement the protocols for communication
> > +  between the root or host OS and guest OSs (virtual machines).
> 
> Why this cannot be auto-discoverable? Why do you need OF for this?

This is a virtulization device, and I guess we have discussed this in greater
length in other thread.

> 
> > +
> > +properties:
> > +  compatible:
> > +    const: msft,vmbus
> > +
> > +  ranges :
> > +    const: <0x00 0x00 0x0f 0xf0000000 0x10000000>
> 
> Did you test the bindings?
> 
> This property does not look correct. If you have static addresses, you
> do not need OF. What do you want to discover here?

fixed in v2

> 
> > +
> > +required:
> > +  - compatible
> > +  - ranges
> > +
> > +examples:
> > +  - |
> > +        vmbus {
> 
> Use 4 spaces for example indentation.

Fix in v2

> 
> > +		#address-cells = <0x02>;
> > +		#size-cells = <0x01>;
> 
> That's not correct style. Drop hex notation. Drop leading zeros.

Will fix in v3

> 
> But anyway you did not test the bindings. This cannot work. Try.
> 
> > +		compatible = "msft,vmbus";
> 
> compatible is a first property.

fixed in v2

> 
> > +		ranges = <0x00 0x00 0x0f 0xf0000000 0x10000000>;
> 
> What do you translate? There is no reg, no unit address.

Commented on v2 thread, if there is any further concern using ranges
please let me know.

> 
> > +	};
> 
> Best regards,
> Krzysztof