On Tuesday, November 13, 2012 04:16:42 PM Mika Westerberg wrote:
> On Tue, Nov 13, 2012 at 01:06:59PM +0100, Rafael J. Wysocki wrote:
> > > I don't know any better option.
> >
> > Well, a better option would be to convince ACPICA to provide us different
> > interfaces. :-)
>
> Heh, indeed.
>
> > We might actually try to do that in the future, but for now we still need
> > _CRS to be evaluated centrally rather than by every interested party in
> > isolation. So below is a replacement for my original [3/3] patch that
> > approaches the problem from a different angle (on top of [1/3] and
> > updated [2/3] sent yesterday). Please let me know what you think.
> >
> > Thanks,
> > Rafael
> >
> >
> > ---
> > From: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> > Subject: ACPI: Centralized processing of ACPI device resources
> >
> > Currently, whoever wants to use ACPI device resources has to call
> > acpi_walk_resources() to browse the buffer returned by the _CRS
> > method for the given device and create filters passed to that
> > routine to apply to the individual resource items. This generally
> > is cumbersome, time-consuming and inefficient. Moreover, it may
> > be problematic if resource conflicts need to be resolved, because
> > the different users of _CRS will need to do that in a consistent
> > way. However, if there are resource conflicts, the ACPI core
> > should be able to resolve them centrally instead of relying on
> > various users of acpi_walk_resources() to handle them correctly
> > together.
> >
> > For this reason, introduce a new function, acpi_dev_get_resources(),
> > that can be used by subsystems to obtain a list of struct resource
> > objects corresponding to the ACPI device resources returned by
> > _CRS and, if necessary, to apply additional preprocessing routine
> > to the ACPI resources before converting them to the struct resource
> > format.
> >
> > Make the ACPI code that creates platform device objects use
> > acpi_dev_get_resources() for resource processing instead of executing
> > acpi_walk_resources() twice by itself, which causes it to be much
> > more straightforward and easier to follow.
> >
> > In the future, acpi_dev_get_resources() can be extended to meet
> > the needs of the ACPI PNP subsystem and other users of _CRS in
> > the kernel.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
>
> I found one problem which resulted failure (see below) but once that was
> fixed devices were enumerated correctly. I tested this also with modified
> SPI/I2C/GPIO patches and they also work as expected on my test machine.
>
> Nice work, thanks.
Well, thanks! :-)
> > ---
> > drivers/acpi/acpi_platform.c | 94 +++++-------------------------
> > drivers/acpi/resource.c | 132
> > +++++++++++++++++++++++++++++++++++++++++++
> > include/linux/acpi.h | 10 +++
> > 3 files changed, 159 insertions(+), 77 deletions(-)
> >
> > Index: linux/include/linux/acpi.h
> > ===================================================================
> > --- linux.orig/include/linux/acpi.h
> > +++ linux/include/linux/acpi.h
> > @@ -261,6 +261,16 @@ unsigned long acpi_dev_irq_flags(u8 trig
> > bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
> > struct resource *res);
> >
> > +struct resource_list_entry {
> > + struct list_head node;
> > + struct resource res;
> > +};
> > +
> > +void acpi_dev_free_resource_list(struct list_head *list);
> > +int acpi_dev_get_resources(struct acpi_device *adev, struct list_head
> > *list,
> > + int (*preproc)(struct acpi_resource *, void *),
> > + void *preproc_data);
> > +
> > int acpi_check_resource_conflict(const struct resource *res);
> >
> > int acpi_check_region(resource_size_t start, resource_size_t n,
> > Index: linux/drivers/acpi/resource.c
> > ===================================================================
> > --- linux.orig/drivers/acpi/resource.c
> > +++ linux/drivers/acpi/resource.c
> > @@ -26,6 +26,7 @@
> > #include <linux/device.h>
> > #include <linux/export.h>
> > #include <linux/ioport.h>
> > +#include <linux/slab.h>
> >
> > #ifdef CONFIG_X86
> > #define valid_IRQ(i) (((i) != 0) && ((i) != 2))
> > @@ -391,3 +392,134 @@ bool acpi_dev_resource_interrupt(struct
> > return true;
> > }
> > EXPORT_SYMBOL_GPL(acpi_dev_resource_interrupt);
> > +
> > +/**
> > + * acpi_dev_free_resource_list - Free resource from
> > %acpi_dev_get_resources().
> > + * @list: The head of the resource list to free.
> > + */
> > +void acpi_dev_free_resource_list(struct list_head *list)
> > +{
> > + struct resource_list_entry *rentry, *re;
> > +
> > + list_for_each_entry_safe(rentry, re, list, node) {
> > + list_del(&rentry->node);
> > + kfree(rentry);
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_dev_free_resource_list);
> > +
> > +struct res_proc_context {
> > + struct list_head *list;
> > + int (*preproc)(struct acpi_resource *, void *);
> > + void *preproc_data;
> > + int count;
> > + int error;
> > +};
> > +
> > +static acpi_status acpi_dev_new_resource_entry(struct resource *r,
> > + struct res_proc_context *c)
> > +{
> > + struct resource_list_entry *rentry;
> > +
> > + rentry = kmalloc(sizeof(*rentry), GFP_KERNEL);
> > + if (!rentry) {
> > + c->error = -ENOMEM;
> > + return AE_NO_MEMORY;
> > + }
> > + INIT_LIST_HEAD(&rentry->node);
> > + rentry->res = *r;
> > + list_add_tail(&rentry->node, c->list);
> > + c->count++;
> > + return AE_OK;
> > +}
> > +
> > +static acpi_status acpi_dev_process_resource(struct acpi_resource *ares,
> > + void *context)
> > +{
> > + struct res_proc_context *c = context;
> > + struct resource r;
>
> We should initialize this to zero, otherwise insert_resource() will fail as
> parent, sibling etc. pointers are garbage.
Ah, sorry for overlooking that.
I'll resend the whole [1-3/3] series later today with this bug fixed
(hopefully).
> > + int i;
> > +
> > + if (c->preproc) {
> > + int ret;
> > +
> > + ret = c->preproc(ares, c->preproc_data);
> > + if (ret < 0) {
> > + c->error = ret;
> > + return AE_ABORT_METHOD;
> > + } else if (ret > 0) {
> > + return AE_OK;
> > + }
> > + }
> > +
> > + if (acpi_dev_resource_memory(ares, &r)
> > + || acpi_dev_resource_io(ares, &r)
> > + || acpi_dev_resource_address_space(ares, &r)
> > + || acpi_dev_resource_ext_address_space(ares, &r))
> > + return acpi_dev_new_resource_entry(&r, c);
> > +
> > + for (i = 0; acpi_dev_resource_interrupt(ares, i, &r); i++) {
> > + acpi_status status;
> > +
> > + status = acpi_dev_new_resource_entry(&r, c);
> > + if (ACPI_FAILURE(status))
> > + return status;
> > + }
> > +
> > + return AE_OK;
> > +}
> > +
> > +/**
> > + * acpi_dev_get_resources - Get current resources of a device.
> > + * @adev: ACPI device node to get the resources for.
> > + * @list: Head of the resultant list of resources (must be empty).
> > + * @preproc: The caller's preprocessing routine.
> > + * @preproc_data: Pointer passed to the caller's preprocessing routine.
> > + *
> > + * Evaluate the _CRS method for the given device node and process its
> > output by
> > + * (1) executing the @preproc() rountine provided by the caller, passing
> > the
> > + * resource pointer and @preproc_data to it as arguments, for each ACPI
> > resource
> > + * returned and (2) converting all of the returned ACPI resources into
> > struct
> > + * resource objects if possible. If the return value of @preproc() in
> > step (1)
> > + * is different from 0, step (2) is not applied to the given ACPI resource
> > and
> > + * if that value is negative, the whole processing is aborted and that
> > value is
> > + * returned as the final error code.
> > + *
> > + * The resultant struct resource objects are put on the list pointed to by
> > + * @list, that must be empty initially, as members of struct
> > resource_list_entry
> > + * objects. Callers of this routine should use
> > %acpi_dev_free_resource_list() to
> > + * free that list.
> > + *
> > + * The number of resources in the output list is returned on success, an
> > error
> > + * code reflecting the error condition is returned otherwise.
> > + */
> > +int acpi_dev_get_resources(struct acpi_device *adev, struct list_head
> > *list,
> > + int (*preproc)(struct acpi_resource *, void *),
> > + void *preproc_data)
>
> Could we change this so that you can pass NULL list as well (provided that
> the preproc is given)?
>
> This is useful in case when we try to find the SPI/I2C device handle based
> on the ACPI serial bus resource address. In that case we are not interested
> in any other resources (and hence there is no need to allocate memory for
> them etc.)
I'd prefer to have a separate helper function for that, if that's not a
problem. It should be clear when we ask for resources of a given device
and when we only want to poke things like that.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
