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

[Landge, Sudan]

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.