linux-kernel - Re: [PATCH v3 3/5] driver core: platform: Add platform_put

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f6fb9ff74c8b361a592a6a4ceebd032d@kernel.org>
Date:   Thu, 26 Nov 2020 09:28:50 +0000
From:   Marc Zyngier <maz@...nel.org>
To:     John Garry <john.garry@...wei.com>
Cc:     jejb@...ux.ibm.com, martin.petersen@...cle.com, lenb@...nel.org,
        rjw@...ysocki.net, gregkh@...uxfoundation.org, tglx@...utronix.de,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        linuxarm@...wei.com, linux-acpi@...r.kernel.org
Subject: Re: [PATCH v3 3/5] driver core: platform: Add platform_put_irq()

On 2020-11-25 17:20, John Garry wrote:
> Add a function to tear down the work which was done in 
> platform_get_irq()
> for when the device driver is done with the irq.
> 
> For ACPI companion devices the irq resource is set as disabled, as this
> resource is configured from platform_get_irq()->acpi_irq_get() and 
> requires
> resetting.
> 
> Signed-off-by: John Garry <john.garry@...wei.com>
> ---
>  drivers/base/platform.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 88aef93eb4dd..3eeda3746701 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -289,6 +289,20 @@ int platform_irq_count(struct platform_device 
> *dev)
>  }
>  EXPORT_SYMBOL_GPL(platform_irq_count);
> 
> +void platform_put_irq(struct platform_device *dev, unsigned int num)
> +{
> +	unsigned int virq = platform_get_irq(dev, num);

I find it pretty odd to have to recompute the interrupt number,
which in turn results in a domain lookup. It things were refcounted
(they aren't yet), irq_dispose_mapping() would have no effect.

<pedant>
It also goes against the usual construct where if you obtain an object
based on some parameters, the release happens by specifying the object
itself, and not the parameters that lead to the object.
</pedant>

> +
> +	irq_dispose_mapping(virq);
> +	if (has_acpi_companion(&dev->dev)) {
> +		struct resource *r = platform_get_resource(dev, IORESOURCE_IRQ,
> +							   num);
> +
> +		if (r)
> +			acpi_dev_irqresource_disabled(r, 0);

It looks to me that the ACPI thing is what needs to be promoted to a
first class function, releasing all the resources that have used by
a given device.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...