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 <[email protected]>
> ---
> 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 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 <[email protected]>
>> ---
>> 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.
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