[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <c4caca2d-8840-4ed5-b580-f984bd0ee5cd@amazon.es>
Date: Tue, 16 Apr 2024 17:49:47 +0200
From: Babis Chalios <bchalios@...zon.es>
To: Alexander Graf <graf@...zon.de>, Sudan Landge <sudanl@...zon.com>,
<tytso@....edu>, <Jason@...c4.com>, <robh+dt@...nel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
<sathyanarayanan.kuppuswamy@...ux.intel.com>, <thomas.lendacky@....com>,
<dan.j.williams@...el.com>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
CC: <dwmw@...zon.co.uk>, <xmarcalx@...zon.co.uk>
Subject: Re: [PATCH v4 5/5] virt: vmgenid: add support for devicetree bindings
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@...zon.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@...c4.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,
>> },
>> };
Powered by blists - more mailing lists