| Message ID | 20170710183248.2417-1-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 10, 2017 at 11:32 AM, Matthew Wilcox <willy@infradead.org> wrote: > From: Matthew Wilcox <mawilcox@microsoft.com> > > There was no need to have a minimum size of 4MB for NV-DIMMs; it was > just a sanity check. Keep a check that it's at least one page in size > because we really can't add less than a page to the memory map. Promote > the print statement from 'debug' level to 'warning', since there was no > information for my colleague who stumbled over this problem while > attempting to add a 2MB chunk of memory. > > Reported-by: Cheng-mean Liu <soccerl@microsoft.com> > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com> > --- > drivers/nvdimm/namespace_devs.c | 6 +++--- > include/uapi/linux/ndctl.h | 4 ---- > 2 files changed, 3 insertions(+), 7 deletions(-) Looks good to me, just one fix up: > > diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c > index 5f1c6756e57c..95169308078a 100644 > --- a/drivers/nvdimm/namespace_devs.c > +++ b/drivers/nvdimm/namespace_devs.c > @@ -1689,9 +1689,9 @@ struct nd_namespace_common *nvdimm_namespace_common_probe(struct device *dev) > } > > size = nvdimm_namespace_capacity(ndns); > - if (size < ND_MIN_NAMESPACE_SIZE) { > - dev_dbg(&ndns->dev, "%pa, too small must be at least %#x\n", > - &size, ND_MIN_NAMESPACE_SIZE); > + if (size < PAGE_SIZE) { If we're going to change the print level away from 'debug' then I think this should be "if (size && size < PAGE_SIZE)". The sub-system pre-creates 0-sized devices that are later configured into full namespaces, in those cases we shouldn't fire the warning.
On Mon, Jul 10, 2017 at 11:41:08AM -0700, Dan Williams wrote: > > size = nvdimm_namespace_capacity(ndns); > > - if (size < ND_MIN_NAMESPACE_SIZE) { > > - dev_dbg(&ndns->dev, "%pa, too small must be at least %#x\n", > > - &size, ND_MIN_NAMESPACE_SIZE); > > + if (size < PAGE_SIZE) { > > If we're going to change the print level away from 'debug' then I > think this should be "if (size && size < PAGE_SIZE)". The sub-system > pre-creates 0-sized devices that are later configured into full > namespaces, in those cases we shouldn't fire the warning. Should we allow drivers to claim zero-sized devices? If not, then the dev_warn() should be predicated on size being non-zero, but the return -ENODEV should still trigger.
I have tough time to image any scenarios that could make sense a zero-sized device. -----Original Message----- From: Matthew Wilcox [mailto:willy@infradead.org] Sent: Monday, July 10, 2017 1:30 PM To: Dan Williams <dan.j.williams@intel.com> Cc: linux-nvdimm@lists.01.org; Cheng-mean Liu (SOCCER) <soccerl@microsoft.com>; Matthew Wilcox <mawilcox@microsoft.com> Subject: Re: [PATCH] nvdimm: Remove minimum size requirement On Mon, Jul 10, 2017 at 11:41:08AM -0700, Dan Williams wrote: > > size = nvdimm_namespace_capacity(ndns); > > - if (size < ND_MIN_NAMESPACE_SIZE) { > > - dev_dbg(&ndns->dev, "%pa, too small must be at least %#x\n", > > - &size, ND_MIN_NAMESPACE_SIZE); > > + if (size < PAGE_SIZE) { > > If we're going to change the print level away from 'debug' then I > think this should be "if (size && size < PAGE_SIZE)". The sub-system > pre-creates 0-sized devices that are later configured into full > namespaces, in those cases we shouldn't fire the warning. Should we allow drivers to claim zero-sized devices? If not, then the dev_warn() should be predicated on size being non-zero, but the return -ENODEV should still trigger.
On Mon, Jul 10, 2017 at 1:30 PM, Matthew Wilcox <willy@infradead.org> wrote: > On Mon, Jul 10, 2017 at 11:41:08AM -0700, Dan Williams wrote: >> > size = nvdimm_namespace_capacity(ndns); >> > - if (size < ND_MIN_NAMESPACE_SIZE) { >> > - dev_dbg(&ndns->dev, "%pa, too small must be at least %#x\n", >> > - &size, ND_MIN_NAMESPACE_SIZE); >> > + if (size < PAGE_SIZE) { >> >> If we're going to change the print level away from 'debug' then I >> think this should be "if (size && size < PAGE_SIZE)". The sub-system >> pre-creates 0-sized devices that are later configured into full >> namespaces, in those cases we shouldn't fire the warning. > > Should we allow drivers to claim zero-sized devices? If not, then the > dev_warn() should be predicated on size being non-zero, but the return > -ENODEV should still trigger. You're right. It should return -ENODEV regardless, but the warning should be for non-zero too small namespaces.
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c index 5f1c6756e57c..95169308078a 100644 --- a/drivers/nvdimm/namespace_devs.c +++ b/drivers/nvdimm/namespace_devs.c @@ -1689,9 +1689,9 @@ struct nd_namespace_common *nvdimm_namespace_common_probe(struct device *dev) } size = nvdimm_namespace_capacity(ndns); - if (size < ND_MIN_NAMESPACE_SIZE) { - dev_dbg(&ndns->dev, "%pa, too small must be at least %#x\n", - &size, ND_MIN_NAMESPACE_SIZE); + if (size < PAGE_SIZE) { + dev_warn(&ndns->dev, "%pa, too small must be at least %ld\n", + &size, PAGE_SIZE); return ERR_PTR(-ENODEV); } diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h index 6d3c54264d8e..3ad1623bb585 100644 --- a/include/uapi/linux/ndctl.h +++ b/include/uapi/linux/ndctl.h @@ -299,10 +299,6 @@ enum nd_driver_flags { ND_DRIVER_DAX_PMEM = 1 << ND_DEVICE_DAX_PMEM, }; -enum { - ND_MIN_NAMESPACE_SIZE = 0x00400000, -}; - enum ars_masks { ARS_STATUS_MASK = 0x0000FFFF, ARS_EXT_STATUS_SHIFT = 16,