Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem entry
From: Toshi Kani
Date: Wed Feb 17 2016 - 12:07:27 EST
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.
Thanks,
-Toshi