Message ID | 20240409181154.9962-1-sudanl@amazon.com |
---|---|
Headers | show |
Series | virt: vmgenid: Add devicetree bindings support | expand |
On 09.04.24 20:11, Sudan Landge wrote: > Extend the vmgenid platform driver to support devicetree bindings. > With this support, hypervisors can send vmgenid notifications to > the virtual machine without the need to enable ACPI. > The bindings are located at: > Documentation/devicetree/bindings/rng/microsoft,vmgenid.yaml > > Signed-off-by: Sudan Landge <sudanl@amazon.com> > --- > drivers/virt/vmgenid.c | 53 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 51 insertions(+), 2 deletions(-) > > diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c > index 3d93e3fb94c4..e1ad74116c0c 100644 > --- a/drivers/virt/vmgenid.c > +++ b/drivers/virt/vmgenid.c > @@ -2,12 +2,13 @@ > /* > * Copyright (C) 2022 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved. > * > - * The "Virtual Machine Generation ID" is exposed via ACPI and changes when a > + * The "Virtual Machine Generation ID" is exposed via ACPI or DT and changes when a > * virtual machine forks or is cloned. This driver exists for shepherding that > * information to random.c. > */ > > #include <linux/acpi.h> > +#include <linux/interrupt.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/platform_device.h> > @@ -20,6 +21,7 @@ enum { VMGENID_SIZE = 16 }; > struct vmgenid_state { > u8 *next_id; > u8 this_id[VMGENID_SIZE]; > + int irq; > }; > > static void vmgenid_notify(struct device *device) > @@ -43,6 +45,14 @@ vmgenid_acpi_handler(acpi_handle __always_unused handle, > vmgenid_notify(dev); > } > > +static __maybe_unused irqreturn_t Why is this maybe_unused? It seems to be always referenced by vmgenid_add_of(), no? > +vmgenid_of_irq_handler(int __always_unused irq, void *dev) > +{ > + vmgenid_notify(dev); > + > + return IRQ_HANDLED; > +} > + > static int __maybe_unused > setup_vmgenid_state(struct vmgenid_state *state, u8 *next_id) > { > @@ -106,6 +116,35 @@ static int vmgenid_add_acpi(struct device __maybe_unused *dev, > #endif > } > > +static int vmgenid_add_of(struct platform_device *pdev, > + struct vmgenid_state *state) > +{ > + u8 *virt_addr; > + int ret = 0; > + > + virt_addr = (u8 *)devm_platform_get_and_ioremap_resource(pdev, 0, NULL); > + if (IS_ERR(virt_addr)) > + return PTR_ERR(virt_addr); > + > + ret = setup_vmgenid_state(state, virt_addr); > + if (ret) > + return ret; > + > + ret = platform_get_irq(pdev, 0); > + if (ret < 0) > + return ret; Doesn't this error path need to do something about the ioremap'ed resource? Or does devm do that automatically for you? Alex > + > + state->irq = ret; > + pdev->dev.driver_data = state; > + > + ret = devm_request_irq(&pdev->dev, state->irq, vmgenid_of_irq_handler, > + IRQF_SHARED, "vmgenid", &pdev->dev); > + if (ret) > + pdev->dev.driver_data = NULL; > + > + return ret; > +} > + > static int vmgenid_add(struct platform_device *pdev) > { > struct vmgenid_state *state; > @@ -116,7 +155,10 @@ static int vmgenid_add(struct platform_device *pdev) > if (!state) > return -ENOMEM; > > - ret = vmgenid_add_acpi(dev, state); > + if (dev->of_node) > + ret = vmgenid_add_of(pdev, state); > + else > + ret = vmgenid_add_acpi(dev, state); > > if (ret) > devm_kfree(dev, state); > @@ -124,6 +166,12 @@ static int vmgenid_add(struct platform_device *pdev) > return ret; > } > > +static const struct of_device_id vmgenid_of_ids[] = { > + { .compatible = "microsoft,vmgenid", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, vmgenid_of_ids); > + > static const struct acpi_device_id vmgenid_acpi_ids[] = { > { "VMGENCTR", 0 }, > { "VM_GEN_COUNTER", 0 }, > @@ -136,6 +184,7 @@ static struct platform_driver vmgenid_plaform_driver = { > .driver = { > .name = "vmgenid", > .acpi_match_table = vmgenid_acpi_ids, > + .of_match_table = vmgenid_of_ids, > }, > }; > Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On 16/4/24 12:49, Alexander Graf wrote: > > On 09.04.24 20:11, Sudan Landge wrote: >> Extend the vmgenid platform driver to support devicetree bindings. >> With this support, hypervisors can send vmgenid notifications to >> the virtual machine without the need to enable ACPI. >> The bindings are located at: >> Documentation/devicetree/bindings/rng/microsoft,vmgenid.yaml >> >> Signed-off-by: Sudan Landge <sudanl@amazon.com> >> --- >> drivers/virt/vmgenid.c | 53 ++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 51 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c >> index 3d93e3fb94c4..e1ad74116c0c 100644 >> --- a/drivers/virt/vmgenid.c >> +++ b/drivers/virt/vmgenid.c >> @@ -2,12 +2,13 @@ >> /* >> * Copyright (C) 2022 Jason A. Donenfeld <Jason@zx2c4.com>. All >> Rights Reserved. >> * >> - * The "Virtual Machine Generation ID" is exposed via ACPI and >> changes when a >> + * The "Virtual Machine Generation ID" is exposed via ACPI or DT and >> changes when a >> * virtual machine forks or is cloned. This driver exists for >> shepherding that >> * information to random.c. >> */ >> #include <linux/acpi.h> >> +#include <linux/interrupt.h> >> #include <linux/kernel.h> >> #include <linux/module.h> >> #include <linux/platform_device.h> >> @@ -20,6 +21,7 @@ enum { VMGENID_SIZE = 16 }; >> struct vmgenid_state { >> u8 *next_id; >> u8 this_id[VMGENID_SIZE]; >> + int irq; >> }; >> static void vmgenid_notify(struct device *device) >> @@ -43,6 +45,14 @@ vmgenid_acpi_handler(acpi_handle __always_unused >> handle, >> vmgenid_notify(dev); >> } >> +static __maybe_unused irqreturn_t > > > Why is this maybe_unused? It seems to be always referenced by > vmgenid_add_of(), no? You are right, Alex. I removed the attribute and build the kernel without `CONFIG_OF` without any warnings. I will remove it in the next version. > > >> +vmgenid_of_irq_handler(int __always_unused irq, void *dev) >> +{ >> + vmgenid_notify(dev); >> + >> + return IRQ_HANDLED; >> +} >> + >> static int __maybe_unused >> setup_vmgenid_state(struct vmgenid_state *state, u8 *next_id) >> { >> @@ -106,6 +116,35 @@ static int vmgenid_add_acpi(struct device >> __maybe_unused *dev, >> #endif >> } >> +static int vmgenid_add_of(struct platform_device *pdev, >> + struct vmgenid_state *state) >> +{ >> + u8 *virt_addr; >> + int ret = 0; >> + >> + virt_addr = (u8 *)devm_platform_get_and_ioremap_resource(pdev, >> 0, NULL); >> + if (IS_ERR(virt_addr)) >> + return PTR_ERR(virt_addr); >> + >> + ret = setup_vmgenid_state(state, virt_addr); >> + if (ret) >> + return ret; >> + >> + ret = platform_get_irq(pdev, 0); >> + if (ret < 0) >> + return ret; > > > Doesn't this error path need to do something about the ioremap'ed > resource? Or does devm do that automatically for you? devm should be doing this automatically according to this: https://docs.kernel.org/driver-api/driver-model/devres.html#devres Also, I took a quick look in other drivers and it looks like the virtio-mmio probe callback follows the same pattern: https://elixir.bootlin.com/linux/latest/source/drivers/virtio/virtio_mmio.c#L636 Cheers, Babis > > Alex > > >> + >> + state->irq = ret; >> + pdev->dev.driver_data = state; >> + >> + ret = devm_request_irq(&pdev->dev, state->irq, >> vmgenid_of_irq_handler, >> + IRQF_SHARED, "vmgenid", &pdev->dev); >> + if (ret) >> + pdev->dev.driver_data = NULL; >> + >> + return ret; >> +} >> + >> static int vmgenid_add(struct platform_device *pdev) >> { >> struct vmgenid_state *state; >> @@ -116,7 +155,10 @@ static int vmgenid_add(struct platform_device >> *pdev) >> if (!state) >> return -ENOMEM; >> - ret = vmgenid_add_acpi(dev, state); >> + if (dev->of_node) >> + ret = vmgenid_add_of(pdev, state); >> + else >> + ret = vmgenid_add_acpi(dev, state); >> if (ret) >> devm_kfree(dev, state); >> @@ -124,6 +166,12 @@ static int vmgenid_add(struct platform_device >> *pdev) >> return ret; >> } >> +static const struct of_device_id vmgenid_of_ids[] = { >> + { .compatible = "microsoft,vmgenid", }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(of, vmgenid_of_ids); >> + >> static const struct acpi_device_id vmgenid_acpi_ids[] = { >> { "VMGENCTR", 0 }, >> { "VM_GEN_COUNTER", 0 }, >> @@ -136,6 +184,7 @@ static struct platform_driver >> vmgenid_plaform_driver = { >> .driver = { >> .name = "vmgenid", >> .acpi_match_table = vmgenid_acpi_ids, >> + .of_match_table = vmgenid_of_ids, >> }, >> };