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

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Sudan Landge <sudanl@amazon.com>,
	tytso@mit.edu, Jason@zx2c4.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	sathyanarayanan.kuppuswamy@linux.intel.com,
	thomas.lendacky@amd.com, dan.j.williams@intel.com,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: graf@amazon.de, dwmw@amazon.co.uk, bchalios@amazon.es,
	xmarcalx@amazon.co.uk
Subject: Re: [PATCH v3 4/4] virt: vmgenid: add support for devicetree bindings
Date: Mon, 25 Mar 2024 22:51:25 +0100	[thread overview]
Message-ID: <5ca78fc8-4a53-4f09-878f-4a47875f9de5@linaro.org> (raw)
In-Reply-To: <20240325195306.13133-5-sudanl@amazon.com>

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