Re: [PATCH v4 5/6] dt-bindings: hypervisor: VMBus

Re: [PATCH v4 5/6] dt-bindings: hypervisor: VMBus

[Rob Herring]

On Mon, 06 Feb 2023 23:49:58 -0800, Saurabh Sengar wrote:
> Add dt-bindings for Hyper-V VMBus.
> 
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
>  .../bindings/hypervisor/microsoft,vmbus.yaml       | 48 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hypervisor/microsoft,vmbus.yaml
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/serial/brcm,bcm6345-uart.example.dtb'
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1508: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):
Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/virtio/
MAINTAINERS: Documentation/devicetree/bindings/virtio/

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1675756199-5917-6-git-send-email-ssengar@linux.microsoft.com

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

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

pip3 install dtschema --upgrade

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 5/6] dt-bindings: hypervisor: VMBus

[Rob Herring]

On Tue, Feb 07, 2023 at 07:00:23AM -0600, Rob Herring wrote:
> 
> On Mon, 06 Feb 2023 23:49:58 -0800, Saurabh Sengar wrote:
> > Add dt-bindings for Hyper-V VMBus.
> > 
> > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > ---
> >  .../bindings/hypervisor/microsoft,vmbus.yaml       | 48 ++++++++++++++++++++++
> >  MAINTAINERS                                        |  1 +
> >  2 files changed, 49 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/hypervisor/microsoft,vmbus.yaml
> > 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> make[1]: *** Deleting file 'Documentation/devicetree/bindings/serial/brcm,bcm6345-uart.example.dtb'
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1508: dt_binding_check] Error 2

You can ignore this, it's a problem with the CI job.

> 
> doc reference errors (make refcheckdocs):
> Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/virtio/
> MAINTAINERS: Documentation/devicetree/bindings/virtio/

But this probably needs to be fixed.

Rob
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 4/6] dt-bindings: hypervisor: Rename virtio to hypervisor

[Rob Herring]

On Mon, Feb 06, 2023 at 11:49:57PM -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 <ssengar@linux.microsoft.com>
> ---
>  Documentation/devicetree/bindings/gpio/gpio-virtio.yaml               | 4 ++--
>  Documentation/devicetree/bindings/{virtio => hypervisor}/mmio.yaml    | 2 +-
>  .../devicetree/bindings/{virtio => hypervisor}/pci-iommu.yaml         | 2 +-
>  .../devicetree/bindings/{virtio => hypervisor}/virtio-device.yaml     | 2 +-
>  Documentation/devicetree/bindings/i2c/i2c-virtio.yaml                 | 4 ++--
>  MAINTAINERS                                                           | 2 +-
>  6 files changed, 8 insertions(+), 8 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%)

virtio is used for more than just an interface to hypervisors. I think 
this should remain. Instead, I'd put vmbus under bindings/bus/.

Rob
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael Kelley (LINUX) via Virtualization]

From: Saurabh Sengar <ssengar@linux.microsoft.com> Sent: Monday, February 6, 2023 11:50 PM
> 
> Add a placeholder function for the hv_setup_stimer0_irq API to accommodate
> systems without ACPI support. Since this function is not utilized on
> x86/x64 systems and non-ACPI support is only intended for x86/x64 systems,
> a placeholder function is sufficient for now and can be improved upon if
> necessary in the future.
> 
> This change will make it easier to add device tree support for VMBus in
> subsequent commits.
> 
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
>  drivers/clocksource/hyperv_timer.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
> index c0cef92..f32948c 100644
> --- a/drivers/clocksource/hyperv_timer.c
> +++ b/drivers/clocksource/hyperv_timer.c
> @@ -49,7 +49,7 @@
> 
>  static int stimer0_irq = -1;
>  static int stimer0_message_sint;
> -static DEFINE_PER_CPU(long, stimer0_evt);
> +static __maybe_unused DEFINE_PER_CPU(long, stimer0_evt);
> 
>  /*
>   * Common code for stimer0 interrupts coming via Direct Mode or
> @@ -68,7 +68,7 @@ void hv_stimer0_isr(void)
>   * stimer0 interrupt handler for architectures that support
>   * per-cpu interrupts, which also implies Direct Mode.
>   */
> -static irqreturn_t hv_stimer0_percpu_isr(int irq, void *dev_id)
> +static irqreturn_t __maybe_unused hv_stimer0_percpu_isr(int irq, void *dev_id)
>  {
>  	hv_stimer0_isr();
>  	return IRQ_HANDLED;
> @@ -196,6 +196,7 @@ void __weak hv_remove_stimer0_handler(void)
>  {
>  };
> 
> +#ifdef CONFIG_ACPI
>  /* Called only on architectures with per-cpu IRQs (i.e., not x86/x64) */
>  static int hv_setup_stimer0_irq(void)
>  {
> @@ -230,6 +231,16 @@ static void hv_remove_stimer0_irq(void)
>  		stimer0_irq = -1;
>  	}
>  }
> +#else
> +static int hv_setup_stimer0_irq(void)
> +{
> +	return 0;
> +}
> +
> +static void hv_remove_stimer0_irq(void)
> +{
> +}
> +#endif
> 
>  /* hv_stimer_alloc - Global initialization of the clockevent and stimer0 */
>  int hv_stimer_alloc(bool have_percpu_irqs)
> --
> 1.8.3.1

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH v4 2/6] Drivers: hv: allow non ACPI compilation for hv_is_hibernation_supported

[Michael Kelley (LINUX) via Virtualization]

From: Saurabh Sengar <ssengar@linux.microsoft.com> Sent: Monday, February 6, 2023 11:50 PM
> 
> acpi_sleep_state_supported API is only define for CONFIG_ACPI flag and
> thus it can't be used for non-ACPI builds. Initially there won't be
> hibernation support for non ACPI builds.
> 
> This change will help adding device tree support in subsequent commits.

In keeping with the guideline to avoid references like "this patch" or "this
change" in commit messages, I'd suggest wording the commit message
something like the following:

acpi_sleep_state_supported() currently is defined only when
CONFIG_ACPI=y.  For future work to enable device tree builds, put this
function under #ifdef CONFIG_ACPI.  Otherwise, return 'false' from
hv_is_hibernation_supported() as Hyper-V guest configs using device
tree don't support hibernation.

With that update,
Reviewed-by: Michael Kelley

I gave a Reviewed-by on Patch 1 of this series, but the same comment
applies about "this change" in that commit message.

Michael

> 
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
>  drivers/hv/hv_common.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 52a6f89..370ec20 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -234,7 +234,11 @@ void hv_setup_dma_ops(struct device *dev, bool coherent)
> 
>  bool hv_is_hibernation_supported(void)
>  {
> +#ifdef CONFIG_ACPI
>  	return !hv_root_partition && acpi_sleep_state_supported(ACPI_STATE_S4);
> +#else
> +	return false;
> +#endif
>  }
>  EXPORT_SYMBOL_GPL(hv_is_hibernation_supported);
> 
> --
> 1.8.3.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH v4 3/6] Drivers: hv: vmbus: Convert acpi_device to more generic platform_device

[Michael Kelley (LINUX) via Virtualization]

From: Saurabh Sengar <ssengar@linux.microsoft.com> Sent: Monday, February 6, 2023 11:50 PM
> 
> Use more generic platform device instead of acpi device. Also rename the
> function vmbus_acpi_remove to more generic name vmbus_mmio_remove.

Let me suggest a slightly expanded commit message along these lines:

VMBus driver code currently has direct dependency on ACPI and struct
acpi_device.  As a staging step toward optionally configuring based on
device tree instead of ACPI, use a more generic platform device to reduce
the dependency on ACPI where possible, though the dependency on ACPI
is not completely removed.  Also rename the function vmbus_acpi_remove()
to the more generic vmbus_mmio_remove().

With this tweak,

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

> 
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
> [V4]
> - rebased which fixed return type of 'vmbus_mmio_remove' from int to void
> 
>  drivers/hv/vmbus_drv.c | 58 ++++++++++++++++++++++++++++++--------------------
>  1 file changed, 35 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index d24dd65..7349715 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 device  *hv_dev;
> 
>  static int hyperv_cpuhp_online;
> 
> @@ -143,7 +144,7 @@ static int hv_die_panic_notify_crash(struct notifier_block *self,
> 
>  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_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;
>  	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,14 @@ 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 acpi_device *device = ACPI_COMPANION(&pdev->dev);
> 
> -	hv_acpi_dev = device;
> +	hv_dev = &device->dev;
> 
>  	/*
>  	 * Older versions of Hyper-V for ARM64 fail to include the _CCA
> @@ -2489,10 +2491,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 +2671,15 @@ static int vmbus_bus_resume(struct device *dev)
>  	.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 +2763,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 +2797,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 +2851,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);
>  }
> 
> 
> --
> 1.8.3.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization