From mboxrd@z Thu Jan 1 00:00:00 1970 From: Toshi Kani Subject: Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem entry Date: Wed, 17 Feb 2016 15:56:00 -0700 Message-ID: <1455749760.2925.291.camel@hpe.com> References: <1454439311-23690-1-git-send-email-toshi.kani@hpe.com> <1454439311-23690-4-git-send-email-toshi.kani@hpe.com> <1455316203.2925.132.camel@hpe.com> <1455674441.2925.204.camel@hpe.com> <1455732020.2925.255.camel@hpe.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Dan Williams Cc: "linux-nvdimm@lists.01.org" , "Rafael J. Wysocki" , "linux-kernel@vger.kernel.org" , Linux ACPI , Andrew Morton , Borislav Petkov , Ingo Molnar List-Id: linux-acpi@vger.kernel.org On Wed, 2016-02-17 at 11:34 -0800, Dan Williams wrote: > On Wed, Feb 17, 2016 at 10:00 AM, Toshi Kani wro= te: > > 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 > > > > wrote: =C2=A0: > > > Here are three options I can think of. > > >=20 > > > 1) Set pmem type to "reserved" (This patch-set) > > > =C2=A0- Add a new iomem_set_desc(), which sets a given type to a = top-level > > > entry.=C2=A0=C2=A0Change the ACPI NFIT driver to call it to set p= mem type to > > > "reserved" entry. > > > =C2=A0- region_intersects() finds a pmem entry by checking top-le= vel > > > entries (no change). > > >=20 > > > 2) Change region_intersects() to check children's type > > > =C2=A0- Add a new request_region_ext(), which is an extension to > > > request_region() to allow specifying a type of resource.=C2=A0=C2= =A0It puts a > > > new child entry under "reserved".=C2=A0=C2=A0Change the pmem driv= er to call > > > this func. > > > =C2=A0- Change region_intersects() to check children's type for f= inding > > > this child pmem entry. > > >=20 > > > 3) Pmem driver to call insert_resource() > > > =C2=A0- Change the pmem driver to call insert_resource(), which p= uts a new > > > pmem entry as the parent of "reserved". > > > =C2=A0- region_intersects() finds a pmem entry by checking top-le= vel > > > entries (no change). > > > =C2=A0- Add a new release_resource_self(), which releases a given= entry > > > and keeps its children if any.=C2=A0=C2=A0Change the pmem driver = to call it for > > > release. > >=20 > > Thinking=C2=A0further,=C2=A03)=C2=A0needs=C2=A0to=C2=A0be=C2=A0modi= fied=C2=A0as=C2=A0follows.=C2=A0=C2=A0insert_resource( > > ) should only be allowed for producers of resource (ex. nfit), not > > consumers (ex. pmem).=C2=A0=C2=A0It also needs to export insert_res= ource(). > >=20 > > 3) NFIT driver to call insert_resource() > > =C2=A0- 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 iom= em. > > =C2=A0This puts a new PMEM entry as the parent of "reserved". > > =C2=A0- region_intersects() finds a pmem entry by checking top-leve= l entries > > (no change). > > =C2=A0- Add a new release_resource_self(), which releases a given e= ntry and > > keeps its children if any.=C2=A0=C2=A0Change the nfit driver to cal= l it for > > release. > >=20 > > > This patch-set implements 1).=C2=A0=C2=A0The pmem type is set to = "reserved" for > > > its life-cycle.=C2=A0=C2=A0This option is simplest. > > >=20 > > > For 2), the changes to region_intersects() may be too complex for > > > maintenance. > >=20 > > I should have said "region_intersects() may be overly complicated f= or > > this purpose and maintenance". > >=20 > > > Here are a few examples when region_intersects() is called > > > with addr [1-10] where iomem has entry P and its children. > > >=20 > > > Case A: P is fully covered by children C1 & C2.=C2=A0=C2=A0region= _intersects() > > > ignores P's type, but checks C1 and C2's. > > >=20 > > > =C2=A0 P [1-10] + C1 [1-5] > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= + C2 [6-10] > > >=20 > > > Case B: C2 is fully covered by C3, but P is not.=C2=A0=C2=A0regio= n_intersects() > > > ignores C2's type, but checks P, C1, C3's. > > >=20 > > > =C2=A0 P [1-10] + C1 [1-2] > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= + C2 [6-10] + C3 [6-10] > > >=20 > > > I think region_intersects() will need to construct a flat table f= rom > > > the tree while making recursive calls to walk thru all children. > > >=20 > > > 3) is similar to 2), but avoids the changes to region_intersects(= ) > > > since insert_resource() inserts a new entry as the parent to > > > "reserved". > >=20 > > 3) is actually similar to 1) as both options change the producer si= de. > >=20 > > > =C2=A0However, a new interface is necessary to put "reserved" bac= k to top- > > > level when releasing the added entry. > > >=20 > > > My recommendation is go with either 1) or 3).=C2=A0=C2=A0What do = you think? > >=20 > > I think we should modify the producer side, so 1) or 3) are still m= y > > recommendation. > >=20 >=20 > I think 3 is the most promising option.=C2=A0=C2=A0It 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. >=20 > 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. Sounds good. =C2=A0I will implement 3), and add an ACPI built-in wrappe= r function to call insert_resource(). Thanks! -Toshi From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1423737AbcBQWDG (ORCPT ); Wed, 17 Feb 2016 17:03:06 -0500 Received: from g4t3427.houston.hp.com ([15.201.208.55]:52353 "EHLO g4t3427.houston.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965392AbcBQWDD (ORCPT ); Wed, 17 Feb 2016 17:03:03 -0500 Message-ID: <1455749760.2925.291.camel@hpe.com> Subject: Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem entry From: Toshi Kani To: Dan Williams Cc: "linux-nvdimm@lists.01.org" , "Rafael J. Wysocki" , "linux-kernel@vger.kernel.org" , Linux ACPI , Andrew Morton , Borislav Petkov , Ingo Molnar Date: Wed, 17 Feb 2016 15:56:00 -0700 In-Reply-To: References: <1454439311-23690-1-git-send-email-toshi.kani@hpe.com> <1454439311-23690-4-git-send-email-toshi.kani@hpe.com> <1455316203.2925.132.camel@hpe.com> <1455674441.2925.204.camel@hpe.com> <1455732020.2925.255.camel@hpe.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.4 (3.18.4-1.fc23) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2016-02-17 at 11:34 -0800, Dan Williams wrote: > On Wed, Feb 17, 2016 at 10:00 AM, Toshi Kani 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 > > > > wrote:  : > > > 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. Sounds good.  I will implement 3), and add an ACPI built-in wrapper function to call insert_resource(). Thanks! -Toshi