Re: [PATCH v3 4/4] virt: vmgenid: add support for devicetree bindings

["Landge, Sudan" <sudanl@amazon.co.uk>]

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.