- 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/vmgenid.yaml
- Use proper FLAGS to compile with and without ACPI and/or devicetree.
Signed-off-by: Sudan Landge <sudanl@amazon.com>
---
drivers/virt/Kconfig | 1 -
drivers/virt/vmgenid.c | 85 +++++++++++++++++++++++++++++++++++++++---
2 files changed, 80 insertions(+), 6 deletions(-)
diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
index 40129b6f0eca..d8c848cf09a6 100644
--- a/drivers/virt/Kconfig
+++ b/drivers/virt/Kconfig
@@ -16,7 +16,6 @@ if VIRT_DRIVERS
config VMGENID
tristate "Virtual Machine Generation ID driver"
default y
- depends on ACPI
help
Say Y here to use the hypervisor-provided Virtual Machine Generation ID
to reseed the RNG when the VM is cloned. This is highly recommended if
diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
index d5394c706bd9..a648465bea43 100644
--- a/drivers/virt/vmgenid.c
+++ b/drivers/virt/vmgenid.c
@@ -2,7 +2,7 @@
/*
* 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.
*/
@@ -21,6 +21,7 @@ enum { VMGENID_SIZE = 16 };
struct vmgenid_state {
u8 *next_id;
u8 this_id[VMGENID_SIZE];
+ unsigned int irq;
};
static void vmgenid_notify(struct device *device)
@@ -37,10 +38,24 @@ static void vmgenid_notify(struct device *device)
kobject_uevent_env(&device->kobj, KOBJ_CHANGE, envp);
}
+#if IS_ENABLED(CONFIG_ACPI)
static void vmgenid_acpi_handler(acpi_handle handle, u32 event, void *dev)
{
+ (void)handle;
+ (void)event;
vmgenid_notify(dev);
}
+#endif
+
+#if IS_ENABLED(CONFIG_OF)
+static irqreturn_t vmgenid_of_irq_handler(int irq, void *dev)
+{
+ (void)irq;
+ vmgenid_notify(dev);
+
+ return IRQ_HANDLED;
+}
+#endif
static int setup_vmgenid_state(struct vmgenid_state *state, u8 *next_id)
{
@@ -55,6 +70,7 @@ static int setup_vmgenid_state(struct vmgenid_state *state, u8 *next_id)
static int vmgenid_add_acpi(struct device *dev, struct vmgenid_state *state)
{
+#if IS_ENABLED(CONFIG_ACPI)
struct acpi_device *device = ACPI_COMPANION(dev);
struct acpi_buffer parsed = { ACPI_ALLOCATE_BUFFER };
union acpi_object *obj;
@@ -96,6 +112,49 @@ static int vmgenid_add_acpi(struct device *dev, struct vmgenid_state *state)
out:
ACPI_FREE(parsed.pointer);
return ret;
+#else
+ (void)dev;
+ (void)state;
+ return -EINVAL;
+#endif
+}
+
+static int vmgenid_add_of(struct platform_device *pdev, struct vmgenid_state *state)
+{
+#if IS_ENABLED(CONFIG_OF)
+ void __iomem *remapped_ptr;
+ int ret = 0;
+
+ remapped_ptr = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
+ if (IS_ERR(remapped_ptr)) {
+ ret = PTR_ERR(remapped_ptr);
+ goto out;
+ }
+
+ ret = setup_vmgenid_state(state, remapped_ptr);
+ if (ret)
+ goto out;
+
+ state->irq = platform_get_irq(pdev, 0);
+ if (state->irq < 0) {
+ ret = state->irq;
+ goto out;
+ }
+ 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;
+
+out:
+ return ret;
+#else
+ (void)dev;
+ (void)state;
+ return -EINVAL;
+#endif
}
static int vmgenid_add(struct platform_device *pdev)
@@ -108,7 +167,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);
@@ -116,18 +178,31 @@ static int vmgenid_add(struct platform_device *pdev)
return ret;
}
-static const struct acpi_device_id vmgenid_ids[] = {
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id vmgenid_of_ids[] = {
+ { .compatible = "linux,vmgenctr", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, vmgenid_of_ids);
+#endif
+
+#if IS_ENABLED(CONFIG_ACPI)
+static const struct acpi_device_id vmgenid_acpi_ids[] = {
{ "VMGENCTR", 0 },
{ "VM_GEN_COUNTER", 0 },
{ }
};
-MODULE_DEVICE_TABLE(acpi, vmgenid_ids);
+MODULE_DEVICE_TABLE(acpi, vmgenid_acpi_ids);
+#endif
static struct platform_driver vmgenid_plaform_driver = {
.probe = vmgenid_add,
.driver = {
.name = "vmgenid",
- .acpi_match_table = ACPI_PTR(vmgenid_ids),
+ .acpi_match_table = ACPI_PTR(vmgenid_acpi_ids),
+#if IS_ENABLED(CONFIG_OF)
+ .of_match_table = vmgenid_of_ids,
+#endif
.owner = THIS_MODULE,
},
};
--
2.40.1Hi Sudan,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 8e938e39866920ddc266898e6ae1fffc5c8f51aa]
url: https://github.com/intel-lab-lkp/linux/commits/Sudan-Landge/virt-vmgenid-rearrange-code-to-make-review-easier/20240326-035657
base: 8e938e39866920ddc266898e6ae1fffc5c8f51aa
patch link: https://lore.kernel.org/r/20240325195306.13133-5-sudanl%40amazon.com
patch subject: [PATCH v3 4/4] virt: vmgenid: add support for devicetree bindings
config: x86_64-buildonly-randconfig-004-20240326 (https://download.01.org/0day-ci/archive/20240327/202403270050.wJ0Hd5aB-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240327/202403270050.wJ0Hd5aB-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403270050.wJ0Hd5aB-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/virt/vmgenid.c: In function 'vmgenid_add_of':
drivers/virt/vmgenid.c:154:15: error: 'dev' undeclared (first use in this function); did you mean 'pdev'?
154 | (void)dev;
| ^~~
| pdev
drivers/virt/vmgenid.c:154:15: note: each undeclared identifier is reported only once for each function it appears in
drivers/virt/vmgenid.c: At top level:
>> drivers/virt/vmgenid.c:60:12: warning: 'setup_vmgenid_state' defined but not used [-Wunused-function]
60 | static int setup_vmgenid_state(struct vmgenid_state *state, u8 *next_id)
| ^~~~~~~~~~~~~~~~~~~
>> drivers/virt/vmgenid.c:27:13: warning: 'vmgenid_notify' defined but not used [-Wunused-function]
27 | static void vmgenid_notify(struct device *device)
| ^~~~~~~~~~~~~~
vim +/setup_vmgenid_state +60 drivers/virt/vmgenid.c
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 26
fba2c3554a30ca Sudan Landge 2024-03-25 @27 static void vmgenid_notify(struct device *device)
5eb78dfcd888d3 Sudan Landge 2024-03-25 28 {
fba2c3554a30ca Sudan Landge 2024-03-25 29 struct vmgenid_state *state = device->driver_data;
5eb78dfcd888d3 Sudan Landge 2024-03-25 30 char *envp[] = { "NEW_VMGENID=1", NULL };
5eb78dfcd888d3 Sudan Landge 2024-03-25 31 u8 old_id[VMGENID_SIZE];
5eb78dfcd888d3 Sudan Landge 2024-03-25 32
5eb78dfcd888d3 Sudan Landge 2024-03-25 33 memcpy(old_id, state->this_id, sizeof(old_id));
5eb78dfcd888d3 Sudan Landge 2024-03-25 34 memcpy(state->this_id, state->next_id, sizeof(state->this_id));
5eb78dfcd888d3 Sudan Landge 2024-03-25 35 if (!memcmp(old_id, state->this_id, sizeof(old_id)))
5eb78dfcd888d3 Sudan Landge 2024-03-25 36 return;
5eb78dfcd888d3 Sudan Landge 2024-03-25 37 add_vmfork_randomness(state->this_id, sizeof(state->this_id));
fba2c3554a30ca Sudan Landge 2024-03-25 38 kobject_uevent_env(&device->kobj, KOBJ_CHANGE, envp);
5eb78dfcd888d3 Sudan Landge 2024-03-25 39 }
5eb78dfcd888d3 Sudan Landge 2024-03-25 40
84d2b6732ae89f Sudan Landge 2024-03-25 41 #if IS_ENABLED(CONFIG_ACPI)
fba2c3554a30ca Sudan Landge 2024-03-25 42 static void vmgenid_acpi_handler(acpi_handle handle, u32 event, void *dev)
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 43 {
84d2b6732ae89f Sudan Landge 2024-03-25 44 (void)handle;
84d2b6732ae89f Sudan Landge 2024-03-25 45 (void)event;
fba2c3554a30ca Sudan Landge 2024-03-25 46 vmgenid_notify(dev);
fba2c3554a30ca Sudan Landge 2024-03-25 47 }
84d2b6732ae89f Sudan Landge 2024-03-25 48 #endif
84d2b6732ae89f Sudan Landge 2024-03-25 49
84d2b6732ae89f Sudan Landge 2024-03-25 50 #if IS_ENABLED(CONFIG_OF)
84d2b6732ae89f Sudan Landge 2024-03-25 51 static irqreturn_t vmgenid_of_irq_handler(int irq, void *dev)
84d2b6732ae89f Sudan Landge 2024-03-25 52 {
84d2b6732ae89f Sudan Landge 2024-03-25 53 (void)irq;
84d2b6732ae89f Sudan Landge 2024-03-25 54 vmgenid_notify(dev);
84d2b6732ae89f Sudan Landge 2024-03-25 55
84d2b6732ae89f Sudan Landge 2024-03-25 56 return IRQ_HANDLED;
84d2b6732ae89f Sudan Landge 2024-03-25 57 }
84d2b6732ae89f Sudan Landge 2024-03-25 58 #endif
fba2c3554a30ca Sudan Landge 2024-03-25 59
fba2c3554a30ca Sudan Landge 2024-03-25 @60 static int setup_vmgenid_state(struct vmgenid_state *state, u8 *next_id)
fba2c3554a30ca Sudan Landge 2024-03-25 61 {
fba2c3554a30ca Sudan Landge 2024-03-25 62 if (IS_ERR(next_id))
fba2c3554a30ca Sudan Landge 2024-03-25 63 return PTR_ERR(next_id);
fba2c3554a30ca Sudan Landge 2024-03-25 64
fba2c3554a30ca Sudan Landge 2024-03-25 65 state->next_id = next_id;
fba2c3554a30ca Sudan Landge 2024-03-25 66 memcpy(state->this_id, state->next_id, sizeof(state->this_id));
fba2c3554a30ca Sudan Landge 2024-03-25 67 add_device_randomness(state->this_id, sizeof(state->this_id));
fba2c3554a30ca Sudan Landge 2024-03-25 68 return 0;
fba2c3554a30ca Sudan Landge 2024-03-25 69 }
fba2c3554a30ca Sudan Landge 2024-03-25 70
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Sudan,
kernel test robot noticed the following build errors:
[auto build test ERROR on 8e938e39866920ddc266898e6ae1fffc5c8f51aa]
url: https://github.com/intel-lab-lkp/linux/commits/Sudan-Landge/virt-vmgenid-rearrange-code-to-make-review-easier/20240326-035657
base: 8e938e39866920ddc266898e6ae1fffc5c8f51aa
patch link: https://lore.kernel.org/r/20240325195306.13133-5-sudanl%40amazon.com
patch subject: [PATCH v3 4/4] virt: vmgenid: add support for devicetree bindings
config: x86_64-randconfig-161-20240326 (https://download.01.org/0day-ci/archive/20240326/202403262327.ZwiqykRF-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240326/202403262327.ZwiqykRF-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403262327.ZwiqykRF-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/virt/vmgenid.c:154:8: error: use of undeclared identifier 'dev'
154 | (void)dev;
| ^
1 error generated.
vim +/dev +154 drivers/virt/vmgenid.c
121
122 static int vmgenid_add_of(struct platform_device *pdev, struct vmgenid_state *state)
123 {
124 #if IS_ENABLED(CONFIG_OF)
125 void __iomem *remapped_ptr;
126 int ret = 0;
127
128 remapped_ptr = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
129 if (IS_ERR(remapped_ptr)) {
130 ret = PTR_ERR(remapped_ptr);
131 goto out;
132 }
133
134 ret = setup_vmgenid_state(state, remapped_ptr);
135 if (ret)
136 goto out;
137
138 state->irq = platform_get_irq(pdev, 0);
139 if (state->irq < 0) {
140 ret = state->irq;
141 goto out;
142 }
143 pdev->dev.driver_data = state;
144
145 ret = devm_request_irq(&pdev->dev, state->irq,
146 vmgenid_of_irq_handler,
147 IRQF_SHARED, "vmgenid", &pdev->dev);
148 if (ret)
149 pdev->dev.driver_data = NULL;
150
151 out:
152 return ret;
153 #else
> 154 (void)dev;
155 (void)state;
156 return -EINVAL;
157 #endif
158 }
159
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Sudan,
kernel test robot noticed the following build errors:
[auto build test ERROR on 8e938e39866920ddc266898e6ae1fffc5c8f51aa]
url: https://github.com/intel-lab-lkp/linux/commits/Sudan-Landge/virt-vmgenid-rearrange-code-to-make-review-easier/20240326-035657
base: 8e938e39866920ddc266898e6ae1fffc5c8f51aa
patch link: https://lore.kernel.org/r/20240325195306.13133-5-sudanl%40amazon.com
patch subject: [PATCH v3 4/4] virt: vmgenid: add support for devicetree bindings
config: x86_64-rhel-8.3-bpf (https://download.01.org/0day-ci/archive/20240326/202403262047.aZVjmDY5-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240326/202403262047.aZVjmDY5-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403262047.aZVjmDY5-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/virt/vmgenid.c: In function 'vmgenid_add_of':
>> drivers/virt/vmgenid.c:154:15: error: 'dev' undeclared (first use in this function); did you mean 'pdev'?
154 | (void)dev;
| ^~~
| pdev
drivers/virt/vmgenid.c:154:15: note: each undeclared identifier is reported only once for each function it appears in
vim +154 drivers/virt/vmgenid.c
121
122 static int vmgenid_add_of(struct platform_device *pdev, struct vmgenid_state *state)
123 {
124 #if IS_ENABLED(CONFIG_OF)
125 void __iomem *remapped_ptr;
126 int ret = 0;
127
128 remapped_ptr = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
129 if (IS_ERR(remapped_ptr)) {
130 ret = PTR_ERR(remapped_ptr);
131 goto out;
132 }
133
134 ret = setup_vmgenid_state(state, remapped_ptr);
135 if (ret)
136 goto out;
137
138 state->irq = platform_get_irq(pdev, 0);
139 if (state->irq < 0) {
140 ret = state->irq;
141 goto out;
142 }
143 pdev->dev.driver_data = state;
144
145 ret = devm_request_irq(&pdev->dev, state->irq,
146 vmgenid_of_irq_handler,
147 IRQF_SHARED, "vmgenid", &pdev->dev);
148 if (ret)
149 pdev->dev.driver_data = NULL;
150
151 out:
152 return ret;
153 #else
> 154 (void)dev;
155 (void)state;
156 return -EINVAL;
157 #endif
158 }
159
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 26/03/2024 13:48, kernel test robot wrote:
> Hi Sudan,
>
> kernel test robot noticed the following build errors:
...
> 134 ret = setup_vmgenid_state(state, remapped_ptr);
> 135 if (ret)
> 136 goto out;
> 137
> 138 state->irq = platform_get_irq(pdev, 0);
> 139 if (state->irq < 0) {
> 140 ret = state->irq;
> 141 goto out;
> 142 }
> 143 pdev->dev.driver_data = state;
> 144
> 145 ret = devm_request_irq(&pdev->dev, state->irq,
> 146 vmgenid_of_irq_handler,
> 147 IRQF_SHARED, "vmgenid", &pdev->dev);
> 148 if (ret)
> 149 pdev->dev.driver_data = NULL;
> 150
> 151 out:
> 152 return ret;
> 153 #else
> > 154 (void)dev;
So this code was not even built...
Best regards,
Krzysztof
On 26/03/2024 12:53, Krzysztof Kozlowski wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On 26/03/2024 13:48, kernel test robot wrote:
>> Hi Sudan,
>>
>> kernel test robot noticed the following build errors:
>
> ...
>
>> 134 ret = setup_vmgenid_state(state, remapped_ptr);
>> 135 if (ret)
>> 136 goto out;
>> 137
>> 138 state->irq = platform_get_irq(pdev, 0);
>> 139 if (state->irq < 0) {
>> 140 ret = state->irq;
>> 141 goto out;
>> 142 }
>> 143 pdev->dev.driver_data = state;
>> 144
>> 145 ret = devm_request_irq(&pdev->dev, state->irq,
>> 146 vmgenid_of_irq_handler,
>> 147 IRQF_SHARED, "vmgenid", &pdev->dev);
>> 148 if (ret)
>> 149 pdev->dev.driver_data = NULL;
>> 150
>> 151 out:
>> 152 return ret;
>> 153 #else
>> > 154 (void)dev;
>
> So this code was not even built...
>
> Best regards,
> Krzysztof
>
I built it with CONFIG_ACPI and CONFIG_OF enabled but missed to build it
without the CONFIG_OF flag. As mentioned in the other mail I'll make
sure to run all required tools and check for all combinations before
posting future patches.
On 25/03/2024 20:53, 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/vmgenid.yaml
>
> - Use proper FLAGS to compile with and without ACPI and/or devicetree.
I do not see any flags. You use if/ifdefs which is a NAK.
Do not mix independent changes within one commit. Please follow
guidelines in submitting-patches for this.
>
> Signed-off-by: Sudan Landge <sudanl@amazon.com>
> ---
> drivers/virt/Kconfig | 1 -
> drivers/virt/vmgenid.c | 85 +++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 80 insertions(+), 6 deletions(-)
>
...
> +
> +#if IS_ENABLED(CONFIG_OF)
> +static irqreturn_t vmgenid_of_irq_handler(int irq, void *dev)
> +{
> + (void)irq;
> + vmgenid_notify(dev);
> +
> + return IRQ_HANDLED;
> +}
> +#endif
>
> static int setup_vmgenid_state(struct vmgenid_state *state, u8 *next_id)
> {
> @@ -55,6 +70,7 @@ static int setup_vmgenid_state(struct vmgenid_state *state, u8 *next_id)
>
> static int vmgenid_add_acpi(struct device *dev, struct vmgenid_state *state)
> {
> +#if IS_ENABLED(CONFIG_ACPI)
Why do you create all this ifdeffery in the code? You already got
comments to get rid of all this #if.
> struct acpi_device *device = ACPI_COMPANION(dev);
> struct acpi_buffer parsed = { ACPI_ALLOCATE_BUFFER };
> union acpi_object *obj;
> @@ -96,6 +112,49 @@ static int vmgenid_add_acpi(struct device *dev, struct vmgenid_state *state)
> out:
> ACPI_FREE(parsed.pointer);
> return ret;
> +#else
> + (void)dev;
> + (void)state;
> + return -EINVAL;
> +#endif
> +}
> +
> +static int vmgenid_add_of(struct platform_device *pdev, struct vmgenid_state *state)
> +{
> +#if IS_ENABLED(CONFIG_OF)
> + void __iomem *remapped_ptr;
> + int ret = 0;
> +
> + remapped_ptr = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> + if (IS_ERR(remapped_ptr)) {
> + ret = PTR_ERR(remapped_ptr);
> + goto out;
> + }
> +
> + ret = setup_vmgenid_state(state, remapped_ptr);
> + if (ret)
> + goto out;
> +
> + state->irq = platform_get_irq(pdev, 0);
> + if (state->irq < 0) {
> + ret = state->irq;
> + goto out;
> + }
> + 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;
> +
> +out:
> + return ret;
> +#else
That's neither readable code nor matching Linux coding style. Driver
don't do such magic. Please open some recent drivers for ACPI and OF and
look there. Old drivers had stubs for !CONFIG_XXX, but new drivers don't
have even that.
> + (void)dev;
> + (void)state;
> + return -EINVAL;
> +#endif
> }
>
> static int vmgenid_add(struct platform_device *pdev)
> @@ -108,7 +167,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);
> @@ -116,18 +178,31 @@ static int vmgenid_add(struct platform_device *pdev)
> return ret;
> }
>
> -static const struct acpi_device_id vmgenid_ids[] = {
> +#if IS_ENABLED(CONFIG_OF)
No, drop.
> +static const struct of_device_id vmgenid_of_ids[] = {
> + { .compatible = "linux,vmgenctr", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, vmgenid_of_ids);
> +#endif
> +
> +#if IS_ENABLED(CONFIG_ACPI)
> +static const struct acpi_device_id vmgenid_acpi_ids[] = {
> { "VMGENCTR", 0 },
> { "VM_GEN_COUNTER", 0 },
> { }
> };
> -MODULE_DEVICE_TABLE(acpi, vmgenid_ids);
> +MODULE_DEVICE_TABLE(acpi, vmgenid_acpi_ids);
> +#endif
>
> static struct platform_driver vmgenid_plaform_driver = {
> .probe = vmgenid_add,
> .driver = {
> .name = "vmgenid",
> - .acpi_match_table = ACPI_PTR(vmgenid_ids),
> + .acpi_match_table = ACPI_PTR(vmgenid_acpi_ids),
> +#if IS_ENABLED(CONFIG_OF)
No, really, this is some ancient code.
Please point me to single driver doing such ifdef.
> + .of_match_table = vmgenid_of_ids,
> +#endif
> .owner = THIS_MODULE,
This is clearly some abandoned driver... sigh... I thought we get rid of
all this owner crap. Many years ago. How could it appear back if
automated tools report it?
Considering how many failures LKP reported for your patchsets, I have
real doubts that anyone actually tests this code.
Please confirm:
Did you build W=1, run smatch, sparse and coccinelle on the driver after
your changes?
Best regards,
Krzysztof
On Mon, Mar 25, 2024 at 10:51:25PM +0100, Krzysztof Kozlowski wrote: > > .owner = THIS_MODULE, > > This is clearly some abandoned driver... sigh... I thought we get rid of > all this owner crap. Many years ago. How could it appear back if > automated tools report it? > > Considering how many failures LKP reported for your patchsets, I have > real doubts that anyone actually tests this code. Now you're commenting on the context rather than the patch. No, this isn't an abandoned driver, no it's not untested. Rather, it's code I maintain, care deeply about, and have a tree that receives quite a bit of testing (random.git) where I'll be taking these OF patches in the case that this patchset improves (and thanks very much for your review on it; I'll be appreciative of your ack whenever/if ever it improves to that point), and if you have other cleanups like removing .owner, please don't hesitate to send a patch. That all is to say, I'm following these threads closely and care. Jason
On 26/03/2024 15:10, Jason A. Donenfeld wrote: > On Mon, Mar 25, 2024 at 10:51:25PM +0100, Krzysztof Kozlowski wrote: >>> .owner = THIS_MODULE, >> >> This is clearly some abandoned driver... sigh... I thought we get rid of >> all this owner crap. Many years ago. How could it appear back if >> automated tools report it? >> >> Considering how many failures LKP reported for your patchsets, I have >> real doubts that anyone actually tests this code. > > Now you're commenting on the context rather than the patch. > > No, this isn't an abandoned driver, no it's not untested. Rather, it's > code I maintain, care deeply about, and have a tree that receives quite > a bit of testing (random.git) where I'll be taking these OF patches in > the case that this patchset improves (and thanks very much for your I apologize. I jumped too fast to conclusions and missed important point - acpi drivers do need to set the owner. I am sorry. (platform driver do not need, but that's a different thing) Best regards, Krzysztof
On 25/03/2024 21:51, Krzysztof Kozlowski wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On 25/03/2024 20:53, 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/vmgenid.yaml
>>
>> - Use proper FLAGS to compile with and without ACPI and/or devicetree.
>
> I do not see any flags. You use if/ifdefs which is a NAK.
>
> Do not mix independent changes within one commit. Please follow
> guidelines in submitting-patches for this.
>
By flags, I was referring to "#if IS_ENABLED(CONFIG_ACPI)". I will
remove the comment if its incorrect.
>>
>> Signed-off-by: Sudan Landge <sudanl@amazon.com>
>> ---
>> drivers/virt/Kconfig | 1 -
>> drivers/virt/vmgenid.c | 85 +++++++++++++++++++++++++++++++++++++++---
>> 2 files changed, 80 insertions(+), 6 deletions(-)
>>
>
> ...
>
>> +
>> +#if IS_ENABLED(CONFIG_OF)
>> +static irqreturn_t vmgenid_of_irq_handler(int irq, void *dev)
>> +{
>> + (void)irq;
>> + vmgenid_notify(dev);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +#endif
>>
>> static int setup_vmgenid_state(struct vmgenid_state *state, u8 *next_id)
>> {
>> @@ -55,6 +70,7 @@ static int setup_vmgenid_state(struct vmgenid_state *state, u8 *next_id)
>>
>> static int vmgenid_add_acpi(struct device *dev, struct vmgenid_state *state)
>> {
>> +#if IS_ENABLED(CONFIG_ACPI)
>
> Why do you create all this ifdeffery in the code? You already got
> comments to get rid of all this #if.
>
This was added to avoid errors if CONFIG_ACPI or CONFIG_OF was not
enabled. I will refer to other drivers again to see how they handle this.
>> struct acpi_device *device = ACPI_COMPANION(dev);
>> struct acpi_buffer parsed = { ACPI_ALLOCATE_BUFFER };
>> union acpi_object *obj;
>> @@ -96,6 +112,49 @@ static int vmgenid_add_acpi(struct device *dev, struct vmgenid_state *state)
>> out:
>> ACPI_FREE(parsed.pointer);
>> return ret;
>> +#else
>> + (void)dev;
>> + (void)state;
>> + return -EINVAL;
>> +#endif
>> +}
>> +
>> +static int vmgenid_add_of(struct platform_device *pdev, struct vmgenid_state *state)
>> +{
>> +#if IS_ENABLED(CONFIG_OF)
>> + void __iomem *remapped_ptr;
>> + int ret = 0;
>> +
>> + remapped_ptr = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
>> + if (IS_ERR(remapped_ptr)) {
>> + ret = PTR_ERR(remapped_ptr);
>> + goto out;
>> + }
>> +
>> + ret = setup_vmgenid_state(state, remapped_ptr);
>> + if (ret)
>> + goto out;
>> +
>> + state->irq = platform_get_irq(pdev, 0);
>> + if (state->irq < 0) {
>> + ret = state->irq;
>> + goto out;
>> + }
>> + 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;
>> +
>> +out:
>> + return ret;
>> +#else
>
>
> That's neither readable code nor matching Linux coding style. Driver
> don't do such magic. Please open some recent drivers for ACPI and OF and
> look there. Old drivers had stubs for !CONFIG_XXX, but new drivers don't
> have even that.
>
Sorry about that, I will refer to a new driver and correct this.
>> + (void)dev;
>> + (void)state;
>> + return -EINVAL;
>> +#endif
>> }
>>
>> static int vmgenid_add(struct platform_device *pdev)
>> @@ -108,7 +167,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);
>> @@ -116,18 +178,31 @@ static int vmgenid_add(struct platform_device *pdev)
>> return ret;
>> }
>>
>> -static const struct acpi_device_id vmgenid_ids[] = {
>> +#if IS_ENABLED(CONFIG_OF)
>
> No, drop.
>
>> +static const struct of_device_id vmgenid_of_ids[] = {
>> + { .compatible = "linux,vmgenctr", },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, vmgenid_of_ids);
>> +#endif
>> +
>> +#if IS_ENABLED(CONFIG_ACPI)
>
>
>> +static const struct acpi_device_id vmgenid_acpi_ids[] = {
>> { "VMGENCTR", 0 },
>> { "VM_GEN_COUNTER", 0 },
>> { }
>> };
>> -MODULE_DEVICE_TABLE(acpi, vmgenid_ids);
>> +MODULE_DEVICE_TABLE(acpi, vmgenid_acpi_ids);
>> +#endif
>>
>> static struct platform_driver vmgenid_plaform_driver = {
>> .probe = vmgenid_add,
>> .driver = {
>> .name = "vmgenid",
>> - .acpi_match_table = ACPI_PTR(vmgenid_ids),
>> + .acpi_match_table = ACPI_PTR(vmgenid_acpi_ids),
>> +#if IS_ENABLED(CONFIG_OF)
>
> No, really, this is some ancient code.
>
> Please point me to single driver doing such ifdef.
>
>> + .of_match_table = vmgenid_of_ids,
>> +#endif
>> .owner = THIS_MODULE,
>
> This is clearly some abandoned driver... sigh... I thought we get rid of
> all this owner crap. Many years ago. How could it appear back if
> automated tools report it?
>
> Considering how many failures LKP reported for your patchsets, I have
> real doubts that anyone actually tests this code.
>
> Please confirm:
> Did you build W=1, run smatch, sparse and coccinelle on the driver after
> your changes?
>
> Best regards,
> Krzysztof
>
I built with W=1 but wasn't aware of the other tools. I will make sure
to run all the above before submitting any future patches.
© 2016 - 2026 Red Hat, Inc.