Message-ID: <94F2FBAB4432B54E8AACC7DFDE6C92E346BC08EB@ORSMSX101.amr.corp.intel.com>
Date:	Wed, 14 Nov 2012 02:23:51 +0000
From:	"Moore, Robert" <robert.moore@...el.com>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
CC:	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	"mathias.nyman@...ux.intel.com" <mathias.nyman@...ux.intel.com>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"lenb@...nel.org" <lenb@...nel.org>,
	"Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
	"broonie@...nsource.wolfsonmicro.com" 
	<broonie@...nsource.wolfsonmicro.com>,
	"grant.likely@...retlab.ca" <grant.likely@...retlab.ca>,
	"linus.walleij@...aro.org" <linus.walleij@...aro.org>,
	"khali@...ux-fr.org" <khali@...ux-fr.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	"Zheng, Lv" <lv.zheng@...el.com>
Subject: RE: [PATCH 3/3] ACPI: Evaluate _CRS while creating device node
 objects

Rafael,

I sounds like with a few changes, we can enhance this mechanism to be more useful to you and others. Some comments below. I need to look at the code in question a bit more, but I see no insurmountable issues.

Bob


> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@...k.pl]
> Sent: Tuesday, November 13, 2012 2:57 PM
> To: Moore, Robert
> Cc: Mika Westerberg; mathias.nyman@...ux.intel.com; linux-
> acpi@...r.kernel.org; linux-kernel@...r.kernel.org; lenb@...nel.org;
> Wysocki, Rafael J; broonie@...nsource.wolfsonmicro.com;
> grant.likely@...retlab.ca; linus.walleij@...aro.org; khali@...ux-fr.org;
> Bjorn Helgaas; Zheng, Lv
> Subject: Re: [PATCH 3/3] ACPI: Evaluate _CRS while creating device node
> objects
> 
> On Tuesday, November 13, 2012 10:06:03 PM Moore, Robert wrote:
> > I may not quite understand what you are asking for, but I will try.
> > It seems like we already have much of what you want/need, so maybe I'm
> > missing something.
> 
> I think all of the necessary pieces are there.
> 
> > > So what I would like to have, in general terms, is something like
> > > acpi_walk_resources() split into three parts:
> > >
> > >  (1) One that processes the _CRS output and creates a list of
> > >      struct acpi_resource objects for us to play with.  I suppose
> > >      it's OK if that's just a buffer filled with resource objects,
> > >      but a linked list might be more convenient.
> > >
> >
> > This sounds like AcpiGetCurrentResources. It executes _CRS and formats
> > the data into acpi_resource objects.
> 
> Yes, it does.  However, it is not completely clear to me if/how the caller
> is supposed to prepare the buffer object pointed to by the second arg.
> 
> If the buffer is initialized by AcpiGetCurrentResources, then that's what
> I need for (1).


It looks to me that at least AcpiGetCurrentResources does not actually ever allocate a buffer for the resource template, it expects the caller to eventually provide one of at least the size of the returned resource template.

This is really quite a bit out-of-date as far as the memory allocation model. It should also support the option to just allocate the buffer of the appropriate size before returning it to the caller.




> 
> > >  (2) One that allows us to access (read/write) resources in the
> > >      list returned by (1).  We don't need to open code walking
> > >      the list and I probably wouldn't event want to do that.  What
> > >      we need is to be able to walk the same list for a number of
> > >      times and possibly to modify values in the resource objects
> > >      if there are conflicts.
> >
> > This sounds like AcpiWalkResources. I suppose a possible issue is that
> > currently, AcpiWalkResources actually invokes the _CRS, _PRS, or _AEI
> > method on behalf of the caller.
> 
> Yes, that exactly is the problem.
> 
> > It might make more sense to allow the caller to pass in the resource
> > buffer returned from a call to _CRS, etc.
> 
> Yes! :-)


I'll take a closer look at this tomorrow.




> 
> > >
> > >  (3) One allowing us to free the list returned by (1) if not needed
> > >      any more.
> > >
> >
> > AcpiGetCurrentResources: Currently, everything is returned in a single
> > buffer to minimize the number of allocations. A buffer you can free
> > when you are done with it.
> 
> I suppose I should use ACPI_FREE(buffer.pointer) for that, but isn't it
> for the ACPICA's internal use only?
> 
> Besides, I would prefer to be able to pass just "buffer" for freeing,
> without having to touch its internals.  No big deal, but it would be
> nicer. :-)


The ACPI_BUFFER type is in fact a public type that is meant to return both the buffer and the (actual) length. You will find many instances of ACPI_FREE(buffer.pointer) within existing linux code, since it also used for objects returned by control method execution/object evaluation.


> 
> > I think I saw where you mentioned that you cannot copy this buffer
> > because of internal pointers to other areas of the buffer. Yes.
> > However, we can build linked lists all day if you really want them :-)
> 
> I really won't care if I can pass a resource buffer to a "walker" routine.
> :-)
> 
> > > And it would be great if we could take the list returned by (1),
> > > modify the resources in it and feed it back to _SRS (after
> > > conversion back to the format that _SRS understands).
> > >
> >
> > AcpiSetCurrentResources.
> >
> > The AML debugger already has a command that illustrates the use of the
> > various resource interfaces, see dbcmds.c
> 
> I will.
> 
> Thanks,
> Rafael
> 
> 
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Powered by blists - more mailing lists