| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7932459d-4dae-4a29-b702-5103d853dbdd@amazon.co.uk>
Date: Tue, 26 Mar 2024 14:01:25 +0000
From: "Landge, Sudan" <sudanl@...zon.co.uk>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, Sudan Landge
<sudanl@...zon.com>, <tytso@....edu>, <Jason@...c4.com>,
<robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
<conor+dt@...nel.org>, <sathyanarayanan.kuppuswamy@...ux.intel.com>,
<thomas.lendacky@....com>, <dan.j.williams@...el.com>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
CC: <graf@...zon.de>, <dwmw@...zon.co.uk>, <bchalios@...zon.es>,
<xmarcalx@...zon.co.uk>
Subject: Re: [PATCH v3 4/4] virt: vmgenid: add support for devicetree bindings
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@...zon.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.
Powered by blists - more mailing lists