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@xxxxxxxxxx>
---
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.
