2023-01-31 18:10:23

by Saurabh Singh Sengar

[permalink] [raw]
Subject: [PATCH v2 0/6] Device tree support for Hyper-V VMBus driver

This set of patches expands the VMBus driver to include device tree
support.

The first two patches enable compilation of Hyper-V APIs in a non-ACPI
build.

The third patch converts the VMBus driver from acpi to more generic
platform driver.

Further to add device tree documentation for VMBus, it needs to club with
other virtualization driver's documentation. For this rename the virtio
folder to more generic hypervisor, so that all the hypervisor based
devices can co-exist in a single place in device tree documentation. The
fourth patch does this renaming.

The fifth patch introduces the device tree documentation for VMBus.

The sixth patch adds device tree support to the VMBus driver. Currently
this is tested only for x86 and it may not work for other archs.

[v2]
- Convert VMBus acpi device to platform device, and added device tree support
in separate patch. This enables using same driver structure for both the flows.
- In Device tree documentation, changed virtio folder to hypervisor and moved
VMBus documentation there.
- Moved bindings before Device tree patch.
- Removed stale ".data" and ".name" field from of_device match table.
- Removed debug print.
- Fix "make DT_CHECKER_FLAGS=-m dt_binding_check" warnings.

Saurabh Sengar (6):
drivers/clocksource/hyper-v: non ACPI support in hyperv clock
Drivers: hv: allow non ACPI compilation for
hv_is_hibernation_supported
Drivers: hv: vmbus: Convert acpi_device to platform_device
dt-bindings: hypervisor: Rename virtio to hypervisor
dt-bindings: hypervisor: Add dt-bindings for VMBus
Driver: VMBus: Add device tree support

.../bindings/{virtio => hypervisor}/mmio.yaml | 2 +-
.../bindings/hypervisor/msft,vmbus.yaml | 50 ++++++
.../{virtio => hypervisor}/pci-iommu.yaml | 2 +-
.../{virtio => hypervisor}/virtio-device.yaml | 2 +-
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
MAINTAINERS | 3 +-
drivers/clocksource/hyperv_timer.c | 15 +-
drivers/hv/hv_common.c | 4 +
drivers/hv/vmbus_drv.c | 153 ++++++++++++++----
9 files changed, 193 insertions(+), 40 deletions(-)
rename Documentation/devicetree/bindings/{virtio => hypervisor}/mmio.yaml (95%)
create mode 100644 Documentation/devicetree/bindings/hypervisor/msft,vmbus.yaml
rename Documentation/devicetree/bindings/{virtio => hypervisor}/pci-iommu.yaml (98%)
rename Documentation/devicetree/bindings/{virtio => hypervisor}/virtio-device.yaml (93%)

--
2.25.1



2023-01-31 18:10:28

by Saurabh Singh Sengar

[permalink] [raw]
Subject: [PATCH v2 3/6] Drivers: hv: vmbus: Convert acpi_device to platform_device

Use more generic platform device instead of acpi device. Also rename the
function vmbus_acpi_remove to more generic name vmbus_mmio_remove.

Signed-off-by: Saurabh Sengar <[email protected]>
---
drivers/hv/vmbus_drv.c | 78 +++++++++++++++++++++++++-----------------
1 file changed, 46 insertions(+), 32 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index d24dd65b33d4..49030e756b9f 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -12,6 +12,7 @@
#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>
@@ -44,7 +45,7 @@ struct vmbus_dynid {
struct hv_vmbus_device_id id;
};

-static struct acpi_device *hv_acpi_dev;
+static struct platform_device *hv_dev;

static int hyperv_cpuhp_online;

@@ -143,7 +144,7 @@ static DEFINE_MUTEX(hyperv_mmio_lock);

static int vmbus_exists(void)
{
- if (hv_acpi_dev == NULL)
+ if (hv_dev == NULL)
return -ENODEV;

return 0;
@@ -932,7 +933,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(&hv_dev->dev) == DEV_DMA_COHERENT);
return 0;
}

@@ -2090,7 +2091,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 = &hv_dev->dev;
child_device_obj->device.release = vmbus_device_release;

child_device_obj->device.dma_parms = &child_device_obj->dma_parms;
@@ -2262,7 +2263,7 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
return AE_OK;
}

-static void vmbus_acpi_remove(struct acpi_device *device)
+static void vmbus_mmio_remove(void)
{
struct resource *cur_res;
struct resource *next_res;
@@ -2441,13 +2442,15 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size)
}
EXPORT_SYMBOL_GPL(vmbus_free_mmio);

-static int vmbus_acpi_add(struct acpi_device *device)
+static int vmbus_acpi_add(struct platform_device *pdev)
{
acpi_status result;
int ret_val = -ENODEV;
- struct acpi_device *ancestor;
+ struct platform_device *ancestor;
+ struct acpi_device *adev = to_acpi_device(&pdev->dev);

- hv_acpi_dev = device;
+ hv_dev = pdev;
+ adev->fwnode.dev = &pdev->dev;

/*
* Older versions of Hyper-V for ARM64 fail to include the _CCA
@@ -2456,15 +2459,16 @@ static int vmbus_acpi_add(struct acpi_device *device)
* up the ACPI device to behave as if _CCA is present and indicates
* hardware coherence.
*/
- ACPI_COMPANION_SET(&device->dev, device);
+ ACPI_COMPANION_SET(&pdev->dev, ACPI_COMPANION(&pdev->dev));
if (IS_ENABLED(CONFIG_ACPI_CCA_REQUIRED) &&
- device_get_dma_attr(&device->dev) == DEV_DMA_NOT_SUPPORTED) {
+ device_get_dma_attr(&pdev->dev) == DEV_DMA_NOT_SUPPORTED) {
+ struct acpi_device *adev_node = ACPI_COMPANION(&pdev->dev);
pr_info("No ACPI _CCA found; assuming coherent device I/O\n");
- device->flags.cca_seen = true;
- device->flags.coherent_dma = true;
+ adev_node->flags.cca_seen = true;
+ adev_node->flags.coherent_dma = true;
}

- result = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
+ result = acpi_walk_resources(ACPI_HANDLE(&pdev->dev), METHOD_NAME__CRS,
vmbus_walk_resources, NULL);

if (ACPI_FAILURE(result))
@@ -2473,9 +2477,9 @@ static int vmbus_acpi_add(struct acpi_device *device)
* Some ancestor of the vmbus acpi device (Gen1 or Gen2
* firmware) is the VMOD that has the mmio ranges. Get that.
*/
- for (ancestor = acpi_dev_parent(device); ancestor;
- ancestor = acpi_dev_parent(ancestor)) {
- result = acpi_walk_resources(ancestor->handle, METHOD_NAME__CRS,
+ for (ancestor = to_platform_device(pdev->dev.parent); ancestor;
+ ancestor = to_platform_device(ancestor->dev.parent)) {
+ result = acpi_walk_resources(ACPI_HANDLE(&ancestor->dev), METHOD_NAME__CRS,
vmbus_walk_resources, NULL);

if (ACPI_FAILURE(result))
@@ -2489,10 +2493,21 @@ static int vmbus_acpi_add(struct acpi_device *device)

acpi_walk_err:
if (ret_val)
- vmbus_acpi_remove(device);
+ vmbus_mmio_remove();
return ret_val;
}

+static int vmbus_platform_driver_probe(struct platform_device *pdev)
+{
+ return vmbus_acpi_add(pdev);
+}
+
+static int vmbus_platform_driver_remove(struct platform_device *pdev)
+{
+ vmbus_mmio_remove();
+ return 0;
+}
+
#ifdef CONFIG_PM_SLEEP
static int vmbus_bus_suspend(struct device *dev)
{
@@ -2658,15 +2673,15 @@ static const struct dev_pm_ops vmbus_bus_pm = {
.restore_noirq = vmbus_bus_resume
};

-static struct acpi_driver vmbus_acpi_driver = {
- .name = "vmbus",
- .ids = vmbus_acpi_device_ids,
- .ops = {
- .add = vmbus_acpi_add,
- .remove = vmbus_acpi_remove,
- },
- .drv.pm = &vmbus_bus_pm,
- .drv.probe_type = PROBE_FORCE_SYNCHRONOUS,
+static struct platform_driver vmbus_platform_driver = {
+ .probe = vmbus_platform_driver_probe,
+ .remove = vmbus_platform_driver_remove,
+ .driver = {
+ .name = "vmbus",
+ .acpi_match_table = ACPI_PTR(vmbus_acpi_device_ids),
+ .pm = &vmbus_bus_pm,
+ .probe_type = PROBE_FORCE_SYNCHRONOUS,
+ }
};

static void hv_kexec_handler(void)
@@ -2750,12 +2765,11 @@ static int __init hv_acpi_init(void)
/*
* Get ACPI resources first.
*/
- ret = acpi_bus_register_driver(&vmbus_acpi_driver);
-
+ ret = platform_driver_register(&vmbus_platform_driver);
if (ret)
return ret;

- if (!hv_acpi_dev) {
+ if (!hv_dev) {
ret = -ENODEV;
goto cleanup;
}
@@ -2785,8 +2799,8 @@ static int __init hv_acpi_init(void)
return 0;

cleanup:
- acpi_bus_unregister_driver(&vmbus_acpi_driver);
- hv_acpi_dev = NULL;
+ platform_driver_unregister(&vmbus_platform_driver);
+ hv_dev = NULL;
return ret;
}

@@ -2839,7 +2853,7 @@ static void __exit vmbus_exit(void)

cpuhp_remove_state(hyperv_cpuhp_online);
hv_synic_free();
- acpi_bus_unregister_driver(&vmbus_acpi_driver);
+ platform_driver_unregister(&vmbus_platform_driver);
}


--
2.25.1


2023-01-31 18:10:31

by Saurabh Singh Sengar

[permalink] [raw]
Subject: [PATCH v2 6/6] Driver: VMBus: Add device tree support

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 <[email protected]>
---
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
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");
+ 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++];
+
+ 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


2023-01-31 18:10:34

by Saurabh Singh Sengar

[permalink] [raw]
Subject: [PATCH v2 5/6] dt-bindings: hypervisor: Add dt-bindings for VMBus

Add dt-bindings for Hyper-V VMBus

Signed-off-by: Saurabh Sengar <[email protected]>
---
.../bindings/hypervisor/msft,vmbus.yaml | 50 +++++++++++++++++++
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
MAINTAINERS | 1 +
3 files changed, 53 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hypervisor/msft,vmbus.yaml

diff --git a/Documentation/devicetree/bindings/hypervisor/msft,vmbus.yaml b/Documentation/devicetree/bindings/hypervisor/msft,vmbus.yaml
new file mode 100644
index 000000000000..8f50d6097c48
--- /dev/null
+++ b/Documentation/devicetree/bindings/hypervisor/msft,vmbus.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hypervisor/msft,vmbus.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microsoft Hyper-V VMBus device tree bindings
+
+maintainers:
+ - Saurabh Sengar <[email protected]>
+
+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: true
+
+ '#address-cells':
+ const: 2
+
+ '#size-cells':
+ const: 1
+
+required:
+ - compatible
+ - ranges
+ - '#address-cells'
+ - '#size-cells'
+
+additionalProperties: false
+
+examples:
+ - |
+ / {
+ compatible = "foo";
+ model = "foo";
+ #address-cells = <0x02>;
+ #size-cells = <0x02>;
+
+ vmbus@ff0000000 {
+ #address-cells = <0x02>;
+ #size-cells = <0x01>;
+ compatible = "msft,vmbus";
+ ranges = <0x00 0x00 0x0f 0xf0000000 0x10000000>;
+ };
+ };
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 1f7a519a936f..ab74ea97535f 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -876,6 +876,8 @@ patternProperties:
deprecated: true
"^mscc,.*":
description: Microsemi Corporation
+ "^msft,.*":
+ description: Microsoft Corporation
"^msi,.*":
description: Micro-Star International Co. Ltd.
"^mstar,.*":
diff --git a/MAINTAINERS b/MAINTAINERS
index 9bc5b88b723a..c749782b0cff 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9496,6 +9496,7 @@ S: Supported
T: git git://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git
F: Documentation/ABI/stable/sysfs-bus-vmbus
F: Documentation/ABI/testing/debugfs-hyperv
+F: Documentation/devicetree/bindings/hypervisor/msft,vmbus.yaml
F: Documentation/virt/hyperv
F: Documentation/networking/device_drivers/ethernet/microsoft/netvsc.rst
F: arch/arm64/hyperv
--
2.25.1


2023-01-31 18:10:37

by Saurabh Singh Sengar

[permalink] [raw]
Subject: [PATCH v2 4/6] dt-bindings: hypervisor: Rename virtio to hypervisor

Rename virtio folder to more generic hypervisor, so that this can
accommodate more devices of similar type.

Signed-off-by: Saurabh Sengar <[email protected]>
---
.../devicetree/bindings/{virtio => hypervisor}/mmio.yaml | 2 +-
.../devicetree/bindings/{virtio => hypervisor}/pci-iommu.yaml | 2 +-
.../bindings/{virtio => hypervisor}/virtio-device.yaml | 2 +-
MAINTAINERS | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
rename Documentation/devicetree/bindings/{virtio => hypervisor}/mmio.yaml (95%)
rename Documentation/devicetree/bindings/{virtio => hypervisor}/pci-iommu.yaml (98%)
rename Documentation/devicetree/bindings/{virtio => hypervisor}/virtio-device.yaml (93%)

diff --git a/Documentation/devicetree/bindings/virtio/mmio.yaml b/Documentation/devicetree/bindings/hypervisor/mmio.yaml
similarity index 95%
rename from Documentation/devicetree/bindings/virtio/mmio.yaml
rename to Documentation/devicetree/bindings/hypervisor/mmio.yaml
index 0aa8433f0a5e..8492c9b4fec9 100644
--- a/Documentation/devicetree/bindings/virtio/mmio.yaml
+++ b/Documentation/devicetree/bindings/hypervisor/mmio.yaml
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
%YAML 1.2
---
-$id: http://devicetree.org/schemas/virtio/mmio.yaml#
+$id: http://devicetree.org/schemas/hypervisor/mmio.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#

title: virtio memory mapped devices
diff --git a/Documentation/devicetree/bindings/virtio/pci-iommu.yaml b/Documentation/devicetree/bindings/hypervisor/pci-iommu.yaml
similarity index 98%
rename from Documentation/devicetree/bindings/virtio/pci-iommu.yaml
rename to Documentation/devicetree/bindings/hypervisor/pci-iommu.yaml
index 972a785a42de..52de535fd4ef 100644
--- a/Documentation/devicetree/bindings/virtio/pci-iommu.yaml
+++ b/Documentation/devicetree/bindings/hypervisor/pci-iommu.yaml
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
%YAML 1.2
---
-$id: http://devicetree.org/schemas/virtio/pci-iommu.yaml#
+$id: http://devicetree.org/schemas/hypervisor/pci-iommu.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#

title: virtio-iommu device using the virtio-pci transport
diff --git a/Documentation/devicetree/bindings/virtio/virtio-device.yaml b/Documentation/devicetree/bindings/hypervisor/virtio-device.yaml
similarity index 93%
rename from Documentation/devicetree/bindings/virtio/virtio-device.yaml
rename to Documentation/devicetree/bindings/hypervisor/virtio-device.yaml
index 8c6919ba9497..7b8d93b06237 100644
--- a/Documentation/devicetree/bindings/virtio/virtio-device.yaml
+++ b/Documentation/devicetree/bindings/hypervisor/virtio-device.yaml
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
%YAML 1.2
---
-$id: http://devicetree.org/schemas/virtio/virtio-device.yaml#
+$id: http://devicetree.org/schemas/hypervisor/virtio-device.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#

title: Virtio device
diff --git a/MAINTAINERS b/MAINTAINERS
index 263d37a129b1..9bc5b88b723a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21967,7 +21967,7 @@ L: [email protected]
S: Maintained
F: Documentation/ABI/testing/sysfs-bus-vdpa
F: Documentation/ABI/testing/sysfs-class-vduse
-F: Documentation/devicetree/bindings/virtio/
+F: Documentation/devicetree/bindings/hypervisor/
F: Documentation/driver-api/virtio/
F: drivers/block/virtio_blk.c
F: drivers/crypto/virtio/
--
2.25.1


2023-01-31 18:51:29

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 5/6] dt-bindings: hypervisor: Add dt-bindings for VMBus

On 31/01/2023 19:10, Saurabh Sengar wrote:
> Add dt-bindings for Hyper-V VMBus
>
> Signed-off-by: Saurabh Sengar <[email protected]>
> ---
> .../bindings/hypervisor/msft,vmbus.yaml | 50 +++++++++++++++++++
> .../devicetree/bindings/vendor-prefixes.yaml | 2 +
> MAINTAINERS | 1 +
> 3 files changed, 53 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hypervisor/msft,vmbus.yaml
>
> diff --git a/Documentation/devicetree/bindings/hypervisor/msft,vmbus.yaml b/Documentation/devicetree/bindings/hypervisor/msft,vmbus.yaml
> new file mode 100644
> index 000000000000..8f50d6097c48
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hypervisor/msft,vmbus.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hypervisor/msft,vmbus.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microsoft Hyper-V VMBus device tree bindings

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

(other places as well...)

Best regards,
Krzysztof


2023-01-31 18:55:12

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 5/6] dt-bindings: hypervisor: Add dt-bindings for VMBus

On 31/01/2023 19:10, Saurabh Sengar wrote:
>
> Signed-off-by: Saurabh Sengar <[email protected]>

> + - |
> + / {
> + compatible = "foo";
> + model = "foo";
> + #address-cells = <0x02>;
> + #size-cells = <0x02>;

Except previous comments (all of them were ignored), also:
Drop entire part. Not related, not correct, not helping and you cannot
have top level nodes in example.

> +
> + vmbus@ff0000000 {
> + #address-cells = <0x02>;
> + #size-cells = <0x01>;
> + compatible = "msft,vmbus";
> + ranges = <0x00 0x00 0x0f 0xf0000000 0x10000000>;
> + };
> + };
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 1f7a519a936f..ab74ea97535f 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -876,6 +876,8 @@ patternProperties:
> deprecated: true
> "^mscc,.*":
> description: Microsemi Corporation
> + "^msft,.*":
> + description: Microsoft Corporation

Don't duplicate vendor prefixes. Drop and use correct prefix.

Best regards,
Krzysztof


2023-01-31 19:57:42

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v2 4/6] dt-bindings: hypervisor: Rename virtio to hypervisor


On Tue, 31 Jan 2023 10:10:07 -0800, Saurabh Sengar wrote:
> Rename virtio folder to more generic hypervisor, so that this can
> accommodate more devices of similar type.
>
> Signed-off-by: Saurabh Sengar <[email protected]>
> ---
> .../devicetree/bindings/{virtio => hypervisor}/mmio.yaml | 2 +-
> .../devicetree/bindings/{virtio => hypervisor}/pci-iommu.yaml | 2 +-
> .../bindings/{virtio => hypervisor}/virtio-device.yaml | 2 +-
> MAINTAINERS | 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
> rename Documentation/devicetree/bindings/{virtio => hypervisor}/mmio.yaml (95%)
> rename Documentation/devicetree/bindings/{virtio => hypervisor}/pci-iommu.yaml (98%)
> rename Documentation/devicetree/bindings/{virtio => hypervisor}/virtio-device.yaml (93%)
>

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:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/i2c/i2c-virtio.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/virtio/virtio-device.yaml
./Documentation/devicetree/bindings/gpio/gpio-virtio.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/virtio/virtio-device.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/i2c-virtio.example.dtb: i2c: False schema does not allow {'compatible': ['virtio,device22'], '#address-cells': [[1]], '#size-cells': [[0]], 'light-sensor@20': {'compatible': ['dynaimage,al3320a'], 'reg': [[32]]}, '$nodename': ['i2c']}
From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/gpio/gpio-virtio.example.dtb: gpio: False schema does not allow {'compatible': ['virtio,device29'], 'gpio-controller': True, '#gpio-cells': [[2]], 'interrupt-controller': True, '#interrupt-cells': [[2]], '$nodename': ['gpio']}
From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/gpio/gpio-virtio.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hypervisor/virtio-device.example.dtb: i2c: False schema does not allow {'compatible': ['virtio,device22'], '$nodename': ['i2c']}
From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml

doc reference errors (make refcheckdocs):
MAINTAINERS: Documentation/devicetree/bindings/virtio/

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]

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.


2023-01-31 20:13:14

by Rob Herring

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

On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar
<[email protected]> 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 <[email protected]>
> ---
> 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
>

2023-01-31 20:28:20

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH v2 0/6] Device tree support for Hyper-V VMBus driver

On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar
<[email protected]> wrote:
>
> This set of patches expands the VMBus driver to include device tree
> support.
>
> The first two patches enable compilation of Hyper-V APIs in a non-ACPI
> build.
>
> The third patch converts the VMBus driver from acpi to more generic
> platform driver.
>
> Further to add device tree documentation for VMBus, it needs to club with
> other virtualization driver's documentation. For this rename the virtio
> folder to more generic hypervisor, so that all the hypervisor based
> devices can co-exist in a single place in device tree documentation. The
> fourth patch does this renaming.
>
> The fifth patch introduces the device tree documentation for VMBus.
>
> The sixth patch adds device tree support to the VMBus driver. Currently
> this is tested only for x86 and it may not work for other archs.

I can read all the patches and see *what* they do. You don't really
need to list that here. I'm still wondering *why*. That is what the
cover letter and commit messages should answer. Why do you need DT
support? How does this even work on x86? FDT is only enabled for
CE4100 platform.

Rob

2023-02-01 01:39:37

by Saurabh Singh Sengar

[permalink] [raw]
Subject: Re: [PATCH v2 5/6] dt-bindings: hypervisor: Add dt-bindings for VMBus

On Tue, Jan 31, 2023 at 07:51:20PM +0100, Krzysztof Kozlowski wrote:
> On 31/01/2023 19:10, Saurabh Sengar wrote:
> > Add dt-bindings for Hyper-V VMBus
> >
> > Signed-off-by: Saurabh Sengar <[email protected]>
> > ---
> > .../bindings/hypervisor/msft,vmbus.yaml | 50 +++++++++++++++++++
> > .../devicetree/bindings/vendor-prefixes.yaml | 2 +
> > MAINTAINERS | 1 +
> > 3 files changed, 53 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/hypervisor/msft,vmbus.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/hypervisor/msft,vmbus.yaml b/Documentation/devicetree/bindings/hypervisor/msft,vmbus.yaml
> > new file mode 100644
> > index 000000000000..8f50d6097c48
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hypervisor/msft,vmbus.yaml
> > @@ -0,0 +1,50 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hypervisor/msft,vmbus.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Microsoft Hyper-V VMBus device tree bindings
>
> This is a friendly reminder during the review process.
>
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.
>
> Thank you.
>
> (other places as well...)

Hi Krzysztof,

Thank you for the review. Sorry I missed this, will fix in V3.
The patches have gone significant modification, and I have thought
I have fixed all the comments you have provided hence didnt send the
follow up discussion. Apparently, I may have missed few will look again
and fix in next version.

Regards,
Saurabh

>
> Best regards,
> Krzysztof

2023-02-01 01:57:27

by Saurabh Singh Sengar

[permalink] [raw]
Subject: Re: [PATCH v2 5/6] dt-bindings: hypervisor: Add dt-bindings for VMBus

On Tue, Jan 31, 2023 at 07:54:56PM +0100, Krzysztof Kozlowski wrote:
> On 31/01/2023 19:10, Saurabh Sengar wrote:
> >
> > Signed-off-by: Saurabh Sengar <[email protected]>
>
> > + - |
> > + / {
> > + compatible = "foo";
> > + model = "foo";
> > + #address-cells = <0x02>;
> > + #size-cells = <0x02>;
>
> Except previous comments (all of them were ignored),

Thank you for your comments, I have tried to address them all in this version
but I may have missed few. I will go and look again all the emails, but if
there is any thing which you can point again and we can start a new dicussion
from here will be very helpful.

I think one concern was related to use of 'reg' or 'ranges', and I
thought this patch will give clarity that I intend to use 'ranges'
without any child node. Is this acceptable ?


> also:
> Drop entire part. Not related, not correct, not helping and you cannot
> have top level nodes in example.

If I dont use the top level node, the parent address cells are assumed to be 1,
and I get below warning. If there is better way to address this warning, please
suggest I will work on it.

Warning (ranges_format): /example-0/vmbus@ff0000000:ranges: "ranges" property has invalid length (20 bytes) (parent #address-cells == 1, child #address-cells == 2, #size-cells == 1)

>
> > +
> > + vmbus@ff0000000 {
> > + #address-cells = <0x02>;
> > + #size-cells = <0x01>;
> > + compatible = "msft,vmbus";
> > + ranges = <0x00 0x00 0x0f 0xf0000000 0x10000000>;
> > + };
> > + };
> > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> > index 1f7a519a936f..ab74ea97535f 100644
> > --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> > +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> > @@ -876,6 +876,8 @@ patternProperties:
> > deprecated: true
> > "^mscc,.*":
> > description: Microsemi Corporation
> > + "^msft,.*":
> > + description: Microsoft Corporation
>
> Don't duplicate vendor prefixes. Drop and use correct prefix.

Sure, will fix this.

Regards,
Saurabh

>
> Best regards,
> Krzysztof

2023-02-01 02:04:53

by Saurabh Singh Sengar

[permalink] [raw]
Subject: Re: [PATCH v2 0/6] Device tree support for Hyper-V VMBus driver

On Tue, Jan 31, 2023 at 02:27:51PM -0600, Rob Herring wrote:
> On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar
> <[email protected]> wrote:
> >
> > This set of patches expands the VMBus driver to include device tree
> > support.
> >
> > The first two patches enable compilation of Hyper-V APIs in a non-ACPI
> > build.
> >
> > The third patch converts the VMBus driver from acpi to more generic
> > platform driver.
> >
> > Further to add device tree documentation for VMBus, it needs to club with
> > other virtualization driver's documentation. For this rename the virtio
> > folder to more generic hypervisor, so that all the hypervisor based
> > devices can co-exist in a single place in device tree documentation. The
> > fourth patch does this renaming.
> >
> > The fifth patch introduces the device tree documentation for VMBus.
> >
> > The sixth patch adds device tree support to the VMBus driver. Currently
> > this is tested only for x86 and it may not work for other archs.
>
> I can read all the patches and see *what* they do. You don't really
> need to list that here. I'm still wondering *why*. That is what the
> cover letter and commit messages should answer. Why do you need DT
> support? How does this even work on x86? FDT is only enabled for
> CE4100 platform.

HI Rob,

Thanks for your comments.
We are working on a solution where kernel is booted without ACPI tables to keep
the overall system's memory footprints slim and possibly faster boot time.
We have tested this by enabling CONFIG_OF for x86.

I can add this info in cover letter in next version.

Regards,
Saurabh

>
> Rob

2023-02-01 05:36:49

by Saurabh Singh Sengar

[permalink] [raw]
Subject: Re: [PATCH v2 4/6] dt-bindings: hypervisor: Rename virtio to hypervisor

On Tue, Jan 31, 2023 at 01:57:36PM -0600, Rob Herring wrote:
>
> On Tue, 31 Jan 2023 10:10:07 -0800, Saurabh Sengar wrote:
> > Rename virtio folder to more generic hypervisor, so that this can
> > accommodate more devices of similar type.
> >
> > Signed-off-by: Saurabh Sengar <[email protected]>
> > ---
> > .../devicetree/bindings/{virtio => hypervisor}/mmio.yaml | 2 +-
> > .../devicetree/bindings/{virtio => hypervisor}/pci-iommu.yaml | 2 +-
> > .../bindings/{virtio => hypervisor}/virtio-device.yaml | 2 +-
> > MAINTAINERS | 2 +-
> > 4 files changed, 4 insertions(+), 4 deletions(-)
> > rename Documentation/devicetree/bindings/{virtio => hypervisor}/mmio.yaml (95%)
> > rename Documentation/devicetree/bindings/{virtio => hypervisor}/pci-iommu.yaml (98%)
> > rename Documentation/devicetree/bindings/{virtio => hypervisor}/virtio-device.yaml (93%)
> >
>
> 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:
>
> dtschema/dtc warnings/errors:
> ./Documentation/devicetree/bindings/i2c/i2c-virtio.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/virtio/virtio-device.yaml
> ./Documentation/devicetree/bindings/gpio/gpio-virtio.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/virtio/virtio-device.yaml
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/i2c-virtio.example.dtb: i2c: False schema does not allow {'compatible': ['virtio,device22'], '#address-cells': [[1]], '#size-cells': [[0]], 'light-sensor@20': {'compatible': ['dynaimage,al3320a'], 'reg': [[32]]}, '$nodename': ['i2c']}
> From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/gpio/gpio-virtio.example.dtb: gpio: False schema does not allow {'compatible': ['virtio,device29'], 'gpio-controller': True, '#gpio-cells': [[2]], 'interrupt-controller': True, '#interrupt-cells': [[2]], '$nodename': ['gpio']}
> From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/gpio/gpio-virtio.yaml
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hypervisor/virtio-device.example.dtb: i2c: False schema does not allow {'compatible': ['virtio,device22'], '$nodename': ['i2c']}
> From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml
>
> doc reference errors (make refcheckdocs):
> MAINTAINERS: Documentation/devicetree/bindings/virtio/
>
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
>
> 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.

Hi Rob,

I set DT_SCHEMA_FILES as below and ran "make -j32 DT_CHECKER_FLAGS=-m dt_binding_check".
export DT_SCHEMA_FILES=Documentation/devicetree/bindings/hypervisor

But I can see only below error:
/work/upstream/linux-next/Documentation/devicetree/bindings/hypervisor/virtio-device.example.dtb: i2c: False schema does not allow {'compatible': ['virtio,device22'], '$nodename': ['i2c']}
From schema: /work/upstream/linux-next/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml

If I unset DT_SCHEMA_FILES, I see lot many errors which are not related to my changes.
May I know what can I do to simulate the exact behaviour of your bot.

Version of all the packages look latest:

$ pip3 show dtschema
Name: dtschema
Version: 2023.1
Summary: DeviceTree validation schema and tools
Home-page: https://github.com/devicetree-org/dt-schema
Author: Rob Herring
Author-email: [email protected]
License: BSD
Location: /home/azureuser/.local/lib/python3.8/site-packages
Requires: rfc3987, jsonschema, pylibfdt, ruamel.yaml
Required-by:


$ dt-doc-validate --version
2023.1


$ yamllint --version
yamllint 1.20.0


Regards,
Saurabh


2023-02-01 07:23:55

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 5/6] dt-bindings: hypervisor: Add dt-bindings for VMBus

On 01/02/2023 02:57, Saurabh Singh Sengar wrote:
> On Tue, Jan 31, 2023 at 07:54:56PM +0100, Krzysztof Kozlowski wrote:
>> On 31/01/2023 19:10, Saurabh Sengar wrote:
>>>
>>> Signed-off-by: Saurabh Sengar <[email protected]>
>>
>>> + - |
>>> + / {
>>> + compatible = "foo";
>>> + model = "foo";
>>> + #address-cells = <0x02>;
>>> + #size-cells = <0x02>;
>>
>> Except previous comments (all of them were ignored),
>
> Thank you for your comments, I have tried to address them all in this version
> but I may have missed few. I will go and look again all the emails, but if
> there is any thing which you can point again and we can start a new dicussion
> from here will be very helpful.
>
> I think one concern was related to use of 'reg' or 'ranges', and I
> thought this patch will give clarity that I intend to use 'ranges'
> without any child node. Is this acceptable ?

There were several comments.

>
>
>> also:
>> Drop entire part. Not related, not correct, not helping and you cannot
>> have top level nodes in example.
>
> If I dont use the top level node, the parent address cells are assumed to be 1,
> and I get below warning. If there is better way to address this warning, please
> suggest I will work on it.
>
> Warning (ranges_format): /example-0/vmbus@ff0000000:ranges: "ranges" property has invalid length (20 bytes) (parent #address-cells == 1, child #address-cells == 2, #size-cells == 1)

Then use soc just like every other example.


Best regards,
Krzysztof


2023-02-01 08:05:50

by Saurabh Singh Sengar

[permalink] [raw]
Subject: Re: [PATCH v2 5/6] dt-bindings: hypervisor: Add dt-bindings for VMBus

On Wed, Feb 01, 2023 at 08:23:48AM +0100, Krzysztof Kozlowski wrote:
> On 01/02/2023 02:57, Saurabh Singh Sengar wrote:
> > On Tue, Jan 31, 2023 at 07:54:56PM +0100, Krzysztof Kozlowski wrote:
> >> On 31/01/2023 19:10, Saurabh Sengar wrote:
> >>>
> >>> Signed-off-by: Saurabh Sengar <[email protected]>
> >>
> >>> + - |
> >>> + / {
> >>> + compatible = "foo";
> >>> + model = "foo";
> >>> + #address-cells = <0x02>;
> >>> + #size-cells = <0x02>;
> >>
> >> Except previous comments (all of them were ignored),
> >
> > Thank you for your comments, I have tried to address them all in this version
> > but I may have missed few. I will go and look again all the emails, but if
> > there is any thing which you can point again and we can start a new dicussion
> > from here will be very helpful.
> >
> > I think one concern was related to use of 'reg' or 'ranges', and I
> > thought this patch will give clarity that I intend to use 'ranges'
> > without any child node. Is this acceptable ?
>
> There were several comments.

Ok, let me reply to all of your comments from previous thread to avoid
any confusion. Hope this is fine.

>
> >
> >
> >> also:
> >> Drop entire part. Not related, not correct, not helping and you cannot
> >> have top level nodes in example.
> >
> > If I dont use the top level node, the parent address cells are assumed to be 1,
> > and I get below warning. If there is better way to address this warning, please
> > suggest I will work on it.
> >
> > Warning (ranges_format): /example-0/vmbus@ff0000000:ranges: "ranges" property has invalid length (20 bytes) (parent #address-cells == 1, child #address-cells == 2, #size-cells == 1)
>
> Then use soc just like every other example.

Thanks for suggestion, I will fix this in V3

Regards,
Saurabh

>
>
> Best regards,
> Krzysztof

2023-02-01 14:51:52

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v2 0/6] Device tree support for Hyper-V VMBus driver

On Tue, Jan 31, 2023 at 06:04:49PM -0800, Saurabh Singh Sengar wrote:
> On Tue, Jan 31, 2023 at 02:27:51PM -0600, Rob Herring wrote:
> > On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar
> > <[email protected]> wrote:
> > >
> > > This set of patches expands the VMBus driver to include device tree
> > > support.
> > >
> > > The first two patches enable compilation of Hyper-V APIs in a non-ACPI
> > > build.
> > >
> > > The third patch converts the VMBus driver from acpi to more generic
> > > platform driver.
> > >
> > > Further to add device tree documentation for VMBus, it needs to club with
> > > other virtualization driver's documentation. For this rename the virtio
> > > folder to more generic hypervisor, so that all the hypervisor based
> > > devices can co-exist in a single place in device tree documentation. The
> > > fourth patch does this renaming.
> > >
> > > The fifth patch introduces the device tree documentation for VMBus.
> > >
> > > The sixth patch adds device tree support to the VMBus driver. Currently
> > > this is tested only for x86 and it may not work for other archs.
> >
> > I can read all the patches and see *what* they do. You don't really
> > need to list that here. I'm still wondering *why*. That is what the
> > cover letter and commit messages should answer. Why do you need DT
> > support? How does this even work on x86? FDT is only enabled for
> > CE4100 platform.
>
> HI Rob,
>
> Thanks for your comments.
> We are working on a solution where kernel is booted without ACPI tables to keep
> the overall system's memory footprints slim and possibly faster boot time.
> We have tested this by enabling CONFIG_OF for x86.

It's CONFIG_OF_EARLY_FLATTREE which you would need and that's not user
selectable. At a minimum, you need some kconfig changes. Where are
those?

Also see my comment on v1 about running DT validation on your dtb. I'm
sure running it would point out other issues. Such as the root level
comaptible string(s) need to be documented. You need cpu nodes,
interrupt controller, timers, etc. Those all have to be documented.

Rob

2023-02-01 16:35:03

by Saurabh Singh Sengar

[permalink] [raw]
Subject: Re: [PATCH v2 0/6] Device tree support for Hyper-V VMBus driver

On Wed, Feb 01, 2023 at 08:51:46AM -0600, Rob Herring wrote:
> On Tue, Jan 31, 2023 at 06:04:49PM -0800, Saurabh Singh Sengar wrote:
> > On Tue, Jan 31, 2023 at 02:27:51PM -0600, Rob Herring wrote:
> > > On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar
> > > <[email protected]> wrote:
> > > >
> > > > This set of patches expands the VMBus driver to include device tree
> > > > support.
> > > >
> > > > The first two patches enable compilation of Hyper-V APIs in a non-ACPI
> > > > build.
> > > >
> > > > The third patch converts the VMBus driver from acpi to more generic
> > > > platform driver.
> > > >
> > > > Further to add device tree documentation for VMBus, it needs to club with
> > > > other virtualization driver's documentation. For this rename the virtio
> > > > folder to more generic hypervisor, so that all the hypervisor based
> > > > devices can co-exist in a single place in device tree documentation. The
> > > > fourth patch does this renaming.
> > > >
> > > > The fifth patch introduces the device tree documentation for VMBus.
> > > >
> > > > The sixth patch adds device tree support to the VMBus driver. Currently
> > > > this is tested only for x86 and it may not work for other archs.
> > >
> > > I can read all the patches and see *what* they do. You don't really
> > > need to list that here. I'm still wondering *why*. That is what the
> > > cover letter and commit messages should answer. Why do you need DT
> > > support? How does this even work on x86? FDT is only enabled for
> > > CE4100 platform.
> >
> > HI Rob,
> >
> > Thanks for your comments.
> > We are working on a solution where kernel is booted without ACPI tables to keep
> > the overall system's memory footprints slim and possibly faster boot time.
> > We have tested this by enabling CONFIG_OF for x86.
>
> It's CONFIG_OF_EARLY_FLATTREE which you would need and that's not user
> selectable. At a minimum, you need some kconfig changes. Where are
> those?

You are right we have define a new config flag in Kconfig, and selected CONFIG_OF
and CONFIG_OF_EARLY_FLATTREE. We are working on upstreaming that patch as well
however that will be a separate patch series.

>
> Also see my comment on v1 about running DT validation on your dtb. I'm
> sure running it would point out other issues. Such as the root level
> comaptible string(s) need to be documented. You need cpu nodes,
> interrupt controller, timers, etc. Those all have to be documented.

I will be changing the parent node to soc node as suggested by Krzysztof
in other thread.

soc {
#address-cells = <2>;
#size-cells = <2>;

vmbus@ff0000000 {
#address-cells = <2>;
#size-cells = <1>;
compatible = "Microsoft,vmbus";
ranges = <0x00 0x00 0x0f 0xf0000000 0x10000000>;
};
};

This will be sufficient.

Regards,
Saurabh

>
> Rob

2023-02-01 16:51:38

by Saurabh Singh Sengar

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

On Tue, Jan 31, 2023 at 02:12:53PM -0600, Rob Herring wrote:
> On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar
> <[email protected]> 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 <[email protected]>
> > ---
> > 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.

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

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.

>
> > + 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 :(

Regards,
Saurabh

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

2023-02-01 17:15:36

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 0/6] Device tree support for Hyper-V VMBus driver

On 01/02/2023 17:34, Saurabh Singh Sengar wrote:
>> Also see my comment on v1 about running DT validation on your dtb. I'm
>> sure running it would point out other issues. Such as the root level
>> comaptible string(s) need to be documented. You need cpu nodes,
>> interrupt controller, timers, etc. Those all have to be documented.
>
> I will be changing the parent node to soc node as suggested by Krzysztof
> in other thread.
>
> soc {
> #address-cells = <2>;
> #size-cells = <2>;
>
> vmbus@ff0000000 {
> #address-cells = <2>;
> #size-cells = <1>;
> compatible = "Microsoft,vmbus";
> ranges = <0x00 0x00 0x0f 0xf0000000 0x10000000>;
> };
> };
>
> This will be sufficient.

It will be ok for the example, but will not be ok for supporting your
use case. Please solve all the points from Rob's comment above. Where is
their documentation?

Best regards,
Krzysztof


2023-02-01 17:46:46

by Rob Herring (Arm)

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

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
> > <[email protected]> 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 <[email protected]>
> > > ---
> > > 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/

2023-02-01 18:31:48

by Saurabh Singh Sengar

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

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
> > > <[email protected]> wrote:
> > > >
(...)
> > > > @@ -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.

Thanks, Will fix this in v3.

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

Thanks I will work on this suggestion and fix this up in next version.

2023-02-01 18:32:41

by Michael Kelley (LINUX)

[permalink] [raw]
Subject: RE: [PATCH v2 3/6] Drivers: hv: vmbus: Convert acpi_device to platform_device

From: Saurabh Sengar <[email protected]> Sent: Tuesday, January 31, 2023 10:10 AM
>
> Use more generic platform device instead of acpi device. Also rename the
> function vmbus_acpi_remove to more generic name vmbus_mmio_remove.
>
> Signed-off-by: Saurabh Sengar <[email protected]>
> ---
> drivers/hv/vmbus_drv.c | 78 +++++++++++++++++++++++++-----------------
> 1 file changed, 46 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index d24dd65b33d4..49030e756b9f 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -12,6 +12,7 @@
> #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>
> @@ -44,7 +45,7 @@ struct vmbus_dynid {
> struct hv_vmbus_device_id id;
> };
>
> -static struct acpi_device *hv_acpi_dev;
> +static struct platform_device *hv_dev;
>
> static int hyperv_cpuhp_online;
>
> @@ -143,7 +144,7 @@ static DEFINE_MUTEX(hyperv_mmio_lock);
>
> static int vmbus_exists(void)
> {
> - if (hv_acpi_dev == NULL)
> + if (hv_dev == NULL)
> return -ENODEV;
>
> return 0;
> @@ -932,7 +933,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(&hv_dev->dev) == DEV_DMA_COHERENT);
> return 0;
> }
>
> @@ -2090,7 +2091,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 = &hv_dev->dev;
> child_device_obj->device.release = vmbus_device_release;
>
> child_device_obj->device.dma_parms = &child_device_obj->dma_parms;
> @@ -2262,7 +2263,7 @@ static acpi_status vmbus_walk_resources(struct
> acpi_resource *res, void *ctx)
> return AE_OK;
> }
>
> -static void vmbus_acpi_remove(struct acpi_device *device)
> +static void vmbus_mmio_remove(void)
> {
> struct resource *cur_res;
> struct resource *next_res;
> @@ -2441,13 +2442,15 @@ void vmbus_free_mmio(resource_size_t start,
> resource_size_t size)
> }
> EXPORT_SYMBOL_GPL(vmbus_free_mmio);
>
> -static int vmbus_acpi_add(struct acpi_device *device)
> +static int vmbus_acpi_add(struct platform_device *pdev)
> {
> acpi_status result;
> int ret_val = -ENODEV;
> - struct acpi_device *ancestor;
> + struct platform_device *ancestor;
> + struct acpi_device *adev = to_acpi_device(&pdev->dev);

This doesn't work. The argument to vmbus_acpi_add() is a struct
platform_device, which has a struct device embedded in it (not a
pointer). to_acpi_device() takes a struct device as an argument,
assuming that the struct device is embedded in a struct
acpi_device, which is not the case here. The resulting local
variable adev is actually pointing to some (perhaps negative)
offset within the struct platform_device, and uses of adev are
getting unknown random data from within (or before) the
struct platform_device.

>
> - hv_acpi_dev = device;
> + hv_dev = pdev;
> + adev->fwnode.dev = &pdev->dev;
>
> /*
> * Older versions of Hyper-V for ARM64 fail to include the _CCA
> @@ -2456,15 +2459,16 @@ static int vmbus_acpi_add(struct acpi_device *device)
> * up the ACPI device to behave as if _CCA is present and indicates
> * hardware coherence.
> */
> - ACPI_COMPANION_SET(&device->dev, device);
> + ACPI_COMPANION_SET(&pdev->dev, ACPI_COMPANION(&pdev->dev));

This statement seems tautological. If ACPI_COMPANION(&pdev->dev)
returns a valid result, why would the ACPI companion for &pdev->dev
need to be set? The original code was setting the ACPI companion for the
embedded struct device to be the struct acpi_device. I forget why this
wasn't already done for the VMBus device when it was originally parsed
from the ACPI DSDT ...

> if (IS_ENABLED(CONFIG_ACPI_CCA_REQUIRED) &&
> - device_get_dma_attr(&device->dev) == DEV_DMA_NOT_SUPPORTED) {
> + device_get_dma_attr(&pdev->dev) == DEV_DMA_NOT_SUPPORTED) {
> + struct acpi_device *adev_node = ACPI_COMPANION(&pdev->dev);

If earlier code in this function can get a correct pointer to the struct acpi_device,
then this statement shouldn't be necessary. You already have it.

> pr_info("No ACPI _CCA found; assuming coherent device I/O\n");
> - device->flags.cca_seen = true;
> - device->flags.coherent_dma = true;
> + adev_node->flags.cca_seen = true;
> + adev_node->flags.coherent_dma = true;
> }
>
> - result = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> + result = acpi_walk_resources(ACPI_HANDLE(&pdev->dev), METHOD_NAME__CRS,

Again, if you have a correct pointer to the struct acpi_device, then adev->handle
(like the original code) should be simpler than looking it up again with ACPI_HANDLE().

> vmbus_walk_resources, NULL);
>
> if (ACPI_FAILURE(result))
> @@ -2473,9 +2477,9 @@ static int vmbus_acpi_add(struct acpi_device *device)
> * Some ancestor of the vmbus acpi device (Gen1 or Gen2
> * firmware) is the VMOD that has the mmio ranges. Get that.
> */
> - for (ancestor = acpi_dev_parent(device); ancestor;
> - ancestor = acpi_dev_parent(ancestor)) {
> - result = acpi_walk_resources(ancestor->handle, METHOD_NAME__CRS,
> + for (ancestor = to_platform_device(pdev->dev.parent); ancestor;
> + ancestor = to_platform_device(ancestor->dev.parent)) {
> + result = acpi_walk_resources(ACPI_HANDLE(&ancestor->dev), METHOD_NAME__CRS,

Similarly, if you get a correct pointer to the struct acpi_device, does the above
code need any changes? I'm hoping not.

> vmbus_walk_resources, NULL);
>
> if (ACPI_FAILURE(result))
> @@ -2489,10 +2493,21 @@ static int vmbus_acpi_add(struct acpi_device *device)
>
> acpi_walk_err:
> if (ret_val)
> - vmbus_acpi_remove(device);
> + vmbus_mmio_remove();
> return ret_val;
> }
>
> +static int vmbus_platform_driver_probe(struct platform_device *pdev)
> +{
> + return vmbus_acpi_add(pdev);
> +}
> +
> +static int vmbus_platform_driver_remove(struct platform_device *pdev)
> +{
> + vmbus_mmio_remove();
> + return 0;
> +}
> +
> #ifdef CONFIG_PM_SLEEP
> static int vmbus_bus_suspend(struct device *dev)
> {
> @@ -2658,15 +2673,15 @@ static const struct dev_pm_ops vmbus_bus_pm = {
> .restore_noirq = vmbus_bus_resume
> };
>
> -static struct acpi_driver vmbus_acpi_driver = {
> - .name = "vmbus",
> - .ids = vmbus_acpi_device_ids,
> - .ops = {
> - .add = vmbus_acpi_add,
> - .remove = vmbus_acpi_remove,
> - },
> - .drv.pm = &vmbus_bus_pm,
> - .drv.probe_type = PROBE_FORCE_SYNCHRONOUS,
> +static struct platform_driver vmbus_platform_driver = {
> + .probe = vmbus_platform_driver_probe,
> + .remove = vmbus_platform_driver_remove,
> + .driver = {
> + .name = "vmbus",
> + .acpi_match_table = ACPI_PTR(vmbus_acpi_device_ids),
> + .pm = &vmbus_bus_pm,
> + .probe_type = PROBE_FORCE_SYNCHRONOUS,
> + }
> };
>
> static void hv_kexec_handler(void)
> @@ -2750,12 +2765,11 @@ static int __init hv_acpi_init(void)
> /*
> * Get ACPI resources first.
> */
> - ret = acpi_bus_register_driver(&vmbus_acpi_driver);
> -
> + ret = platform_driver_register(&vmbus_platform_driver);
> if (ret)
> return ret;
>
> - if (!hv_acpi_dev) {
> + if (!hv_dev) {
> ret = -ENODEV;
> goto cleanup;
> }
> @@ -2785,8 +2799,8 @@ static int __init hv_acpi_init(void)
> return 0;
>
> cleanup:
> - acpi_bus_unregister_driver(&vmbus_acpi_driver);
> - hv_acpi_dev = NULL;
> + platform_driver_unregister(&vmbus_platform_driver);
> + hv_dev = NULL;
> return ret;
> }
>
> @@ -2839,7 +2853,7 @@ static void __exit vmbus_exit(void)
>
> cpuhp_remove_state(hyperv_cpuhp_online);
> hv_synic_free();
> - acpi_bus_unregister_driver(&vmbus_acpi_driver);
> + platform_driver_unregister(&vmbus_platform_driver);
> }
>
>
> --
> 2.25.1


2023-02-02 15:28:35

by Saurabh Singh Sengar

[permalink] [raw]
Subject: Re: [PATCH v2 3/6] Drivers: hv: vmbus: Convert acpi_device to platform_device

On Wed, Feb 01, 2023 at 06:32:29PM +0000, Michael Kelley (LINUX) wrote:
> From: Saurabh Sengar <[email protected]> Sent: Tuesday, January 31, 2023 10:10 AM
> >
> > Use more generic platform device instead of acpi device. Also rename the
> > function vmbus_acpi_remove to more generic name vmbus_mmio_remove.
> >
> > Signed-off-by: Saurabh Sengar <[email protected]>
> > ---
> > drivers/hv/vmbus_drv.c | 78 +++++++++++++++++++++++++-----------------
> > 1 file changed, 46 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index d24dd65b33d4..49030e756b9f 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -12,6 +12,7 @@
> > #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>
> > @@ -44,7 +45,7 @@ struct vmbus_dynid {
> > struct hv_vmbus_device_id id;
> > };
> >
> > -static struct acpi_device *hv_acpi_dev;
> > +static struct platform_device *hv_dev;
> >
> > static int hyperv_cpuhp_online;
> >
> > @@ -143,7 +144,7 @@ static DEFINE_MUTEX(hyperv_mmio_lock);
> >
> > static int vmbus_exists(void)
> > {
> > - if (hv_acpi_dev == NULL)
> > + if (hv_dev == NULL)
> > return -ENODEV;
> >
> > return 0;
> > @@ -932,7 +933,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(&hv_dev->dev) == DEV_DMA_COHERENT);
> > return 0;
> > }
> >
> > @@ -2090,7 +2091,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 = &hv_dev->dev;
> > child_device_obj->device.release = vmbus_device_release;
> >
> > child_device_obj->device.dma_parms = &child_device_obj->dma_parms;
> > @@ -2262,7 +2263,7 @@ static acpi_status vmbus_walk_resources(struct
> > acpi_resource *res, void *ctx)
> > return AE_OK;
> > }
> >
> > -static void vmbus_acpi_remove(struct acpi_device *device)
> > +static void vmbus_mmio_remove(void)
> > {
> > struct resource *cur_res;
> > struct resource *next_res;
> > @@ -2441,13 +2442,15 @@ void vmbus_free_mmio(resource_size_t start,
> > resource_size_t size)
> > }
> > EXPORT_SYMBOL_GPL(vmbus_free_mmio);
> >
> > -static int vmbus_acpi_add(struct acpi_device *device)
> > +static int vmbus_acpi_add(struct platform_device *pdev)
> > {
> > acpi_status result;
> > int ret_val = -ENODEV;
> > - struct acpi_device *ancestor;
> > + struct platform_device *ancestor;
> > + struct acpi_device *adev = to_acpi_device(&pdev->dev);
>
> This doesn't work. The argument to vmbus_acpi_add() is a struct
> platform_device, which has a struct device embedded in it (not a
> pointer). to_acpi_device() takes a struct device as an argument,
> assuming that the struct device is embedded in a struct
> acpi_device, which is not the case here. The resulting local
> variable adev is actually pointing to some (perhaps negative)
> offset within the struct platform_device, and uses of adev are
> getting unknown random data from within (or before) the
> struct platform_device.

Thanks for review.
If I understand correctly you are saying to change the argument of
to_acpi_device from pointer to 'struct device'. I tried changing this
statement to:
"struct acpi_device *adev = to_acpi_device(pdev->dev);"

However on compiling this code I see this error:
./include/linux/container_of.h:19:28: error: invalid type argument of unary ‘*’ (have ‘struct device’)

Please let me know if I missunderstood your suggestion.

I would like to mention that this code has been tested and verified.
The purpose of this change was to assign the device node to the acpi
firmware node, as seen in the line

"adev->fwnode.dev = &pdev->dev;".

This was necessary because without it, the mlx driver would crash as
it searches for the device in the acpi firmware node, which would
otherwise be NULL. I am confident that the addresses are correctly
assigned, as any issues with the assignments would have caused a panic.

Regards,
Saurabh

>
> >
> > - hv_acpi_dev = device;
> > + hv_dev = pdev;
> > + adev->fwnode.dev = &pdev->dev;
> >
> > /*
> > * Older versions of Hyper-V for ARM64 fail to include the _CCA
> > @@ -2456,15 +2459,16 @@ static int vmbus_acpi_add(struct acpi_device *device)
> > * up the ACPI device to behave as if _CCA is present and indicates
> > * hardware coherence.
> > */
> > - ACPI_COMPANION_SET(&device->dev, device);
> > + ACPI_COMPANION_SET(&pdev->dev, ACPI_COMPANION(&pdev->dev));
>
> This statement seems tautological. If ACPI_COMPANION(&pdev->dev)
> returns a valid result, why would the ACPI companion for &pdev->dev
> need to be set? The original code was setting the ACPI companion for the
> embedded struct device to be the struct acpi_device. I forget why this
> wasn't already done for the VMBus device when it was originally parsed
> from the ACPI DSDT ...
>
> > if (IS_ENABLED(CONFIG_ACPI_CCA_REQUIRED) &&
> > - device_get_dma_attr(&device->dev) == DEV_DMA_NOT_SUPPORTED) {
> > + device_get_dma_attr(&pdev->dev) == DEV_DMA_NOT_SUPPORTED) {
> > + struct acpi_device *adev_node = ACPI_COMPANION(&pdev->dev);
>
> If earlier code in this function can get a correct pointer to the struct acpi_device,
> then this statement shouldn't be necessary. You already have it.
>
> > pr_info("No ACPI _CCA found; assuming coherent device I/O\n");
> > - device->flags.cca_seen = true;
> > - device->flags.coherent_dma = true;
> > + adev_node->flags.cca_seen = true;
> > + adev_node->flags.coherent_dma = true;
> > }
> >
> > - result = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> > + result = acpi_walk_resources(ACPI_HANDLE(&pdev->dev), METHOD_NAME__CRS,
>
> Again, if you have a correct pointer to the struct acpi_device, then adev->handle
> (like the original code) should be simpler than looking it up again with ACPI_HANDLE().
>
> > vmbus_walk_resources, NULL);
> >
> > if (ACPI_FAILURE(result))
> > @@ -2473,9 +2477,9 @@ static int vmbus_acpi_add(struct acpi_device *device)
> > * Some ancestor of the vmbus acpi device (Gen1 or Gen2
> > * firmware) is the VMOD that has the mmio ranges. Get that.
> > */
> > - for (ancestor = acpi_dev_parent(device); ancestor;
> > - ancestor = acpi_dev_parent(ancestor)) {
> > - result = acpi_walk_resources(ancestor->handle, METHOD_NAME__CRS,
> > + for (ancestor = to_platform_device(pdev->dev.parent); ancestor;
> > + ancestor = to_platform_device(ancestor->dev.parent)) {
> > + result = acpi_walk_resources(ACPI_HANDLE(&ancestor->dev), METHOD_NAME__CRS,
>
> Similarly, if you get a correct pointer to the struct acpi_device, does the above
> code need any changes? I'm hoping not.
>
> > vmbus_walk_resources, NULL);
> >
> > if (ACPI_FAILURE(result))
> > @@ -2489,10 +2493,21 @@ static int vmbus_acpi_add(struct acpi_device *device)
> >
> > acpi_walk_err:
> > if (ret_val)
> > - vmbus_acpi_remove(device);
> > + vmbus_mmio_remove();
> > return ret_val;
> > }
> >
> > +static int vmbus_platform_driver_probe(struct platform_device *pdev)
> > +{
> > + return vmbus_acpi_add(pdev);
> > +}
> > +
> > +static int vmbus_platform_driver_remove(struct platform_device *pdev)
> > +{
> > + vmbus_mmio_remove();
> > + return 0;
> > +}
> > +
> > #ifdef CONFIG_PM_SLEEP
> > static int vmbus_bus_suspend(struct device *dev)
> > {
> > @@ -2658,15 +2673,15 @@ static const struct dev_pm_ops vmbus_bus_pm = {
> > .restore_noirq = vmbus_bus_resume
> > };
> >
> > -static struct acpi_driver vmbus_acpi_driver = {
> > - .name = "vmbus",
> > - .ids = vmbus_acpi_device_ids,
> > - .ops = {
> > - .add = vmbus_acpi_add,
> > - .remove = vmbus_acpi_remove,
> > - },
> > - .drv.pm = &vmbus_bus_pm,
> > - .drv.probe_type = PROBE_FORCE_SYNCHRONOUS,
> > +static struct platform_driver vmbus_platform_driver = {
> > + .probe = vmbus_platform_driver_probe,
> > + .remove = vmbus_platform_driver_remove,
> > + .driver = {
> > + .name = "vmbus",
> > + .acpi_match_table = ACPI_PTR(vmbus_acpi_device_ids),
> > + .pm = &vmbus_bus_pm,
> > + .probe_type = PROBE_FORCE_SYNCHRONOUS,
> > + }
> > };
> >
> > static void hv_kexec_handler(void)
> > @@ -2750,12 +2765,11 @@ static int __init hv_acpi_init(void)
> > /*
> > * Get ACPI resources first.
> > */
> > - ret = acpi_bus_register_driver(&vmbus_acpi_driver);
> > -
> > + ret = platform_driver_register(&vmbus_platform_driver);
> > if (ret)
> > return ret;
> >
> > - if (!hv_acpi_dev) {
> > + if (!hv_dev) {
> > ret = -ENODEV;
> > goto cleanup;
> > }
> > @@ -2785,8 +2799,8 @@ static int __init hv_acpi_init(void)
> > return 0;
> >
> > cleanup:
> > - acpi_bus_unregister_driver(&vmbus_acpi_driver);
> > - hv_acpi_dev = NULL;
> > + platform_driver_unregister(&vmbus_platform_driver);
> > + hv_dev = NULL;
> > return ret;
> > }
> >
> > @@ -2839,7 +2853,7 @@ static void __exit vmbus_exit(void)
> >
> > cpuhp_remove_state(hyperv_cpuhp_online);
> > hv_synic_free();
> > - acpi_bus_unregister_driver(&vmbus_acpi_driver);
> > + platform_driver_unregister(&vmbus_platform_driver);
> > }
> >
> >
> > --
> > 2.25.1

2023-02-03 05:32:31

by Saurabh Singh Sengar

[permalink] [raw]
Subject: Re: [PATCH v2 3/6] Drivers: hv: vmbus: Convert acpi_device to platform_device

On Wed, Feb 01, 2023 at 06:32:29PM +0000, Michael Kelley (LINUX) wrote:
> From: Saurabh Sengar <[email protected]> Sent: Tuesday, January 31, 2023 10:10 AM
> >
> > Use more generic platform device instead of acpi device. Also rename the
> > function vmbus_acpi_remove to more generic name vmbus_mmio_remove.
> >
> > Signed-off-by: Saurabh Sengar <[email protected]>
> > ---
> > drivers/hv/vmbus_drv.c | 78 +++++++++++++++++++++++++-----------------
> > 1 file changed, 46 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index d24dd65b33d4..49030e756b9f 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -12,6 +12,7 @@
> > #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>
> > @@ -44,7 +45,7 @@ struct vmbus_dynid {
> > struct hv_vmbus_device_id id;
> > };
> >
> > -static struct acpi_device *hv_acpi_dev;
> > +static struct platform_device *hv_dev;
> >
> > static int hyperv_cpuhp_online;
> >
> > @@ -143,7 +144,7 @@ static DEFINE_MUTEX(hyperv_mmio_lock);
> >
> > static int vmbus_exists(void)
> > {
> > - if (hv_acpi_dev == NULL)
> > + if (hv_dev == NULL)
> > return -ENODEV;
> >
> > return 0;
> > @@ -932,7 +933,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(&hv_dev->dev) == DEV_DMA_COHERENT);
> > return 0;
> > }
> >
> > @@ -2090,7 +2091,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 = &hv_dev->dev;
> > child_device_obj->device.release = vmbus_device_release;
> >
> > child_device_obj->device.dma_parms = &child_device_obj->dma_parms;
> > @@ -2262,7 +2263,7 @@ static acpi_status vmbus_walk_resources(struct
> > acpi_resource *res, void *ctx)
> > return AE_OK;
> > }
> >
> > -static void vmbus_acpi_remove(struct acpi_device *device)
> > +static void vmbus_mmio_remove(void)
> > {
> > struct resource *cur_res;
> > struct resource *next_res;
> > @@ -2441,13 +2442,15 @@ void vmbus_free_mmio(resource_size_t start,
> > resource_size_t size)
> > }
> > EXPORT_SYMBOL_GPL(vmbus_free_mmio);
> >
> > -static int vmbus_acpi_add(struct acpi_device *device)
> > +static int vmbus_acpi_add(struct platform_device *pdev)
> > {
> > acpi_status result;
> > int ret_val = -ENODEV;
> > - struct acpi_device *ancestor;
> > + struct platform_device *ancestor;
> > + struct acpi_device *adev = to_acpi_device(&pdev->dev);
>
> This doesn't work. The argument to vmbus_acpi_add() is a struct
> platform_device, which has a struct device embedded in it (not a
> pointer). to_acpi_device() takes a struct device as an argument,
> assuming that the struct device is embedded in a struct
> acpi_device, which is not the case here. The resulting local
> variable adev is actually pointing to some (perhaps negative)
> offset within the struct platform_device, and uses of adev are
> getting unknown random data from within (or before) the
> struct platform_device.

Please discard my earlier reply on this. I will fix it in V3.
Thanks for pointing this.

>
> >
> > - hv_acpi_dev = device;
> > + hv_dev = pdev;
> > + adev->fwnode.dev = &pdev->dev;
> >
> > /*
> > * Older versions of Hyper-V for ARM64 fail to include the _CCA
> > @@ -2456,15 +2459,16 @@ static int vmbus_acpi_add(struct acpi_device *device)
> > * up the ACPI device to behave as if _CCA is present and indicates
> > * hardware coherence.
> > */
> > - ACPI_COMPANION_SET(&device->dev, device);
> > + ACPI_COMPANION_SET(&pdev->dev, ACPI_COMPANION(&pdev->dev));
>
> This statement seems tautological. If ACPI_COMPANION(&pdev->dev)
> returns a valid result, why would the ACPI companion for &pdev->dev
> need to be set? The original code was setting the ACPI companion for the
> embedded struct device to be the struct acpi_device. I forget why this
> wasn't already done for the VMBus device when it was originally parsed
> from the ACPI DSDT ...

This need to be changed to:
ACPI_COMPANION_SET(&adev_node->dev, adev_node)
will fix this as well.

>
> > if (IS_ENABLED(CONFIG_ACPI_CCA_REQUIRED) &&
> > - device_get_dma_attr(&device->dev) == DEV_DMA_NOT_SUPPORTED) {
> > + device_get_dma_attr(&pdev->dev) == DEV_DMA_NOT_SUPPORTED) {
> > + struct acpi_device *adev_node = ACPI_COMPANION(&pdev->dev);
>
> If earlier code in this function can get a correct pointer to the struct acpi_device,
> then this statement shouldn't be necessary. You already have it.

agree, will fix

>
> > pr_info("No ACPI _CCA found; assuming coherent device I/O\n");
> > - device->flags.cca_seen = true;
> > - device->flags.coherent_dma = true;
> > + adev_node->flags.cca_seen = true;
> > + adev_node->flags.coherent_dma = true;
> > }
> >
> > - result = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> > + result = acpi_walk_resources(ACPI_HANDLE(&pdev->dev), METHOD_NAME__CRS,
>
> Again, if you have a correct pointer to the struct acpi_device, then adev->handle
> (like the original code) should be simpler than looking it up again with ACPI_HANDLE().

OK

>
> > vmbus_walk_resources, NULL);
> >
> > if (ACPI_FAILURE(result))
> > @@ -2473,9 +2477,9 @@ static int vmbus_acpi_add(struct acpi_device *device)
> > * Some ancestor of the vmbus acpi device (Gen1 or Gen2
> > * firmware) is the VMOD that has the mmio ranges. Get that.
> > */
> > - for (ancestor = acpi_dev_parent(device); ancestor;
> > - ancestor = acpi_dev_parent(ancestor)) {
> > - result = acpi_walk_resources(ancestor->handle, METHOD_NAME__CRS,
> > + for (ancestor = to_platform_device(pdev->dev.parent); ancestor;
> > + ancestor = to_platform_device(ancestor->dev.parent)) {
> > + result = acpi_walk_resources(ACPI_HANDLE(&ancestor->dev), METHOD_NAME__CRS,
>
> Similarly, if you get a correct pointer to the struct acpi_device, does the above
> code need any changes? I'm hoping not.

Will try to clean this up as well.

Regards,
Saurabh

>
> > vmbus_walk_resources, NULL);
> >
> > if (ACPI_FAILURE(result))
> > @@ -2489,10 +2493,21 @@ static int vmbus_acpi_add(struct acpi_device *device)
> >
> > acpi_walk_err:
> > if (ret_val)
> > - vmbus_acpi_remove(device);
> > + vmbus_mmio_remove();
> > return ret_val;
> > }
> >
> > +static int vmbus_platform_driver_probe(struct platform_device *pdev)
> > +{
> > + return vmbus_acpi_add(pdev);
> > +}
> > +
> > +static int vmbus_platform_driver_remove(struct platform_device *pdev)
> > +{
> > + vmbus_mmio_remove();
> > + return 0;
> > +}
> > +
> > #ifdef CONFIG_PM_SLEEP
> > static int vmbus_bus_suspend(struct device *dev)
> > {
> > @@ -2658,15 +2673,15 @@ static const struct dev_pm_ops vmbus_bus_pm = {
> > .restore_noirq = vmbus_bus_resume
> > };
> >
> > -static struct acpi_driver vmbus_acpi_driver = {
> > - .name = "vmbus",
> > - .ids = vmbus_acpi_device_ids,
> > - .ops = {
> > - .add = vmbus_acpi_add,
> > - .remove = vmbus_acpi_remove,
> > - },
> > - .drv.pm = &vmbus_bus_pm,
> > - .drv.probe_type = PROBE_FORCE_SYNCHRONOUS,
> > +static struct platform_driver vmbus_platform_driver = {
> > + .probe = vmbus_platform_driver_probe,
> > + .remove = vmbus_platform_driver_remove,
> > + .driver = {
> > + .name = "vmbus",
> > + .acpi_match_table = ACPI_PTR(vmbus_acpi_device_ids),
> > + .pm = &vmbus_bus_pm,
> > + .probe_type = PROBE_FORCE_SYNCHRONOUS,
> > + }
> > };
> >
> > static void hv_kexec_handler(void)
> > @@ -2750,12 +2765,11 @@ static int __init hv_acpi_init(void)
> > /*
> > * Get ACPI resources first.
> > */
> > - ret = acpi_bus_register_driver(&vmbus_acpi_driver);
> > -
> > + ret = platform_driver_register(&vmbus_platform_driver);
> > if (ret)
> > return ret;
> >
> > - if (!hv_acpi_dev) {
> > + if (!hv_dev) {
> > ret = -ENODEV;
> > goto cleanup;
> > }
> > @@ -2785,8 +2799,8 @@ static int __init hv_acpi_init(void)
> > return 0;
> >
> > cleanup:
> > - acpi_bus_unregister_driver(&vmbus_acpi_driver);
> > - hv_acpi_dev = NULL;
> > + platform_driver_unregister(&vmbus_platform_driver);
> > + hv_dev = NULL;
> > return ret;
> > }
> >
> > @@ -2839,7 +2853,7 @@ static void __exit vmbus_exit(void)
> >
> > cpuhp_remove_state(hyperv_cpuhp_online);
> > hv_synic_free();
> > - acpi_bus_unregister_driver(&vmbus_acpi_driver);
> > + platform_driver_unregister(&vmbus_platform_driver);
> > }
> >
> >
> > --
> > 2.25.1

2023-02-03 17:37:09

by Saurabh Singh Sengar

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

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
> > > <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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-
> >
> > >
> > > > 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)
>
> [1] https://lwn.net/Articles/443531/

2023-02-06 17:40:27

by Saurabh Singh Sengar

[permalink] [raw]
Subject: Re: [PATCH v2 0/6] Device tree support for Hyper-V VMBus driver

On Wed, Feb 01, 2023 at 06:15:23PM +0100, Krzysztof Kozlowski wrote:
> On 01/02/2023 17:34, Saurabh Singh Sengar wrote:
> >> Also see my comment on v1 about running DT validation on your dtb. I'm
> >> sure running it would point out other issues. Such as the root level
> >> comaptible string(s) need to be documented. You need cpu nodes,
> >> interrupt controller, timers, etc. Those all have to be documented.
> >
> > I will be changing the parent node to soc node as suggested by Krzysztof
> > in other thread.
> >
> > soc {
> > #address-cells = <2>;
> > #size-cells = <2>;
> >
> > vmbus@ff0000000 {
> > #address-cells = <2>;
> > #size-cells = <1>;
> > compatible = "Microsoft,vmbus";
> > ranges = <0x00 0x00 0x0f 0xf0000000 0x10000000>;
> > };
> > };
> >
> > This will be sufficient.
>
> It will be ok for the example, but will not be ok for supporting your
> use case. Please solve all the points from Rob's comment above. Where is
> their documentation?
>
> Best regards,
> Krzysztof

Hi Rob/ Krzysztof,

I am happy to update the documentation as requested. Please note
that, apart from CPUs, there is no other device node in the tree.

Here are some of the info related to our system:

Timers:
VMBus code uses a Hyper-V Synthetic timer and there is no device tree
node or ACPI method required for this. This is implemented as
drivers/clocksource/hyperv_timer.c

Interrupt controller:
The hypervisor virtualizes interrupt delivery to virtual processors.
This is done through the use of a synthetic interrupt controller
(SynIC) which is an extension of a virtualized local APIC. In the cpu
DT nodes we have APIC ids.

Below are the cpu nodes we use, please suggest if I need to update any
document for it.
cpus {
#address-cells = <1>;
#size-cells = <0>;

cpu@0 {
device_type = "cpu";
reg = <0>;
status = "okay";
};

cpu@1 {
device_type = "cpu";
reg = <1>;
status = "okay";
};
};

Regards,
Saurabh


2023-02-07 17:39:15

by Rob Herring (Arm)

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

On Fri, Feb 3, 2023 at 11:36 AM Saurabh Singh Sengar
<[email protected]> 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
> > > > <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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

2023-02-07 17:54:24

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v2 0/6] Device tree support for Hyper-V VMBus driver

On Wed, Feb 1, 2023 at 10:34 AM Saurabh Singh Sengar
<[email protected]> wrote:
>
> On Wed, Feb 01, 2023 at 08:51:46AM -0600, Rob Herring wrote:
> > On Tue, Jan 31, 2023 at 06:04:49PM -0800, Saurabh Singh Sengar wrote:
> > > On Tue, Jan 31, 2023 at 02:27:51PM -0600, Rob Herring wrote:
> > > > On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar
> > > > <[email protected]> wrote:
> > > > >
> > > > > This set of patches expands the VMBus driver to include device tree
> > > > > support.
> > > > >
> > > > > The first two patches enable compilation of Hyper-V APIs in a non-ACPI
> > > > > build.
> > > > >
> > > > > The third patch converts the VMBus driver from acpi to more generic
> > > > > platform driver.
> > > > >
> > > > > Further to add device tree documentation for VMBus, it needs to club with
> > > > > other virtualization driver's documentation. For this rename the virtio
> > > > > folder to more generic hypervisor, so that all the hypervisor based
> > > > > devices can co-exist in a single place in device tree documentation. The
> > > > > fourth patch does this renaming.
> > > > >
> > > > > The fifth patch introduces the device tree documentation for VMBus.
> > > > >
> > > > > The sixth patch adds device tree support to the VMBus driver. Currently
> > > > > this is tested only for x86 and it may not work for other archs.
> > > >
> > > > I can read all the patches and see *what* they do. You don't really
> > > > need to list that here. I'm still wondering *why*. That is what the
> > > > cover letter and commit messages should answer. Why do you need DT
> > > > support? How does this even work on x86? FDT is only enabled for
> > > > CE4100 platform.
> > >
> > > HI Rob,
> > >
> > > Thanks for your comments.
> > > We are working on a solution where kernel is booted without ACPI tables to keep
> > > the overall system's memory footprints slim and possibly faster boot time.
> > > We have tested this by enabling CONFIG_OF for x86.
> >
> > It's CONFIG_OF_EARLY_FLATTREE which you would need and that's not user
> > selectable. At a minimum, you need some kconfig changes. Where are
> > those?
>
> You are right we have define a new config flag in Kconfig, and selected CONFIG_OF
> and CONFIG_OF_EARLY_FLATTREE. We are working on upstreaming that patch as well
> however that will be a separate patch series.

Fair enough, but that should come first IMO. Really I just want to see
a complete picture. That can be a reference to a git branch(es) or
other patch series. But again, what I want to see in particular is the
actual DT and validation run on it.

> > Also see my comment on v1 about running DT validation on your dtb. I'm
> > sure running it would point out other issues. Such as the root level
> > comaptible string(s) need to be documented. You need cpu nodes,
> > interrupt controller, timers, etc. Those all have to be documented.
>
> I will be changing the parent node to soc node as suggested by Krzysztof
> in other thread.

Another issue yes, but orthogonal to my comments.

>
> soc {
> #address-cells = <2>;
> #size-cells = <2>;

You are missing 'ranges' here. Without it, addresses aren't translatable.

You are also missing 'compatible = "simple-bus";'. This happens to
work on x86 because of legacy reasons, but we don't want new cases
added.

>
> vmbus@ff0000000 {
> #address-cells = <2>;
> #size-cells = <1>;
> compatible = "Microsoft,vmbus";

'Microsoft' is not a vendor prefix.

> ranges = <0x00 0x00 0x0f 0xf0000000 0x10000000>;
> };
> };
>
> This will be sufficient.

All these comments are unnecessary because the tools will now check
these things and we shouldn't have to.

Rob

2023-02-07 18:37:50

by Sudeep Holla

[permalink] [raw]
Subject: Re: [PATCH v2 0/6] Device tree support for Hyper-V VMBus driver

On Tue, Jan 31, 2023 at 06:04:49PM -0800, Saurabh Singh Sengar wrote:
>
> We are working on a solution where kernel is booted without ACPI tables to keep
> the overall system's memory footprints slim and possibly faster boot time.
> We have tested this by enabling CONFIG_OF for x86.
>

Very interesting. Do you comparison on how slow/fast DT version w.r.t
ACPI and similarly for the memory footprint ? It would be good to add
those information as well. I know some systems just use ACPI static tables
as they don't run ACPICA runtime interpretter, was that experimented as
well or it wasn't used due to some specific(details please) limitations
without it.

> I can add this info in cover letter in next version.

Yes please.

--
Regards,
Sudeep

2023-02-08 02:30:57

by Saurabh Singh Sengar

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

On Tue, Feb 07, 2023 at 11:38:55AM -0600, Rob Herring wrote:
> On Fri, Feb 3, 2023 at 11:36 AM Saurabh Singh Sengar
> <[email protected]> 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
> > > > > <[email protected]> wrote:
> > > > 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.

Thanks for letting me know the challanges with testing. My intention was to give
ACPI higher priority, in case ACPI is enabled system should go ACPI flow, OF flow
should be used only when ACPI is disabled.

I can replace #else part with #ifdef CONFIG_OF if that helps.

Regards,
Saurabh

>
> Rob

2023-02-08 02:45:02

by Saurabh Singh Sengar

[permalink] [raw]
Subject: Re: [PATCH v2 0/6] Device tree support for Hyper-V VMBus driver

On Tue, Feb 07, 2023 at 11:53:54AM -0600, Rob Herring wrote:
> On Wed, Feb 1, 2023 at 10:34 AM Saurabh Singh Sengar
> <[email protected]> wrote:
> >
> > On Wed, Feb 01, 2023 at 08:51:46AM -0600, Rob Herring wrote:
> > > On Tue, Jan 31, 2023 at 06:04:49PM -0800, Saurabh Singh Sengar wrote:
> > > > On Tue, Jan 31, 2023 at 02:27:51PM -0600, Rob Herring wrote:
> > > > > On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > This set of patches expands the VMBus driver to include device tree
> > > > > > support.
(...)
> > You are right we have define a new config flag in Kconfig, and selected CONFIG_OF
> > and CONFIG_OF_EARLY_FLATTREE. We are working on upstreaming that patch as well
> > however that will be a separate patch series.
>
> Fair enough, but that should come first IMO. Really I just want to see
> a complete picture. That can be a reference to a git branch(es) or
> other patch series. But again, what I want to see in particular is the
> actual DT and validation run on it.

Thank you for explaining the concern. I now understand it fully. I have come to the
realization that enabling the vmbus device tree should not be impacted by any changes.
To address this, I will add the following lines to the HYPERV Kconfig definition I
used for testing:

select OF if !ACPI
select OF_EARLY_FLATTREE if !ACPI"

>
> > > Also see my comment on v1 about running DT validation on your dtb. I'm
> > > sure running it would point out other issues. Such as the root level
> > > comaptible string(s) need to be documented. You need cpu nodes,
> > > interrupt controller, timers, etc. Those all have to be documented.
> >
> > I will be changing the parent node to soc node as suggested by Krzysztof
> > in other thread.
>
> Another issue yes, but orthogonal to my comments.
>
> >
> > soc {
> > #address-cells = <2>;
> > #size-cells = <2>;
>
> You are missing 'ranges' here. Without it, addresses aren't translatable.
>
> You are also missing 'compatible = "simple-bus";'. This happens to
> work on x86 because of legacy reasons, but we don't want new cases
> added.

I am a bit unclear on the reason for adding the ranges property in the root node.
To provide more context, I have included my full device tree below. I believe that
having the ranges property in the VMBus device node is sufficient. Please let me
know if this can be improved.


/dts-v1/;

/ {
#address-cells = <0>;
#size-cells = <0>;
model = "microsoft,test";

cpus {
#address-cells = <0x01>;
#size-cells = <0x00>;

cpu@0 {
device_type = "cpu";
reg = <0x00>;
status = "okay";
};

cpu@1 {
device_type = "cpu";
reg = <0x01>;
status = "okay";
};
};

vmbus@ff0000000 {
#address-cells = <2>;
#size-cells = <2>;
compatible = "microsoft,vmbus";
ranges = <0x0f 0xf0000000 0x0f 0xf0000000 0x0 0x10000000>;
};
};

>
> >
> > vmbus@ff0000000 {
> > #address-cells = <2>;
> > #size-cells = <1>;
> > compatible = "Microsoft,vmbus";
>
> 'Microsoft' is not a vendor prefix.
>
> > ranges = <0x00 0x00 0x0f 0xf0000000 0x10000000>;
> > };
> > };
> >
> > This will be sufficient.
>
> All these comments are unnecessary because the tools will now check
> these things and we shouldn't have to.

Agree, and its fixed in latest version.

Regards,
Saurabh
>
> Rob