Re: [PATCH v4 5/5] virt: vmgenid: add support for devicetree bindings

[Babis Chalios]

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@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxx>. 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,
> >       },
> >   };