| 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: <20231213220437.GA2115075-robh@kernel.org>
Date: Wed, 13 Dec 2023 16:04:37 -0600
From: Rob Herring <robh@...nel.org>
To: Mark Hasemeyer <markhas@...omium.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
Raul Rangel <rrangel@...omium.org>,
David Gow <davidgow@...gle.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Mark Brown <broonie@...nel.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Takashi Iwai <tiwai@...e.de>,
Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
Subject: Re: [PATCH v1 5/6] platform: Modify platform_get_irq_optional() to
use resource
On Wed, Dec 13, 2023 at 11:00:23AM -0700, Mark Hasemeyer wrote:
> Unify handling of ACPI, GPIO, devictree, and platform resource
> interrupts in platform_get_irq_optional(). Each of these subsystems
> provide their own apis which provide IRQ information as a struct
> resource. This simplifies the logic of the function and allows callers
> to get more information about the irq by looking at the resource flags.
> For example, whether or not an irq is wake capable.
>
> Rename the function to platform_get_irq_resource() to better describe
> the function's new behavior.
This is misleading as the original function is still there.
The get_optional() functions are designed to not print an error message
where as the non-optional variant will. You've broken that pattern here
in that there is no platform_get_irq_resource_optional() (at least named
that because your implementation is that since there is no error
message).
What about versions equivalent to platform_get_irq_byname()
and platform_get_irq_byname_optional(), though I guess we need users
first.
>
> Signed-off-by: Mark Hasemeyer <markhas@...omium.org>
> ---
>
> drivers/base/platform.c | 78 ++++++++++++++++++---------------
> include/linux/platform_device.h | 9 +++-
> 2 files changed, 50 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 76bfcba250039..6b58bde776d4f 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -151,9 +151,10 @@ EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
> #endif /* CONFIG_HAS_IOMEM */
>
> /**
> - * platform_get_irq_optional - get an optional IRQ for a device
> + * platform_get_irq_resource - get an IRQ for a device and populate resource struct
> * @dev: platform device
> * @num: IRQ number index
> + * @r: pointer to resource to populate with irq information. It is not modified on failure.
> *
> * Gets an IRQ for a platform device. Device drivers should check the return
> * value for errors so as to not pass a negative integer value to the
> @@ -162,59 +163,47 @@ EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
> *
> * For example::
> *
> - * int irq = platform_get_irq_optional(pdev, 0);
> + * int irq = platform_get_irq_resource(pdev, 0, &res);
> * if (irq < 0)
> * return irq;
> *
> * Return: non-zero IRQ number on success, negative error number on failure.
> */
> -int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
> +int platform_get_irq_resource(struct platform_device *dev, unsigned int num, struct resource *r)
> {
> int ret;
> #ifdef CONFIG_SPARC
> /* sparc does not have irqs represented as IORESOURCE_IRQ resources */
> if (!dev || num >= dev->archdata.num_irqs)
> - goto out_not_found;
> + return -ENXIO;
> ret = dev->archdata.irqs[num];
> + if (ret >= 0)
> + *r = (struct resource)DEFINE_RES_IRQ(ret);
> goto out;
> #else
> - struct resource *r;
> + struct resource *platform_res;
>
> if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node) {
> - ret = of_irq_get(dev->dev.of_node, num);
> + ret = of_irq_to_resource(dev->dev.of_node, num, r);
> if (ret > 0 || ret == -EPROBE_DEFER)
> goto out;
> }
>
> - r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> - if (has_acpi_companion(&dev->dev)) {
> - if (r && r->flags & IORESOURCE_DISABLED) {
> - ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> - if (ret)
> - goto out;
> - }
> - }
> -
> - /*
> - * The resources may pass trigger flags to the irqs that need
> - * to be set up. It so happens that the trigger flags for
> - * IORESOURCE_BITS correspond 1-to-1 to the IRQF_TRIGGER*
> - * settings.
> - */
> - if (r && r->flags & IORESOURCE_BITS) {
> - struct irq_data *irqd;
> -
> - irqd = irq_get_irq_data(r->start);
> - if (!irqd)
> - goto out_not_found;
> - irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
> - }
> -
> - if (r) {
> + platform_res = platform_get_resource(dev, IORESOURCE_IRQ, num);
> + if (platform_res && !(platform_res->flags & IORESOURCE_DISABLED)) {
> + *r = *platform_res;
> ret = r->start;
> goto out;
> }
>
> + if (has_acpi_companion(&dev->dev)) {
> + ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> + if (!ret || ret == -EPROBE_DEFER) {
> + ret = ret ?: r->start;
> + goto out;
> + }
> + }
> +
> /*
> * For the index 0 interrupt, allow falling back to GpioInt
> * resources. While a device could have both Interrupt and GpioInt
> @@ -223,21 +212,38 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
> * allows a common code path across either kind of resource.
> */
> if (num == 0 && has_acpi_companion(&dev->dev)) {
> - ret = acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
> + ret = acpi_dev_get_gpio_irq_resource(ACPI_COMPANION(&dev->dev), NULL,
> + num, r);
> /* Our callers expect -ENXIO for missing IRQs. */
> - if (ret >= 0 || ret == -EPROBE_DEFER)
> + if (!ret || ret == -EPROBE_DEFER) {
> + ret = ret ?: r->start;
> goto out;
> + }
> }
> -
> #endif
> -out_not_found:
> ret = -ENXIO;
> out:
> if (WARN(!ret, "0 is an invalid IRQ number\n"))
> return -EINVAL;
> +
> + /*
> + * The resources may pass trigger flags to the irqs that need
> + * to be set up. It so happens that the trigger flags for
> + * IORESOURCE_BITS correspond 1-to-1 to the IRQF_TRIGGER*
> + * settings.
> + */
> + if (ret > 0 && r->flags & IORESOURCE_BITS) {
> + struct irq_data *irqd;
> +
> + irqd = irq_get_irq_data(r->start);
> + if (!irqd)
> + ret = -ENXIO;
> + else
> + irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
We were not doing any of this in the DT or Sparc cases before. It's
probably just redundant for DT. It might break Sparc.
Rob
Powered by blists - more mailing lists