Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1562085 > unrolled thread

Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

Back to article view | Back to linux.kernel

Contents

  Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping Agustin Vega-Frias <agustinv@codeaurora.org> - 2017-01-18 20:20 +0100
    Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-01-18 20:50 +0100
      Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain  mapping Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2017-01-19 13:40 +0100
        Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain  mapping Agustin Vega-Frias <agustinv@codeaurora.org> - 2017-01-19 17:30 +0100
      Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain  mapping Agustin Vega-Frias <agustinv@codeaurora.org> - 2017-01-19 16:50 +0100

#1562085 — Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

ACPI extended IRQ resources may contain a ResourceSource to specify
an alternate interrupt controller. Introduce acpi_irq_get and use it
to implement ResourceSource/IRQ domain mapping.

The new API is similar to of_irq_get and allows re-initialization
of a platform resource from the ACPI extended IRQ resource, and
provides proper behavior for probe deferral when the domain is not
yet present when called.

Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org>
---
 drivers/acpi/Makefile         |   2 +-
 drivers/acpi/{gsi.c => irq.c} | 184 ++++++++++++++++++++++++++++++++++++++++++
 drivers/base/platform.c       |   9 ++-
 include/linux/acpi.h          |  10 +++
 4 files changed, 203 insertions(+), 2 deletions(-)
 rename drivers/acpi/{gsi.c => irq.c} (32%)

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 9ed0878..a391bbc 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -55,7 +55,7 @@ acpi-$(CONFIG_DEBUG_FS)		+= debugfs.o
 acpi-$(CONFIG_ACPI_NUMA)	+= numa.o
 acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
 acpi-y				+= acpi_lpat.o
-acpi-$(CONFIG_ACPI_GENERIC_GSI) += gsi.o
+acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o
 acpi-$(CONFIG_ACPI_WATCHDOG)	+= acpi_watchdog.o

 # These are (potentially) separate modules
diff --git a/drivers/acpi/gsi.c b/drivers/acpi/irq.c
similarity index 32%
rename from drivers/acpi/gsi.c
rename to drivers/acpi/irq.c
index ee9e0f2..cadc4cdd 100644
--- a/drivers/acpi/gsi.c
+++ b/drivers/acpi/irq.c
@@ -85,6 +85,190 @@ void acpi_unregister_gsi(u32 gsi)
 EXPORT_SYMBOL_GPL(acpi_unregister_gsi);

 /**
+ * acpi_get_irq_source_fwhandle() - Retrieve the fwhandle of the given
+ *                                  acpi_resource_source which is used
+ *                                  as an IRQ domain id
+ * @source: acpi_resource_source to use for the lookup
+ *
+ * Returns: The appropriate IRQ fwhandle domain id
+ *          NULL on failure
+ */
+static struct fwnode_handle *
+acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source)
+{
+	struct fwnode_handle *result;
+	struct acpi_device *device;
+	acpi_handle handle;
+	acpi_status status;
+
+	if (!source->string_length)
+		return acpi_gsi_domain_id;
+
+	status = acpi_get_handle(NULL, source->string_ptr, &handle);
+	if (ACPI_FAILURE(status)) {
+		pr_warn("Could not find handle for %s\n", source->string_ptr);
+		return NULL;
+	}
+
+	device = acpi_bus_get_acpi_device(handle);
+	if (!device) {
+		pr_warn("Could not get device for %s\n", source->string_ptr);
+		return NULL;
+	}
+
+	result = &device->fwnode;
+	acpi_bus_put_acpi_device(device);
+
+	return result;
+}
+
+/**
+ * Context for the resource walk used to lookup IRQ resources.
+ */
+struct acpi_irq_parse_one_ctx {
+	int rc;
+	unsigned int index;
+	unsigned long *res_flags;
+	struct irq_fwspec *fwspec;
+};
+
+/**
+ * acpi_irq_parse_one_match - Handle a matching IRQ resource
+ */
+static inline void acpi_irq_parse_one_match(struct fwnode_handle *fwnode,
+					    u32 hwirq, u8 triggering,
+					    u8 polarity, u8 shareable,
+					    struct acpi_irq_parse_one_ctx *ctx)
+{
+	ctx->rc = 0;
+	*ctx->res_flags = acpi_dev_irq_flags(triggering, polarity, shareable);
+	ctx->fwspec->fwnode = fwnode;
+	ctx->fwspec->param[0] = hwirq;
+	ctx->fwspec->param[1] = acpi_dev_get_irq_type(triggering, polarity);
+	ctx->fwspec->param_count = 2;
+}
+
+/**
+ * acpi_irq_parse_one_cb - Handle the given resource
+ * @ares: resource to handle
+ * @context: context for the walk, contains the lookup index and references
+ *           to the flags and fwspec where the result is returned
+ *
+ * This is called by acpi_walk_resources passing each resource returned by
+ * the _CRS method. We only inspect IRQ resources. Since IRQ resources
+ * might contain multiple interrupts we check if the index is within this
+ * one's interrupt array, otherwise we subtract the current resource IRQ
+ * count from the lookup index to prepare for the next resource.
+ * Once a match is found we call acpi_irq_parse_one_match to populate
+ * the result and end the walk by returning AE_CTRL_TERMINATE.
+ *
+ * Return AE_OK if the walk should continue, AE_CTRL_TERMINATE if a matching
+ * IRQ resource was found.
+ */
+static acpi_status acpi_irq_parse_one_cb(struct acpi_resource *ares,
+					 void *context)
+{
+	struct acpi_irq_parse_one_ctx *ctx = context;
+	struct acpi_resource_irq *irq;
+	struct acpi_resource_extended_irq *eirq;
+	struct fwnode_handle *fwnode;
+
+	switch (ares->type) {
+	case ACPI_RESOURCE_TYPE_IRQ:
+		irq = &ares->data.irq;
+		if (ctx->index >= irq->interrupt_count) {
+			ctx->index -= irq->interrupt_count;
+			return AE_OK;
+		}
+		fwnode = acpi_gsi_domain_id;
+		acpi_irq_parse_one_match(fwnode, irq->interrupts[ctx->index],
+					 irq->triggering, irq->polarity,
+					 irq->sharable, ctx);
+		return AE_CTRL_TERMINATE;
+	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
+		eirq = &ares->data.extended_irq;
+		if (eirq->producer_consumer == ACPI_PRODUCER)
+			return AE_OK;
+		if (ctx->index >= eirq->interrupt_count) {
+			ctx->index -= eirq->interrupt_count;
+			return AE_OK;
+		}
+		fwnode = acpi_get_irq_source_fwhandle(&eirq->resource_source);
+		acpi_irq_parse_one_match(fwnode, eirq->interrupts[ctx->index],
+					 eirq->triggering, eirq->polarity,
+					 eirq->sharable, ctx);
+		return AE_CTRL_TERMINATE;
+	}
+
+	return AE_OK;
+}
+
+/**
+ * acpi_irq_parse_one - Resolve an interrupt for a device
+ * @handle: the device whose interrupt is to be resolved
+ * @index: index of the interrupt to resolve
+ * @fwspec: structure irq_fwspec filled by this function
+ * @flags: resource flags filled by this function
+ *
+ * This function resolves an interrupt for a device by walking its CRS resources
+ * to find the appropriate ACPI IRQ resource and populating the given structure
+ * which can be used to retrieve a Linux IRQ number.
+ *
+ * Returns the result stored in ctx.rc by the callback, or -EINVAL if the given
+ * index is out of range.
+ */
+static int acpi_irq_parse_one(acpi_handle handle, unsigned int index,
+			      struct irq_fwspec *fwspec, unsigned long *flags)
+{
+	struct acpi_irq_parse_one_ctx ctx = { -EINVAL, index, flags, fwspec };
+	acpi_status status;
+
+	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
+				     acpi_irq_parse_one_cb, &ctx);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+	return ctx.rc;
+}
+
+/**
+ * acpi_irq_get - Look for the ACPI IRQ resource with the given index and
+ *                use it to initialize the given Linux IRQ resource.
+ * @handle ACPI device handle
+ * @index  ACPI IRQ resource index to lookup
+ * @res    Linux IRQ resource to initialize
+ *
+ * Return: 0 on success
+ *         -EINVAL if an error occurs
+ *         -EPROBE_DEFER if the IRQ lookup/conversion failed
+ */
+int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res)
+{
+	int rc;
+	struct irq_fwspec fwspec;
+	struct irq_domain *domain;
+	unsigned long flags;
+
+	rc = acpi_irq_parse_one(handle, index, &fwspec, &flags);
+	if (rc)
+		return rc;
+
+	domain = irq_find_matching_fwnode(fwspec.fwnode, DOMAIN_BUS_ANY);
+	if (!domain)
+		return -EPROBE_DEFER;
+
+	rc = irq_create_fwspec_mapping(&fwspec);
+	if (rc <= 0)
+		return -EINVAL;
+
+	res->start = rc;
+	res->end = rc;
+	res->flags = flags;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_irq_get);
+
+/**
  * acpi_set_irq_model - Setup the GSI irqdomain information
  * @model: the value assigned to acpi_irq_model
  * @fwnode: the irq_domain identifier for mapping and looking up
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index c4af003..61423d2 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -102,6 +102,14 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
 	}

 	r = platform_get_resource(dev, IORESOURCE_IRQ, num);
+	if (r && r->flags & IORESOURCE_DISABLED && ACPI_COMPANION(&dev->dev)) {
+		int ret;
+
+		ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
+		if (ret)
+			return ret;
+	}
+
 	/*
 	 * The resources may pass trigger flags to the irqs that need
 	 * to be set up. It so happens that the trigger flags for
@@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void)
 		memset(&pd->dev.devres_head, 0, sizeof(pd->dev.devres_head));
 	}
 }
-
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 5b36974..03a94cd 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1153,4 +1153,14 @@ static inline void acpi_table_upgrade(void) { }
 static inline int parse_spcr(bool earlycon) { return 0; }
 #endif

+#ifdef CONFIG_ACPI_GENERIC_GSI
+int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res);
+#else
+static inline int acpi_irq_get(acpi_handle handle, unsigned int index,
+			       struct resource *res)
+{
+	return -EINVAL;
+}
+#endif
+
 #endif	/*_LINUX_ACPI_H*/
--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [next] | [standalone]

#1562119

On Wed, Jan 18, 2017 at 9:04 PM, Agustin Vega-Frias
<agustinv@codeaurora.org> wrote:
> ACPI extended IRQ resources may contain a ResourceSource to specify
> an alternate interrupt controller. Introduce acpi_irq_get and use it
> to implement ResourceSource/IRQ domain mapping.
>
> The new API is similar to of_irq_get and allows re-initialization
> of a platform resource from the ACPI extended IRQ resource, and
> provides proper behavior for probe deferral when the domain is not
> yet present when called.

Thanks, looks better now.

>  /**
> + * acpi_get_irq_source_fwhandle() - Retrieve the fwhandle of the given
> + *                                  acpi_resource_source which is used
> + *                                  as an IRQ domain id

Please, make short description here and put long one below

> + * @source: acpi_resource_source to use for the lookup
> + *

* Description:
* ...
*

> + * Returns: The appropriate IRQ fwhandle domain id
> + *          NULL on failure

* Return:
* blablabla

> + */
> +static struct fwnode_handle *
> +acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source)
> +{
> +       struct fwnode_handle *result;
> +       struct acpi_device *device;
> +       acpi_handle handle;
> +       acpi_status status;
> +
> +       if (!source->string_length)
> +               return acpi_gsi_domain_id;
> +
> +       status = acpi_get_handle(NULL, source->string_ptr, &handle);
> +       if (ACPI_FAILURE(status)) {

> +               pr_warn("Could not find handle for %s\n", source->string_ptr);
> +               return NULL;
> +       }
> +
> +       device = acpi_bus_get_acpi_device(handle);
> +       if (!device) {
> +               pr_warn("Could not get device for %s\n", source->string_ptr);

I'm not sure both messages have a value.

> +               return NULL;
> +       }

> +/**

You mark as kernel doc, but in fact it's just a comment.

> + * Context for the resource walk used to lookup IRQ resources.
> + */
> +struct acpi_irq_parse_one_ctx {
> +       int rc;
> +       unsigned int index;
> +       unsigned long *res_flags;
> +       struct irq_fwspec *fwspec;
> +};
> +
> +/**
> + * acpi_irq_parse_one_match - Handle a matching IRQ resource

This looks better, but lacks of parameter descriptions.

> + */

> +/**
> + * acpi_irq_parse_one_cb - Handle the given resource
> + * @ares: resource to handle

> + * @context: context for the walk, contains the lookup index and references
> + *           to the flags and fwspec where the result is returned

Make it shorter, ideally into one line

> + *

* Description:

> + * This is called by acpi_walk_resources passing each resource returned by
> + * the _CRS method. We only inspect IRQ resources. Since IRQ resources
> + * might contain multiple interrupts we check if the index is within this
> + * one's interrupt array, otherwise we subtract the current resource IRQ
> + * count from the lookup index to prepare for the next resource.
> + * Once a match is found we call acpi_irq_parse_one_match to populate
> + * the result and end the walk by returning AE_CTRL_TERMINATE.
> + *
> + * Return AE_OK if the walk should continue, AE_CTRL_TERMINATE if a matching

* Return:
* blablabla

> + * IRQ resource was found.
> + */

> +/**
> + * acpi_irq_parse_one - Resolve an interrupt for a device
> + * @handle: the device whose interrupt is to be resolved
> + * @index: index of the interrupt to resolve
> + * @fwspec: structure irq_fwspec filled by this function
> + * @flags: resource flags filled by this function
> + *
> + * This function resolves an interrupt for a device by walking its CRS resources
> + * to find the appropriate ACPI IRQ resource and populating the given structure
> + * which can be used to retrieve a Linux IRQ number.
> + *
> + * Returns the result stored in ctx.rc by the callback, or -EINVAL if the given
> + * index is out of range.

Ditto for Description and Return.

> + */
> +static int acpi_irq_parse_one(acpi_handle handle, unsigned int index,
> +                             struct irq_fwspec *fwspec, unsigned long *flags)
> +{
> +       struct acpi_irq_parse_one_ctx ctx = { -EINVAL, index, flags, fwspec };
> +       acpi_status status;
> +
> +       status = acpi_walk_resources(handle, METHOD_NAME__CRS,
> +                                    acpi_irq_parse_one_cb, &ctx);

> +       if (ACPI_FAILURE(status))
> +               return -EINVAL;

Shouldn't you have the same in rc? Would be redundant.

> +       return ctx.rc;
> +}
> +
> +/**
> + * acpi_irq_get - Look for the ACPI IRQ resource with the given index and
> + *                use it to initialize the given Linux IRQ resource.
> + * @handle ACPI device handle
> + * @index  ACPI IRQ resource index to lookup
> + * @res    Linux IRQ resource to initialize

Ah, you missed colons after field names:
* @field1:

> + *
> + * Return:

Next line:  0 on success

> + *         -EINVAL if an error occurs
> + *         -EPROBE_DEFER if the IRQ lookup/conversion failed
> + */
> +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res)
> +{

> +       int rc;

Put this last in the definition block.

> +       struct irq_fwspec fwspec;
> +       struct irq_domain *domain;
> +       unsigned long flags;
> +
> +       rc = acpi_irq_parse_one(handle, index, &fwspec, &flags);
> +       if (rc)
> +               return rc;
> +
> +       domain = irq_find_matching_fwnode(fwspec.fwnode, DOMAIN_BUS_ANY);
> +       if (!domain)
> +               return -EPROBE_DEFER;

Hmm... Could it be other issues here?

> +
> +       rc = irq_create_fwspec_mapping(&fwspec);
> +       if (rc <= 0)
> +               return -EINVAL;
> +

> +       res->start = rc;
> +       res->end = rc;
> +       res->flags = flags;

Perhaps struct resource *r should be a parameter to acpi_irq_parse_one().

> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(acpi_irq_get);
> +
> +/**
>   * acpi_set_irq_model - Setup the GSI irqdomain information
>   * @model: the value assigned to acpi_irq_model
>   * @fwnode: the irq_domain identifier for mapping and looking up
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index c4af003..61423d2 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -102,6 +102,14 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
>         }
>
>         r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> +       if (r && r->flags & IORESOURCE_DISABLED && ACPI_COMPANION(&dev->dev)) {

has_acpi_companion() ?

> +               int ret;
> +
> +               ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> +               if (ret)
> +                       return ret;
> +       }

> @@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void)
>                 memset(&pd->dev.devres_head, 0, sizeof(pd->dev.devres_head));
>         }
>  }
> -

It doesn't belong here.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1562686 — Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

On Wed, Jan 18, 2017 at 09:45:56PM +0200, Andy Shevchenko wrote:

[...]

> > +/**
> > + * acpi_irq_get - Look for the ACPI IRQ resource with the given index and
> > + *                use it to initialize the given Linux IRQ resource.
> > + * @handle ACPI device handle
> > + * @index  ACPI IRQ resource index to lookup
> > + * @res    Linux IRQ resource to initialize
> 
> Ah, you missed colons after field names:
> * @field1:
> 
> > + *
> > + * Return:
> 
> Next line:  0 on success
> 
> > + *         -EINVAL if an error occurs
> > + *         -EPROBE_DEFER if the IRQ lookup/conversion failed
> > + */
> > +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res)
> > +{
> 
> > +       int rc;
> 
> Put this last in the definition block.
> 
> > +       struct irq_fwspec fwspec;
> > +       struct irq_domain *domain;
> > +       unsigned long flags;
> > +
> > +       rc = acpi_irq_parse_one(handle, index, &fwspec, &flags);
> > +       if (rc)
> > +               return rc;
> > +
> > +       domain = irq_find_matching_fwnode(fwspec.fwnode, DOMAIN_BUS_ANY);
> > +       if (!domain)
> > +               return -EPROBE_DEFER;
> 
> Hmm... Could it be other issues here?

That's a good point. Here probing should be deferred only if we know a
driver capable of handling fwspec.fwnode will have a chance to be
probed/initialized eventually right (which basically means it is
compiled in the kernel and we hope it will probed successfully) ? I am
not sure there is an easy way to detect that in ACPI at the moment, I
suspect it is time we added a linker section (or augment the existing
one used for irqchip early MADT parsing - IRQCHIP_ACPI_DECLARE) for this
purpose I do not see any other option (apart from leaving devices in the
deferred probe list if a driver for the irqchip represented by
fwspec.fwnode is not present in the kernel, which is a bit sloppy, or
resorting to checking Kconfig entries to detect compile time enabled
irqchip drivers).

> > +
> > +       rc = irq_create_fwspec_mapping(&fwspec);
> > +       if (rc <= 0)
> > +               return -EINVAL;
> > +
> 
> > +       res->start = rc;
> > +       res->end = rc;
> > +       res->flags = flags;
> 
> Perhaps struct resource *r should be a parameter to acpi_irq_parse_one().

Yeah but then you would end up with flags initialized in
acpi_irq_parse_one() and the other resource params (ie start, end) here,
I do not think it is nicer.

Thanks,
Lorenzo

> 
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_irq_get);
> > +
> > +/**
> >   * acpi_set_irq_model - Setup the GSI irqdomain information
> >   * @model: the value assigned to acpi_irq_model
> >   * @fwnode: the irq_domain identifier for mapping and looking up
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index c4af003..61423d2 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -102,6 +102,14 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
> >         }
> >
> >         r = platform_get_resource(dev, IORESOURCE_IRQ, num);
> > +       if (r && r->flags & IORESOURCE_DISABLED && ACPI_COMPANION(&dev->dev)) {
> 
> has_acpi_companion() ?
> 
> > +               int ret;
> > +
> > +               ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
> > +               if (ret)
> > +                       return ret;
> > +       }
> 
> > @@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void)
> >                 memset(&pd->dev.devres_head, 0, sizeof(pd->dev.devres_head));
> >         }
> >  }
> > -
> 
> It doesn't belong here.
> 
> -- 
> With Best Regards,
> Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1562876 — Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

On 2017-01-19 07:36, Lorenzo Pieralisi wrote:
> On Wed, Jan 18, 2017 at 09:45:56PM +0200, Andy Shevchenko wrote:
> 
> [...]
> 
>> > +/**
>> > + * acpi_irq_get - Look for the ACPI IRQ resource with the given index and
>> > + *                use it to initialize the given Linux IRQ resource.
>> > + * @handle ACPI device handle
>> > + * @index  ACPI IRQ resource index to lookup
>> > + * @res    Linux IRQ resource to initialize
>> 
>> Ah, you missed colons after field names:
>> * @field1:
>> 
>> > + *
>> > + * Return:
>> 
>> Next line:  0 on success
>> 
>> > + *         -EINVAL if an error occurs
>> > + *         -EPROBE_DEFER if the IRQ lookup/conversion failed
>> > + */
>> > +int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res)
>> > +{
>> 
>> > +       int rc;
>> 
>> Put this last in the definition block.
>> 
>> > +       struct irq_fwspec fwspec;
>> > +       struct irq_domain *domain;
>> > +       unsigned long flags;
>> > +
>> > +       rc = acpi_irq_parse_one(handle, index, &fwspec, &flags);
>> > +       if (rc)
>> > +               return rc;
>> > +
>> > +       domain = irq_find_matching_fwnode(fwspec.fwnode, DOMAIN_BUS_ANY);
>> > +       if (!domain)
>> > +               return -EPROBE_DEFER;
>> 
>> Hmm... Could it be other issues here?
> 
> That's a good point. Here probing should be deferred only if we know a
> driver capable of handling fwspec.fwnode will have a chance to be
> probed/initialized eventually right (which basically means it is
> compiled in the kernel and we hope it will probed successfully) ? I am
> not sure there is an easy way to detect that in ACPI at the moment, I
> suspect it is time we added a linker section (or augment the existing
> one used for irqchip early MADT parsing - IRQCHIP_ACPI_DECLARE) for 
> this
> purpose I do not see any other option (apart from leaving devices in 
> the
> deferred probe list if a driver for the irqchip represented by
> fwspec.fwnode is not present in the kernel, which is a bit sloppy, or
> resorting to checking Kconfig entries to detect compile time enabled
> irqchip drivers).

I'll add the linker section suggested by Lorenzo for the extra check.
We can do that check in acpi_get_irq_source_fwhandle and make the lookup
fail if the device is not listed in the table.

That way we can keep this check as-is.

Thoughts?

> 
>> > +
>> > +       rc = irq_create_fwspec_mapping(&fwspec);
>> > +       if (rc <= 0)
>> > +               return -EINVAL;
>> > +
>> 
>> > +       res->start = rc;
>> > +       res->end = rc;
>> > +       res->flags = flags;
>> 
>> Perhaps struct resource *r should be a parameter to 
>> acpi_irq_parse_one().
> 
> Yeah but then you would end up with flags initialized in
> acpi_irq_parse_one() and the other resource params (ie start, end) 
> here,
> I do not think it is nicer.

The reason this took this form is that we are trying to get some 
symmetry
with what of_irq_get does.

Thanks,
Agustin

> 
> Thanks,
> Lorenzo
> 
> [...]
> 
>> 
>> With Best Regards,
>> Andy Shevchenko

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1562839 — Re: [PATCH V10 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping

Hi Andy,

On 2017-01-18 14:45, Andy Shevchenko wrote:
> On Wed, Jan 18, 2017 at 9:04 PM, Agustin Vega-Frias
> <agustinv@codeaurora.org> wrote:
> 
> [...]
> 
> 
>> + */
>> +static struct fwnode_handle *
>> +acpi_get_irq_source_fwhandle(const struct acpi_resource_source 
>> *source)
>> +{
>> +       struct fwnode_handle *result;
>> +       struct acpi_device *device;
>> +       acpi_handle handle;
>> +       acpi_status status;
>> +
>> +       if (!source->string_length)
>> +               return acpi_gsi_domain_id;
>> +
>> +       status = acpi_get_handle(NULL, source->string_ptr, &handle);
>> +       if (ACPI_FAILURE(status)) {
> 
>> +               pr_warn("Could not find handle for %s\n", 
>> source->string_ptr);
>> +               return NULL;
>> +       }
>> +
>> +       device = acpi_bus_get_acpi_device(handle);
>> +       if (!device) {
>> +               pr_warn("Could not get device for %s\n", 
>> source->string_ptr);
> 
> I'm not sure both messages have a value.

I'll probably just drop the explicit pr_warns and just use WARN_ON on
the if condition. Is that a reasonable alternative in your view?

> 
>> +               return NULL;
>> +       }
> 
> 
> [...]
> 
>> + */
>> +static int acpi_irq_parse_one(acpi_handle handle, unsigned int index,
>> +                             struct irq_fwspec *fwspec, unsigned long 
>> *flags)
>> +{
>> +       struct acpi_irq_parse_one_ctx ctx = { -EINVAL, index, flags, 
>> fwspec };
>> +       acpi_status status;
>> +
>> +       status = acpi_walk_resources(handle, METHOD_NAME__CRS,
>> +                                    acpi_irq_parse_one_cb, &ctx);
> 
>> +       if (ACPI_FAILURE(status))
>> +               return -EINVAL;
> 
> Shouldn't you have the same in rc? Would be redundant.
> 

Good point, since we always return -EINVAL on failure we can skip
the check since rc will only be updated on success.

>> +       return ctx.rc;
>> +}
>> +
> 
> [...]
> 
>> +       domain = irq_find_matching_fwnode(fwspec.fwnode, 
>> DOMAIN_BUS_ANY);
>> +       if (!domain)
>> +               return -EPROBE_DEFER;
> 
> Hmm... Could it be other issues here?

I'll comment on this on Lorenzo's reply.

> 
>> +
>> +       rc = irq_create_fwspec_mapping(&fwspec);
>> +       if (rc <= 0)
>> +               return -EINVAL;
>> +
> 
>> +       res->start = rc;
>> +       res->end = rc;
>> +       res->flags = flags;
> 
> Perhaps struct resource *r should be a parameter to 
> acpi_irq_parse_one().

I'll comment on this on Lorenzo's reply.

> 
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(acpi_irq_get);
>> +
>> +/**
>>   * acpi_set_irq_model - Setup the GSI irqdomain information
>>   * @model: the value assigned to acpi_irq_model
>>   * @fwnode: the irq_domain identifier for mapping and looking up

I'll address the other issues on V11.

Thanks for your thorough review,
Agustin

> 
> --
> With Best Regards,
> Andy Shevchenko

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web