[PATCH v5] libnvdimm/pfn_dev: Don't clear device memmap area during generic namespace probe

2019-10-31 Thread Aneesh Kumar K.V
nvdimm core use nd_pfn_validate when looking for devdax or fsdax namespace. In 
this
case device resources are allocated against nd_namespace_io dev. In-order to
allow remap of range in nd_pfn_clear_memmap_error(), move the device memmap
area clearing while initializing pfn namespace. With this device
resource are allocated against nd_pfn and we can use nd_pfn->dev for remapping.

This also avoids calling nd_pfn_clear_mmap_errors twice. Once while probing the
namespace and second while initializing a pfn namespace.

Signed-off-by: Aneesh Kumar K.V 
---
Changes from v4:
* Make code changes suggested by Dan

 drivers/nvdimm/pfn_devs.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
index 60d81fae06ee..96727fd493f7 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -591,7 +591,7 @@ int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
return -ENXIO;
}
 
-   return nd_pfn_clear_memmap_errors(nd_pfn);
+   return 0;
 }
 EXPORT_SYMBOL(nd_pfn_validate);
 
@@ -729,6 +729,8 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn)
sig = PFN_SIG;
 
rc = nd_pfn_validate(nd_pfn, sig);
+   if (rc == 0)
+   return nd_pfn_clear_memmap_errors(nd_pfn);
if (rc != -ENODEV)
return rc;
 
@@ -796,6 +798,10 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn)
checksum = nd_sb_checksum((struct nd_gen_sb *) pfn_sb);
pfn_sb->checksum = cpu_to_le64(checksum);
 
+   rc = nd_pfn_clear_memmap_errors(nd_pfn);
+   if (rc)
+   return rc;
+
return nvdimm_write_bytes(ndns, SZ_4K, pfn_sb, sizeof(*pfn_sb), 0);
 }
 
-- 
2.23.0
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org


Re: [PATCH 4/4] modpost: do not set ->preloaded for symbols from Module.symvers

2019-10-31 Thread Masahiro Yamada
On Fri, Nov 1, 2019 at 1:51 AM Jeff Moyer  wrote:
>
> Masahiro Yamada  writes:
>
> > Now that there is no overwrap between symbols from ELF files and
> > ones from Module.symvers.
> >
> > So, the 'exported twice' warning should be reported irrespective
> > of where the symbol in question came from. Only the exceptional case
> > is when __crc_ symbol appears before __ksymtab_. This
> > typically occurs for EXPORT_SYMBOL in .S files.
>
> Hi, Masahiro,
>
> After apply this patch, I get the following modpost warnings when doing:
>
> $ make M=tools/tesing/nvdimm
> ...
>   Building modules, stage 2.
>   MODPOST 12 modules
> WARNING: tools/testing/nvdimm/libnvdimm: 'nvdimm_bus_lock' exported twice. 
> Previous export was in drivers/nvdimm/libnvdimm.ko
> WARNING: tools/testing/nvdimm/libnvdimm: 'nvdimm_bus_unlock' exported twice. 
> Previous export was in drivers/nvdimm/libnvdimm.ko
> WARNING: tools/testing/nvdimm/libnvdimm: 'is_nvdimm_bus_locked' exported 
> twice. Previous export was in drivers/nvdimm/libnvdimm.ko
> WARNING: tools/testing/nvdimm/libnvdimm: 'devm_nvdimm_memremap' exported 
> twice. Previous export was in drivers/nvdimm/libnvdimm.ko
> WARNING: tools/testing/nvdimm/libnvdimm: 'nd_fletcher64' exported twice. 
> Previous export was in drivers/nvdimm/libnvdimm.ko
> WARNING: tools/testing/nvdimm/libnvdimm: 'to_nd_desc' exported twice. 
> Previous export was in drivers/nvdimm/libnvdimm.ko
> WARNING: tools/testing/nvdimm/libnvdimm: 'to_nvdimm_bus_dev' exported twice. 
> Previous export was in drivers/nvdimm/libnvdimm.ko
> ...
>
> There are a lot of these warnings.  :)

These warnings are correct since
drivers/nvdimm/Makefile and
tools/testing/nvdimm/Kbuild
compile the same files.




>  If I revert this patch, no
> complaints.
>
> Cheers,
> Jeff
>
>
> >
> > Signed-off-by: Masahiro Yamada 
> > ---
> >
> >  scripts/mod/modpost.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 5234555cf550..6ca38d10efc5 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -2457,7 +2457,6 @@ static void read_dump(const char *fname, unsigned int 
> > kernel)
> >   s = sym_add_exported(symname, namespace, mod,
> >export_no(export));
> >   s->kernel= kernel;
> > - s->preloaded = 1;
> >   s->is_static = 0;
> >   sym_update_crc(symname, mod, crc, export_no(export));
> >   }
>


-- 
Best Regards
Masahiro Yamada
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org


[PATCH] libnvdimm/pmem: Delete include of nd-core.h

2019-10-31 Thread Dan Williams
The entire point of nd-core.h is to hide functionality that no leaf
driver should touch. In fact, the commit that added it had no need to
include it.

Fixes: 06e8ccdab15f ("acpi: nfit: Add support for detect platform...")
Cc: Ira Weiny 
Cc: Dave Jiang 
Cc: Vishal Verma 
Signed-off-by: Dan Williams 
---
 drivers/nvdimm/pmem.c |1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 7a6f4501dcda..ad8e4df1282b 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -28,7 +28,6 @@
 #include "pmem.h"
 #include "pfn.h"
 #include "nd.h"
-#include "nd-core.h"
 
 static struct device *to_dev(struct pmem_device *pmem)
 {
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org


Re: [PATCH v4 2/2] libnvdimm/namespace: Differentiate between probe mapping and runtime mapping

2019-10-31 Thread Dan Williams
On Thu, Oct 31, 2019 at 3:58 AM Aneesh Kumar K.V
 wrote:
>
> The nvdimm core currently maps the full namespace to an ioremap range
> while probing the namespace mode. This can result in probe failures on
> architectures that have limited ioremap space.
>
> For example, with a large btt namespace that consumes most of I/O remap
> range, depending on the sequence of namespace initialization, the user
> can find a pfn namespace initialization failure due to unavailable I/O
> remap space which nvdimm core uses for temporary mapping.
>
> nvdimm core can avoid this failure by only mapping the reserved info
> block area to check for pfn superblock type and map the full namespace
> resource only before using the namespace.
>
> Given that personalities like BTT can be layered on top of any namespace
> type create a generic form of devm_nsio_enable (devm_namespace_enable)
> and use it inside the per-personality attach routines. Now
> devm_namespace_enable() is always paired with disable unless the mapping
> is going to be used for long term runtime access.

Looks good to me, applied.
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org


Re: [PATCH v4 1/2] libnvdimm/pfn_dev: Don't clear device memmap area during generic namespace probe

2019-10-31 Thread Dan Williams
On Thu, Oct 31, 2019 at 3:57 AM Aneesh Kumar K.V
 wrote:
>
> nvdimm core use nd_pfn_validate when looking for devdax or fsdax namespace. 
> In this
> case device resources are allocated against nd_namespace_io dev. In-order to
> allow remap of range in nd_pfn_clear_memmap_error(), move the device memmap
> area clearing while initializing pfn namespace. With this device
> resource are allocated against nd_pfn and we can use nd_pfn->dev for 
> remapping.
>
> This also avoids calling nd_pfn_clear_mmap_errors twice. Once while probing 
> the
> namespace and second while initialzing a pfn namespace.

Nice find!

For the resend: s/initialzing/initializing/

>
> Signed-off-by: Aneesh Kumar K.V 
> ---
>  drivers/nvdimm/pfn_devs.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
> index 60d81fae06ee..fc2cd412861a 100644
> --- a/drivers/nvdimm/pfn_devs.c
> +++ b/drivers/nvdimm/pfn_devs.c
> @@ -591,7 +591,7 @@ int nd_pfn_validate(struct nd_pfn *nd_pfn, const char 
> *sig)
> return -ENXIO;
> }
>
> -   return nd_pfn_clear_memmap_errors(nd_pfn);
> +   return 0;
>  }
>  EXPORT_SYMBOL(nd_pfn_validate);
>
> @@ -730,7 +730,8 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn)
>
> rc = nd_pfn_validate(nd_pfn, sig);
> if (rc != -ENODEV)
> -   return rc;
> +   /* Clear the memap range of errors */

No need for this comment the following function name is descriptive enough.

> +   return nd_pfn_clear_memmap_errors(nd_pfn);

While this does allow for the errors to be cleared once on each
activation, it blocks validation errors from being reported up the
stack. It also does not address errors being cleared on create, but
that was also a problem shared with the current implementation.

I think you want something like this? This passes my testing, but
please review / test to make sure I'm not overlooking something, and
update the changelog to reflect that errors are now also cleared on
create.

diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
index 5a848a014d6d..c1eb99c0ca76 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -725,9 +725,10 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn)
sig = PFN_SIG;

rc = nd_pfn_validate(nd_pfn, sig);
-   if (rc != -ENODEV)
-   /* Clear the memap range of errors */
+   if (rc == 0)
return nd_pfn_clear_memmap_errors(nd_pfn);
+   if (rc != -ENODEV)
+   return rc;

/* no info block, do init */;
memset(pfn_sb, 0, sizeof(*pfn_sb));
@@ -793,7 +794,10 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn)
checksum = nd_sb_checksum((struct nd_gen_sb *) pfn_sb);
pfn_sb->checksum = cpu_to_le64(checksum);

-   return nvdimm_write_bytes(ndns, SZ_4K, pfn_sb, sizeof(*pfn_sb), 0);
+   rc = nvdimm_write_bytes(ndns, SZ_4K, pfn_sb, sizeof(*pfn_sb), 0);
+   if (rc)
+   return rc;
+   return nd_pfn_clear_memmap_errors(nd_pfn);
 }

 /*
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org


Re: [PATCH] Consider namespace with size as active namespace

2019-10-31 Thread Dan Williams
On Wed, Oct 30, 2019 at 4:39 PM Verma, Vishal L
 wrote:
>
> On Thu, 2019-10-17 at 08:35 +0530, Aneesh Kumar K.V wrote:
> >
> > > > ---
> > > >   ndctl/namespace.c | 3 ++-
> > > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/ndctl/namespace.c b/ndctl/namespace.c
> > > > index 58a9e3c53474..1f212a2b3a9b 100644
> > > > --- a/ndctl/namespace.c
> > > > +++ b/ndctl/namespace.c
> > > > @@ -455,7 +455,8 @@ static int is_namespace_active(struct 
> > > > ndctl_namespace *ndns)
> > > >   return ndns && (ndctl_namespace_is_enabled(ndns)
> > > >   || ndctl_namespace_get_pfn(ndns)
> > > >   || ndctl_namespace_get_dax(ndns)
> > > > - || ndctl_namespace_get_btt(ndns));
> > > > + || ndctl_namespace_get_btt(ndns)
> > > > + || ndctl_namespace_get_size(ndns));
> > > >   }
> > > >
> > > >   /*
> [..]
> >
> > > The failing unit tests are sector-mode.sh and dax.sh
> > >
> >
> > I will see if i can run them on ppc64. We still had issues in getting
> > ndctl check to be running on ppc64.
> >
>
> I dug into this a bit more.
>
> The failure happens on 'legacy' namespaces (ND_DEVICE_NAMESPACE_IO).
>
> There is an assumption that legacy namespaces cannot be fully deleted,
> so as part of a reconfigure, when it comes time to delete the namespace
> (ndctl_namespace_delete()), we refuse to do that, and bail, before
> setting the size to zero.
>
> libndctl.c:4467
>
> case ND_DEVICE_NAMESPACE_BLK:
> break;
> default:
> dbg(ctx, "%s: nstype: %d not deletable\n",
> ndctl_namespace_get_devname(ndns),
> ndctl_namespace_get_type(ndns));
> return 0;
> }
>
> rc = namespace_set_size(ndns, 0);
> ...
>
> Indeed, destroy namespace wouldn't even get to that point, because that
> assumption is repeated in namespace_destroy(), where we switch on
> namespace type, and potentially skip over the ndctl_namespace_destroy
> call entirely.
>
> If setting the size to zero is now significant we'd need to rework both
> of these sites. In destroy_namespace(), delay the did_zero checking
> until after ndctl_namespace_delete(), and in ndctl_namespace_delete(),
> set the size to zero before the type check.
>
> Dan, does the above make sense - was there reason to refrain from
> touching the size on legacy namespaces?

It's because the size is read-only on legacy namespaces, so writes
will always fail so the assumption is that ndctl_namespace_delete() is
a nop. Hmm, but that makes me think that size == read-only might be a
good gate for this idle check, i.e.:

if (size_is_writable(ndns) && size(ndns) != 0)
return not_idle;
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org


Re: [PATCH v3] nvdimm/btt: fix variable 'rc' set but not used

2019-10-31 Thread Verma, Vishal L
On Thu, 2019-10-31 at 10:05 -0400, Qian Cai wrote:
> drivers/nvdimm/btt.c: In function 'btt_read_pg':
> drivers/nvdimm/btt.c:1264:8: warning: variable 'rc' set but not used
> [-Wunused-but-set-variable]
> int rc;
> ^~
> 
> Add a ratelimited message in case a storm of errors is encountered.
> 
> Fixes: d9b83c756953 ("libnvdimm, btt: rework error clearing")
> Signed-off-by: Qian Cai 
> ---
> v3: remove the unused "rc" per Vishal.
> v2: include the block address that is returning an error per Dan.
> 
>  drivers/nvdimm/btt.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

Looks good,
Reviewed-by: Vishal Verma 

> 
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> index 3e9f45aec8d1..5129543a0473 100644
> --- a/drivers/nvdimm/btt.c
> +++ b/drivers/nvdimm/btt.c
> @@ -1261,11 +1261,11 @@ static int btt_read_pg(struct btt *btt, struct 
> bio_integrity_payload *bip,
>  
>   ret = btt_data_read(arena, page, off, postmap, cur_len);
>   if (ret) {
> - int rc;
> -
>   /* Media error - set the e_flag */
> - rc = btt_map_write(arena, premap, postmap, 0, 1,
> - NVDIMM_IO_ATOMIC);
> + if (btt_map_write(arena, premap, postmap, 0, 1, 
> NVDIMM_IO_ATOMIC))
> + dev_warn_ratelimited(to_dev(arena),
> + "Error persistently tracking bad blocks 
> at %#x\n",
> + premap);
>   goto out_rtt;
>   }
>  

___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org


Re: [PATCH 4/4] modpost: do not set ->preloaded for symbols from Module.symvers

2019-10-31 Thread Jeff Moyer
Masahiro Yamada  writes:

> Now that there is no overwrap between symbols from ELF files and
> ones from Module.symvers.
>
> So, the 'exported twice' warning should be reported irrespective
> of where the symbol in question came from. Only the exceptional case
> is when __crc_ symbol appears before __ksymtab_. This
> typically occurs for EXPORT_SYMBOL in .S files.

Hi, Masahiro,

After apply this patch, I get the following modpost warnings when doing:

$ make M=tools/tesing/nvdimm
...
  Building modules, stage 2.
  MODPOST 12 modules
WARNING: tools/testing/nvdimm/libnvdimm: 'nvdimm_bus_lock' exported twice. 
Previous export was in drivers/nvdimm/libnvdimm.ko
WARNING: tools/testing/nvdimm/libnvdimm: 'nvdimm_bus_unlock' exported twice. 
Previous export was in drivers/nvdimm/libnvdimm.ko
WARNING: tools/testing/nvdimm/libnvdimm: 'is_nvdimm_bus_locked' exported twice. 
Previous export was in drivers/nvdimm/libnvdimm.ko
WARNING: tools/testing/nvdimm/libnvdimm: 'devm_nvdimm_memremap' exported twice. 
Previous export was in drivers/nvdimm/libnvdimm.ko
WARNING: tools/testing/nvdimm/libnvdimm: 'nd_fletcher64' exported twice. 
Previous export was in drivers/nvdimm/libnvdimm.ko
WARNING: tools/testing/nvdimm/libnvdimm: 'to_nd_desc' exported twice. Previous 
export was in drivers/nvdimm/libnvdimm.ko
WARNING: tools/testing/nvdimm/libnvdimm: 'to_nvdimm_bus_dev' exported twice. 
Previous export was in drivers/nvdimm/libnvdimm.ko
...

There are a lot of these warnings.  :)  If I revert this patch, no
complaints.

Cheers,
Jeff


>
> Signed-off-by: Masahiro Yamada 
> ---
>
>  scripts/mod/modpost.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 5234555cf550..6ca38d10efc5 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2457,7 +2457,6 @@ static void read_dump(const char *fname, unsigned int 
> kernel)
>   s = sym_add_exported(symname, namespace, mod,
>export_no(export));
>   s->kernel= kernel;
> - s->preloaded = 1;
>   s->is_static = 0;
>   sym_update_crc(symname, mod, crc, export_no(export));
>   }
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org


Re: [RFC PATCH 1/4] libnvdimm/namespace: Make namespace size validation arch dependent

2019-10-31 Thread Dan Williams
On Thu, Oct 31, 2019 at 1:38 AM Aneesh Kumar K.V
 wrote:
>
> On 10/31/19 12:00 PM, Dan Williams wrote:
> > On Wed, Oct 30, 2019 at 10:35 PM Aneesh Kumar K.V
> >  wrote:
> > [..]
>
> >
> >
> > All that said, the x86 vmemmap_populate() falls back to use small
> > pages in some case to get around this constraint. Can't powerpc do the
> > same? It would seem to be less work than the above proposal.
> >
>
> ppc64 supports two translation mode (radix and hash). We can do the
> above with radix translation. With hash we use just one page size( in
> this specific case 16MB) for direct-map mapping.

Ok, if it's truly a hard constraint then yes, more infrastructure is
needed to expose that constraint to the namespace provisioning flow.
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org


[PATCH v3] nvdimm/btt: fix variable 'rc' set but not used

2019-10-31 Thread Qian Cai
drivers/nvdimm/btt.c: In function 'btt_read_pg':
drivers/nvdimm/btt.c:1264:8: warning: variable 'rc' set but not used
[-Wunused-but-set-variable]
int rc;
^~

Add a ratelimited message in case a storm of errors is encountered.

Fixes: d9b83c756953 ("libnvdimm, btt: rework error clearing")
Signed-off-by: Qian Cai 
---
v3: remove the unused "rc" per Vishal.
v2: include the block address that is returning an error per Dan.

 drivers/nvdimm/btt.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
index 3e9f45aec8d1..5129543a0473 100644
--- a/drivers/nvdimm/btt.c
+++ b/drivers/nvdimm/btt.c
@@ -1261,11 +1261,11 @@ static int btt_read_pg(struct btt *btt, struct 
bio_integrity_payload *bip,
 
ret = btt_data_read(arena, page, off, postmap, cur_len);
if (ret) {
-   int rc;
-
/* Media error - set the e_flag */
-   rc = btt_map_write(arena, premap, postmap, 0, 1,
-   NVDIMM_IO_ATOMIC);
+   if (btt_map_write(arena, premap, postmap, 0, 1, 
NVDIMM_IO_ATOMIC))
+   dev_warn_ratelimited(to_dev(arena),
+   "Error persistently tracking bad blocks 
at %#x\n",
+   premap);
goto out_rtt;
}
 
-- 
1.8.3.1
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org


[PATCH v4 1/2] libnvdimm/pfn_dev: Don't clear device memmap area during generic namespace probe

2019-10-31 Thread Aneesh Kumar K.V
nvdimm core use nd_pfn_validate when looking for devdax or fsdax namespace. In 
this
case device resources are allocated against nd_namespace_io dev. In-order to
allow remap of range in nd_pfn_clear_memmap_error(), move the device memmap
area clearing while initializing pfn namespace. With this device
resource are allocated against nd_pfn and we can use nd_pfn->dev for remapping.

This also avoids calling nd_pfn_clear_mmap_errors twice. Once while probing the
namespace and second while initialzing a pfn namespace.

Signed-off-by: Aneesh Kumar K.V 
---
 drivers/nvdimm/pfn_devs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
index 60d81fae06ee..fc2cd412861a 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -591,7 +591,7 @@ int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
return -ENXIO;
}
 
-   return nd_pfn_clear_memmap_errors(nd_pfn);
+   return 0;
 }
 EXPORT_SYMBOL(nd_pfn_validate);
 
@@ -730,7 +730,8 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn)
 
rc = nd_pfn_validate(nd_pfn, sig);
if (rc != -ENODEV)
-   return rc;
+   /* Clear the memap range of errors */
+   return nd_pfn_clear_memmap_errors(nd_pfn);
 
/* no info block, do init */;
memset(pfn_sb, 0, sizeof(*pfn_sb));
-- 
2.23.0
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org


[PATCH v4 2/2] libnvdimm/namespace: Differentiate between probe mapping and runtime mapping

2019-10-31 Thread Aneesh Kumar K.V
The nvdimm core currently maps the full namespace to an ioremap range
while probing the namespace mode. This can result in probe failures on
architectures that have limited ioremap space.

For example, with a large btt namespace that consumes most of I/O remap
range, depending on the sequence of namespace initialization, the user
can find a pfn namespace initialization failure due to unavailable I/O
remap space which nvdimm core uses for temporary mapping.

nvdimm core can avoid this failure by only mapping the reserved info
block area to check for pfn superblock type and map the full namespace
resource only before using the namespace.

Given that personalities like BTT can be layered on top of any namespace
type create a generic form of devm_nsio_enable (devm_namespace_enable)
and use it inside the per-personality attach routines. Now
devm_namespace_enable() is always paired with disable unless the mapping
is going to be used for long term runtime access.

Signed-off-by: Aneesh Kumar K.V 
Link: 
https://lore.kernel.org/r/20191017073308.32645-1-aneesh.ku...@linux.ibm.com
[djbw: reworks to move devm_namespace_{en,dis}able into *attach helpers]
Signed-off-by: Dan Williams 
---
 drivers/dax/pmem/core.c |  6 +++---
 drivers/nvdimm/btt.c| 10 --
 drivers/nvdimm/claim.c  | 14 ++
 drivers/nvdimm/namespace_devs.c | 17 +
 drivers/nvdimm/nd-core.h| 16 
 drivers/nvdimm/nd.h | 22 +-
 drivers/nvdimm/pfn_devs.c   | 18 +++---
 drivers/nvdimm/pmem.c   | 17 +
 8 files changed, 83 insertions(+), 37 deletions(-)

diff --git a/drivers/dax/pmem/core.c b/drivers/dax/pmem/core.c
index 6eb6dfdf19bf..2bedf8414fff 100644
--- a/drivers/dax/pmem/core.c
+++ b/drivers/dax/pmem/core.c
@@ -25,20 +25,20 @@ struct dev_dax *__dax_pmem_probe(struct device *dev, enum 
dev_dax_subsys subsys)
ndns = nvdimm_namespace_common_probe(dev);
if (IS_ERR(ndns))
return ERR_CAST(ndns);
-   nsio = to_nd_namespace_io(>dev);
 
/* parse the 'pfn' info block via ->rw_bytes */
-   rc = devm_nsio_enable(dev, nsio);
+   rc = devm_namespace_enable(dev, ndns, nd_info_block_reserve());
if (rc)
return ERR_PTR(rc);
rc = nvdimm_setup_pfn(nd_pfn, );
if (rc)
return ERR_PTR(rc);
-   devm_nsio_disable(dev, nsio);
+   devm_namespace_disable(dev, ndns);
 
/* reserve the metadata area, device-dax will reserve the data */
pfn_sb = nd_pfn->pfn_sb;
offset = le64_to_cpu(pfn_sb->dataoff);
+   nsio = to_nd_namespace_io(>dev);
if (!devm_request_mem_region(dev, nsio->res.start, offset,
dev_name(>dev))) {
dev_warn(dev, "could not reserve metadata\n");
diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
index 3e9f45aec8d1..8cb890a987b0 100644
--- a/drivers/nvdimm/btt.c
+++ b/drivers/nvdimm/btt.c
@@ -1674,7 +1674,8 @@ int nvdimm_namespace_attach_btt(struct 
nd_namespace_common *ndns)
struct nd_region *nd_region;
struct btt_sb *btt_sb;
struct btt *btt;
-   size_t rawsize;
+   size_t size, rawsize;
+   int rc;
 
if (!nd_btt->uuid || !nd_btt->ndns || !nd_btt->lbasize) {
dev_dbg(_btt->dev, "incomplete btt configuration\n");
@@ -1685,6 +1686,11 @@ int nvdimm_namespace_attach_btt(struct 
nd_namespace_common *ndns)
if (!btt_sb)
return -ENOMEM;
 
+   size = nvdimm_namespace_capacity(ndns);
+   rc = devm_namespace_enable(_btt->dev, ndns, size);
+   if (rc)
+   return rc;
+
/*
 * If this returns < 0, that is ok as it just means there wasn't
 * an existing BTT, and we're creating a new one. We still need to
@@ -1693,7 +1699,7 @@ int nvdimm_namespace_attach_btt(struct 
nd_namespace_common *ndns)
 */
nd_btt_version(nd_btt, ndns, btt_sb);
 
-   rawsize = nvdimm_namespace_capacity(ndns) - nd_btt->initial_offset;
+   rawsize = size - nd_btt->initial_offset;
if (rawsize < ARENA_MIN_SIZE) {
dev_dbg(_btt->dev, "%s must be at least %ld bytes\n",
dev_name(>dev),
diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c
index 2985ca949912..45964acba944 100644
--- a/drivers/nvdimm/claim.c
+++ b/drivers/nvdimm/claim.c
@@ -300,13 +300,14 @@ static int nsio_rw_bytes(struct nd_namespace_common *ndns,
return rc;
 }
 
-int devm_nsio_enable(struct device *dev, struct nd_namespace_io *nsio)
+int devm_nsio_enable(struct device *dev, struct nd_namespace_io *nsio,
+   resource_size_t size)
 {
struct resource *res = >res;
struct nd_namespace_common *ndns = >common;
 
-   nsio->size = resource_size(res);
-   if (!devm_request_mem_region(dev, res->start, resource_size(res),
+   nsio->size = size;
+   if 

Re: [RFC PATCH 1/4] libnvdimm/namespace: Make namespace size validation arch dependent

2019-10-31 Thread Aneesh Kumar K.V

On 10/31/19 12:00 PM, Dan Williams wrote:

On Wed, Oct 30, 2019 at 10:35 PM Aneesh Kumar K.V
 wrote:
[..]





All that said, the x86 vmemmap_populate() falls back to use small
pages in some case to get around this constraint. Can't powerpc do the
same? It would seem to be less work than the above proposal.



ppc64 supports two translation mode (radix and hash). We can do the 
above with radix translation. With hash we use just one page size( in 
this specific case 16MB) for direct-map mapping.


-aneesh
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org


Re: [RFC PATCH 1/4] libnvdimm/namespace: Make namespace size validation arch dependent

2019-10-31 Thread Dan Williams
On Wed, Oct 30, 2019 at 10:35 PM Aneesh Kumar K.V
 wrote:
[..]
> > True, for the pfn device and the device-dax mapping size, but I'm
> > suggesting adding another instance of alignment control at the raw
> > namespace level. That would need to be disconnected from the
> > device-dax page mapping granularity.
> >
>
> Can you explain what you mean by raw namespace level ? We don't have
> multiple values against which we need to check the alignment of
> namespace start and namespace size.
>
> If you can outline how and where you would like to enforce that check I
> can start working on it.
>

What I mean is that the process of setting up a pfn namespace goes
something like this in shell script form:

1/ echo $size > /sys/bus/nd/devices/$namespace/size
2/ echo $namespace > /sys/bus/nd/devices/$pfn/namespace
3/ echo $pfn_align > /sys/bus/nd/devices/$pfn/align

What I'm suggesting is add an optional 0th step that does:

echo $raw_align > /sys/bus/nd/devices/$namespace/align

Where the raw align needs to be needs to be max($pfn_align,
arch_mapping_granulariy).

So on powerpc where PAGE_SIZE < arch_mapping_granulariy, the following:

cat /sys/bus/nd/devices/$namespace/supported_aligns

...would show the same output as:

cat /sys/bus/nd/devices/$pfn/align

...but with any alignment choice less than arch_mapping_granulariy removed.



All that said, the x86 vmemmap_populate() falls back to use small
pages in some case to get around this constraint. Can't powerpc do the
same? It would seem to be less work than the above proposal.
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org