Update the driver to support device tree boot as well along with ACPI.
At present the device tree parsing only provides the mmio region info
and is not the exact copy of ACPI parsing. This is sufficient to cater
all the current device tree usecases for VMBus.
Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
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.1On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar
<ssengar@linux.microsoft.com> wrote:
>
> Update the driver to support device tree boot as well along with ACPI.
> At present the device tree parsing only provides the mmio region info
> and is not the exact copy of ACPI parsing. This is sufficient to cater
> all the current device tree usecases for VMBus.
>
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
> 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
>
On Tue, Jan 31, 2023 at 02:12:53PM -0600, Rob Herring wrote:
> On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar
> <ssengar@linux.microsoft.com> wrote:
> >
> > Update the driver to support device tree boot as well along with ACPI.
> > At present the device tree parsing only provides the mmio region info
> > and is not the exact copy of ACPI parsing. This is sufficient to cater
> > all the current device tree usecases for VMBus.
> >
> > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > ---
> > 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
> >
On Wed, Feb 01, 2023 at 08:51:33AM -0800, Saurabh Singh Sengar wrote:
> On Tue, Jan 31, 2023 at 02:12:53PM -0600, Rob Herring wrote:
> > On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar
> > <ssengar@linux.microsoft.com> wrote:
> > >
> > > Update the driver to support device tree boot as well along with ACPI.
> > > At present the device tree parsing only provides the mmio region info
> > > and is not the exact copy of ACPI parsing. This is sufficient to cater
> > > all the current device tree usecases for VMBus.
> > >
> > > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > > ---
> > > 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/
On Wed, Feb 01, 2023 at 11:46:38AM -0600, Rob Herring wrote:
> On Wed, Feb 01, 2023 at 08:51:33AM -0800, Saurabh Singh Sengar wrote:
> > On Tue, Jan 31, 2023 at 02:12:53PM -0600, Rob Herring wrote:
> > > On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar
> > > <ssengar@linux.microsoft.com> wrote:
> > > >
> > > > Update the driver to support device tree boot as well along with ACPI.
> > > > At present the device tree parsing only provides the mmio region info
> > > > and is not the exact copy of ACPI parsing. This is sufficient to cater
> > > > all the current device tree usecases for VMBus.
> > > >
> > > > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > > > ---
> > > > 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/
On Fri, Feb 3, 2023 at 11:36 AM Saurabh Singh Sengar
<ssengar@linux.microsoft.com> wrote:
>
> On Wed, Feb 01, 2023 at 11:46:38AM -0600, Rob Herring wrote:
> > On Wed, Feb 01, 2023 at 08:51:33AM -0800, Saurabh Singh Sengar wrote:
> > > On Tue, Jan 31, 2023 at 02:12:53PM -0600, Rob Herring wrote:
> > > > On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar
> > > > <ssengar@linux.microsoft.com> wrote:
> > > > >
> > > > > Update the driver to support device tree boot as well along with ACPI.
> > > > > At present the device tree parsing only provides the mmio region info
> > > > > and is not the exact copy of ACPI parsing. This is sufficient to cater
> > > > > all the current device tree usecases for VMBus.
> > > > >
> > > > > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > > > > ---
> > > > > 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
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
> <ssengar@linux.microsoft.com> wrote:
> >
> > On Wed, Feb 01, 2023 at 11:46:38AM -0600, Rob Herring wrote:
> > > On Wed, Feb 01, 2023 at 08:51:33AM -0800, Saurabh Singh Sengar wrote:
> > > > On Tue, Jan 31, 2023 at 02:12:53PM -0600, Rob Herring wrote:
> > > > > On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar
> > > > > <ssengar@linux.microsoft.com> 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
On Wed, Feb 01, 2023 at 11:46:38AM -0600, Rob Herring wrote:
> On Wed, Feb 01, 2023 at 08:51:33AM -0800, Saurabh Singh Sengar wrote:
> > On Tue, Jan 31, 2023 at 02:12:53PM -0600, Rob Herring wrote:
> > > On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar
> > > <ssengar@linux.microsoft.com> 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.
© 2016 - 2025 Red Hat, Inc.