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

[Babis Chalios <bchalios@amazon.es>]

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,
>>       },
>>   };