| Message ID | 20240417104046.27253-6-bchalios@amazon.es (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | virt: vmgenid: Add devicetree bindings support | expand |
Hi Sudan & especially Krzysztof, On Wed, Apr 17, 2024 at 12:43 PM Babis Chalios <bchalios@amazon.es> wrote: > struct vmgenid_state { > u8 *next_id; > u8 this_id[VMGENID_SIZE]; > + int irq; This is only ever used inside of one function. Why not just keep it on the stack? > }; > > static void vmgenid_notify(struct device *device) > @@ -43,6 +45,14 @@ vmgenid_acpi_handler(acpi_handle __always_unused handle, > vmgenid_notify(dev); > } > > +static irqreturn_t > +vmgenid_of_irq_handler(int __always_unused irq, void *dev) > +{ > + vmgenid_notify(dev); > + > + return IRQ_HANDLED; > +} Is there a reason the of code isn't conditional on CONFIG_OF? I'm not super familiar with these drivers, but this seems like it would be a thing to do, and then we could do `depends on OF || ACPI` in the Kconfig. After the whole Babis authorship debacle, I'm just fixing various things up in my own tree and I'll send out a v+1. But Krzysztof, I would really appreciate your review of this before I apply it to random-next. Jason
On 17/04/2024 17:16, Jason A. Donenfeld wrote: >> +static irqreturn_t >> +vmgenid_of_irq_handler(int __always_unused irq, void *dev) >> +{ >> + vmgenid_notify(dev); >> + >> + return IRQ_HANDLED; >> +} > > Is there a reason the of code isn't conditional on CONFIG_OF? I'm not > super familiar with these drivers, but this seems like it would be a > thing to do, and then we could do `depends on OF || ACPI` in the > Kconfig. > Usually we do not recommend hiding code behind !CONFIG_OF because this limits possible usage on ACPI systems via PRP0001. Not sure if it is applicable here, because there is already ACPI matching. I would suggest choose whatever makes code simpler... Or just mark some pieces with __maybe_unused if they are really not used? That would avoid ifdeffery. Best regards, Krzysztof
Hi Krzysztof, On Wed, Apr 17, 2024 at 6:02 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > Usually we do not recommend hiding code behind !CONFIG_OF because this > limits possible usage on ACPI systems via PRP0001. Not sure if it is > applicable here, because there is already ACPI matching. > > I would suggest choose whatever makes code simpler... Or just mark some > pieces with __maybe_unused if they are really not used? That would avoid > ifdeffery. Interesting about PRP0001. Alright. It looks like I can't quite do without ifdefs because the code dereferences ->handle in a acpi_device struct, but I can minimize it all to just a single ifdef. Jason
diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c index 3d93e3fb94c4..9a3c12e5dd6e 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 irqreturn_t +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; + + 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, }, };