Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem entry

[Dan Williams]

On Wed, Feb 17, 2016 at 10:00 AM, Toshi Kani <toshi.kani@xxxxxxx> wrote:
> On Tue, 2016-02-16 at 19:00 -0700, Toshi Kani wrote:
>> On Fri, 2016-02-12 at 15:32 -0800, Dan Williams wrote:
>> > On Fri, Feb 12, 2016 at 2:30 PM, Toshi Kani <toshi.kani@xxxxxxx> wrote:
>> > > On Fri, 2016-02-12 at 11:41 -0800, Dan Williams wrote:
>> > > > On Tue, Feb 2, 2016 at 10:55 AM, Toshi Kani <toshi.kani@xxxxxxx>
>> > > > wro
>>  :
>> > > > Hmm, if we set the type on driver load, should we clear the type on
>> > > > driver unload?
>> > >
>> > > I think this type update should stay for the life-cycle of this iomem
>> > > entry itself since this range is PMEM even after the driver is
>> > > unloaded.  This is an extension of the boot-time iomem table
>> > > initialization from e820/EFI, which allows ACPI to set a correct
>> > > type.  This is independent from driver's resource allocations.
>> > >
>> > > > Actually it might be more straightforward to specify a type at
>> > > > request_region() time.  That way it gets released at
>> > > > release_region().  We're already setting a resource name at
>> > > > request_region time, adding a type annotation at the time seems
>> > > > appropriate.
>> > >
>> > > I first considered simply setting "namespaceX.X" as PMEM.  However,
>> > > region_intersects() and its friends only check the top-level entries,
>> > > not their children, of the iomem table.  And I think a child should
>> > > have the same type as the parent as I fixed it in patch 1/3.
>> >
>> > Did we investigate updating region_intersects() to check children?
>> > When a child sub-divides a region with different types it may be the
>> > wrong answer to check the parent.  Is there a problem with moving
>> > checking to the child?
>>
>> Here are three options I can think of.
>>
>> 1) Set pmem type to "reserved" (This patch-set)
>>  - Add a new iomem_set_desc(), which sets a given type to a top-level
>> entry.  Change the ACPI NFIT driver to call it to set pmem type to
>> "reserved" entry.
>>  - region_intersects() finds a pmem entry by checking top-level entries
>> (no change).
>>
>> 2) Change region_intersects() to check children's type
>>  - Add a new request_region_ext(), which is an extension to
>> request_region() to allow specifying a type of resource.  It puts a new
>> child entry under "reserved".  Change the pmem driver to call this func.
>>  - Change region_intersects() to check children's type for finding this
>> child pmem entry.
>>
>> 3) Pmem driver to call insert_resource()
>>  - Change the pmem driver to call insert_resource(), which puts a new
>> pmem entry as the parent of "reserved".
>>  - region_intersects() finds a pmem entry by checking top-level entries
>> (no change).
>>  - Add a new release_resource_self(), which releases a given entry and
>> keeps its children if any.  Change the pmem driver to call it for
>> release.
>
> Thinking further, 3) needs to be modified as follows.  insert_resource()
> should only be allowed for producers of resource (ex. nfit), not consumers
> (ex. pmem).  It also needs to export insert_resource().
>
> 3) NFIT driver to call insert_resource()
>  - Change the ACPI nfit driver to call insert_resource() when a target
> range is not marked as PMEM (i.e. "reserved") or not present in iomem.
>  This puts a new PMEM entry as the parent of "reserved".
>  - region_intersects() finds a pmem entry by checking top-level entries (no
> change).
>  - Add a new release_resource_self(), which releases a given entry and
> keeps its children if any.  Change the nfit driver to call it for release.
>
>> This patch-set implements 1).  The pmem type is set to "reserved" for its
>> life-cycle.  This option is simplest.
>>
>> For 2), the changes to region_intersects() may be too complex for
>> maintenance.
>
> I should have said "region_intersects() may be overly complicated for this
> purpose and maintenance".
>
>> Here are a few examples when region_intersects() is called
>> with addr [1-10] where iomem has entry P and its children.
>>
>> Case A: P is fully covered by children C1 & C2.  region_intersects()
>> ignores P's type, but checks C1 and C2's.
>>
>>   P [1-10] + C1 [1-5]
>>            + C2 [6-10]
>>
>> Case B: C2 is fully covered by C3, but P is not.  region_intersects()
>> ignores C2's type, but checks P, C1, C3's.
>>
>>   P [1-10] + C1 [1-2]
>>            + C2 [6-10] + C3 [6-10]
>>
>> I think region_intersects() will need to construct a flat table from the
>> tree while making recursive calls to walk thru all children.
>>
>> 3) is similar to 2), but avoids the changes to region_intersects() since
>> insert_resource() inserts a new entry as the parent to "reserved".
>
> 3) is actually similar to 1) as both options change the producer side.
>
>>  However, a new interface is necessary to put "reserved" back to top-
>> level when releasing the added entry.
>>
>> My recommendation is go with either 1) or 3).  What do you think?
>
> I think we should modify the producer side, so 1) or 3) are still my
> recommendation.
>

I think 3 is the most promising option.  It aligns the acpi/nfit
driver closer with the acpi/pci_root driver that is also doing
insert_resource() for each root bridge, and removing those resources
when the bridge is disabled/removed.

I'd still want to maintain the ability of nfit to be built as a
module, so we would need to split the resource registration into a
separate built-in object file from nfit.ko.