Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1333068 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem entry Dan Williams <dan.j.williams@intel.com> - 2016-02-12 20:50 +0100
    Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem  entry Toshi Kani <toshi.kani@hpe.com> - 2016-02-12 22:40 +0100
      Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem entry Dan Williams <dan.j.williams@intel.com> - 2016-02-13 00:40 +0100
        Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem  entry Toshi Kani <toshi.kani@hpe.com> - 2016-02-17 02:10 +0100
          Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem  entry Toshi Kani <toshi.kani@hpe.com> - 2016-02-17 18:10 +0100
            Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem entry Dan Williams <dan.j.williams@intel.com> - 2016-02-17 20:40 +0100
              Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem  entry Toshi Kani <toshi.kani@hpe.com> - 2016-02-17 23:10 +0100

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

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.

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1333211

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?

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1336702

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web