diff mbox

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

Message ID 1454439311-23690-4-git-send-email-toshi.kani@hpe.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kani, Toshi Feb. 2, 2016, 6:55 p.m. UTC
Change acpi_nfit_register_region() to call iomem_set_desc() with
IORES_DESC_PERSISTENT_MEMORY for NFIT_SPA_PM ranges found in ACPI
NFIT table.

When FW sets E820_PMEM in e820 and EFI_PERSISTENT_MEMORY in EFI,
this code simply sets PMEM type again to "Persistent Memory" entries
in the iomem table.  When FW sets reserved type for persistent
memory ranges, it sets PMEM type to "reserved" entries covering
PMEM ranges.

This allows the EINJ driver, which calls region_intersects() with
IORES_DESC_PERSISTENT_MEMORY to check persistent memory ranges,
to work continuously even if FW sets reserved type to persistent
memory in e820 and EFI.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 drivers/acpi/nfit.c |    6 ++++++
 1 file changed, 6 insertions(+)

Comments

Dan Williams Feb. 12, 2016, 7:41 p.m. UTC | #1
On Tue, Feb 2, 2016 at 10:55 AM, Toshi Kani <toshi.kani@hpe.com> wrote:
> Change acpi_nfit_register_region() to call iomem_set_desc() with
> IORES_DESC_PERSISTENT_MEMORY for NFIT_SPA_PM ranges found in ACPI
> NFIT table.
>
> When FW sets E820_PMEM in e820 and EFI_PERSISTENT_MEMORY in EFI,
> this code simply sets PMEM type again to "Persistent Memory" entries
> in the iomem table.  When FW sets reserved type for persistent
> memory ranges, it sets PMEM type to "reserved" entries covering
> PMEM ranges.
>
> This allows the EINJ driver, which calls region_intersects() with
> IORES_DESC_PERSISTENT_MEMORY to check persistent memory ranges,
> to work continuously even if FW sets reserved type to persistent
> memory in e820 and EFI.
>
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  drivers/acpi/nfit.c |    6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index ad6d8c6..add04f0 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -1781,6 +1781,12 @@ static int acpi_nfit_register_region(struct acpi_nfit_desc *acpi_desc,
>
>         nvdimm_bus = acpi_desc->nvdimm_bus;
>         if (nfit_spa_type(spa) == NFIT_SPA_PM) {
> +               rc = iomem_set_desc(spa->address, spa->length,
> +                                       IORES_DESC_PERSISTENT_MEMORY);
> +               if (rc)
> +                       dev_dbg(acpi_desc->dev,
> +                               "error setting iomem desc: %d\n", rc);
> +

Hmm, if we set the type on driver load, should we clear the type on
driver unload?

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.
Kani, Toshi Feb. 12, 2016, 10:30 p.m. UTC | #2
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@hpe.com> wrote:
> > Change acpi_nfit_register_region() to call iomem_set_desc() with
> > IORES_DESC_PERSISTENT_MEMORY for NFIT_SPA_PM ranges found in ACPI
> > NFIT table.
> > 
> > When FW sets E820_PMEM in e820 and EFI_PERSISTENT_MEMORY in EFI,
> > this code simply sets PMEM type again to "Persistent Memory" entries
> > in the iomem table.  When FW sets reserved type for persistent
> > memory ranges, it sets PMEM type to "reserved" entries covering
> > PMEM ranges.
> > 
> > This allows the EINJ driver, which calls region_intersects() with
> > IORES_DESC_PERSISTENT_MEMORY to check persistent memory ranges,
> > to work continuously even if FW sets reserved type to persistent
> > memory in e820 and EFI.
> > 
> > Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Borislav Petkov <bp@suse.de>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > ---
> >  drivers/acpi/nfit.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> > index ad6d8c6..add04f0 100644
> > --- a/drivers/acpi/nfit.c
> > +++ b/drivers/acpi/nfit.c
> > @@ -1781,6 +1781,12 @@ static int acpi_nfit_register_region(struct
> > acpi_nfit_desc *acpi_desc,
> > 
> >         nvdimm_bus = acpi_desc->nvdimm_bus;
> >         if (nfit_spa_type(spa) == NFIT_SPA_PM) {
> > +               rc = iomem_set_desc(spa->address, spa->length,
> > +                                       IORES_DESC_PERSISTENT_MEMORY);
> > +               if (rc)
> > +                       dev_dbg(acpi_desc->dev,
> > +                               "error setting iomem desc: %d\n", rc);
> > +
> 
> 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.

Thanks,
-Toshi
Dan Williams Feb. 12, 2016, 11:32 p.m. UTC | #3
On Fri, Feb 12, 2016 at 2:30 PM, Toshi Kani <toshi.kani@hpe.com> 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@hpe.com> wrote:
>> > Change acpi_nfit_register_region() to call iomem_set_desc() with
>> > IORES_DESC_PERSISTENT_MEMORY for NFIT_SPA_PM ranges found in ACPI
>> > NFIT table.
>> >
>> > When FW sets E820_PMEM in e820 and EFI_PERSISTENT_MEMORY in EFI,
>> > this code simply sets PMEM type again to "Persistent Memory" entries
>> > in the iomem table.  When FW sets reserved type for persistent
>> > memory ranges, it sets PMEM type to "reserved" entries covering
>> > PMEM ranges.
>> >
>> > This allows the EINJ driver, which calls region_intersects() with
>> > IORES_DESC_PERSISTENT_MEMORY to check persistent memory ranges,
>> > to work continuously even if FW sets reserved type to persistent
>> > memory in e820 and EFI.
>> >
>> > Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
>> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>> > Cc: Dan Williams <dan.j.williams@intel.com>
>> > Cc: Ingo Molnar <mingo@kernel.org>
>> > Cc: Borislav Petkov <bp@suse.de>
>> > Cc: Andrew Morton <akpm@linux-foundation.org>
>> > ---
>> >  drivers/acpi/nfit.c |    6 ++++++
>> >  1 file changed, 6 insertions(+)
>> >
>> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
>> > index ad6d8c6..add04f0 100644
>> > --- a/drivers/acpi/nfit.c
>> > +++ b/drivers/acpi/nfit.c
>> > @@ -1781,6 +1781,12 @@ static int acpi_nfit_register_region(struct
>> > acpi_nfit_desc *acpi_desc,
>> >
>> >         nvdimm_bus = acpi_desc->nvdimm_bus;
>> >         if (nfit_spa_type(spa) == NFIT_SPA_PM) {
>> > +               rc = iomem_set_desc(spa->address, spa->length,
>> > +                                       IORES_DESC_PERSISTENT_MEMORY);
>> > +               if (rc)
>> > +                       dev_dbg(acpi_desc->dev,
>> > +                               "error setting iomem desc: %d\n", rc);
>> > +
>>
>> 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?
Kani, Toshi Feb. 17, 2016, 2 a.m. UTC | #4
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@hpe.com> 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@hpe.com> 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.


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.  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".
 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?

Thanks,
-Toshi
Kani, Toshi Feb. 17, 2016, 6 p.m. UTC | #5
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@hpe.com> 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@hpe.com>
> > > > 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
Dan Williams Feb. 17, 2016, 7:34 p.m. UTC | #6
On Wed, Feb 17, 2016 at 10:00 AM, Toshi Kani <toshi.kani@hpe.com> 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@hpe.com> 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@hpe.com>
>> > > > 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.
Kani, Toshi Feb. 17, 2016, 10:56 p.m. UTC | #7
On Wed, 2016-02-17 at 11:34 -0800, Dan Williams wrote:
> On Wed, Feb 17, 2016 at 10:00 AM, Toshi Kani <toshi.kani@hpe.com> 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@hpe.com>
> > > > 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
diff mbox

Patch

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index ad6d8c6..add04f0 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -1781,6 +1781,12 @@  static int acpi_nfit_register_region(struct acpi_nfit_desc *acpi_desc,
 
 	nvdimm_bus = acpi_desc->nvdimm_bus;
 	if (nfit_spa_type(spa) == NFIT_SPA_PM) {
+		rc = iomem_set_desc(spa->address, spa->length,
+					IORES_DESC_PERSISTENT_MEMORY);
+		if (rc)
+			dev_dbg(acpi_desc->dev,
+				"error setting iomem desc: %d\n", rc);
+
 		rc = acpi_nfit_find_poison(acpi_desc, ndr_desc);
 		if (rc) {
 			dev_err(acpi_desc->dev,