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. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
