| Message ID | 20190124231442.EFD29EE0@viggo.jf.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Allow persistent memory to be used like normal RAM | expand |
On Thu, Jan 24, 2019 at 5:21 PM Dave Hansen <dave.hansen@linux.intel.com> wrote: > > > From: Dave Hansen <dave.hansen@linux.intel.com> > > walk_system_ram_range() can return an error code either becuase *it* > failed, or because the 'func' that it calls returned an error. The > memory hotplug does the following: > > ret = walk_system_ram_range(..., func); > if (ret) > return ret; > > and 'ret' makes it out to userspace, eventually. The problem is, > walk_system_ram_range() failues that result from *it* failing (as > opposed to 'func') return -1. That leads to a very odd -EPERM (-1) > return code out to userspace. > > Make walk_system_ram_range() return -EINVAL for internal failures to > keep userspace less confused. > > This return code is compatible with all the callers that I audited. > > Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Dave Jiang <dave.jiang@intel.com> > Cc: Ross Zwisler <zwisler@kernel.org> > Cc: Vishal Verma <vishal.l.verma@intel.com> > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Michal Hocko <mhocko@suse.com> > Cc: linux-nvdimm@lists.01.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-mm@kvack.org > Cc: Huang Ying <ying.huang@intel.com> > Cc: Fengguang Wu <fengguang.wu@intel.com> > Cc: Borislav Petkov <bp@suse.de> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Yaowei Bai <baiyaowei@cmss.chinamobile.com> > Cc: Takashi Iwai <tiwai@suse.de> > Cc: Jerome Glisse <jglisse@redhat.com> > --- > > b/kernel/resource.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff -puN kernel/resource.c~memory-hotplug-walk_system_ram_range-returns-neg-1 kernel/resource.c > --- a/kernel/resource.c~memory-hotplug-walk_system_ram_range-returns-neg-1 2019-01-24 15:13:13.950199540 -0800 > +++ b/kernel/resource.c 2019-01-24 15:13:13.954199540 -0800 > @@ -375,7 +375,7 @@ static int __walk_iomem_res_desc(resourc > int (*func)(struct resource *, void *)) > { > struct resource res; > - int ret = -1; > + int ret = -EINVAL; > > while (start < end && > !find_next_iomem_res(start, end, flags, desc, first_lvl, &res)) { > @@ -453,7 +453,7 @@ int walk_system_ram_range(unsigned long > unsigned long flags; > struct resource res; > unsigned long pfn, end_pfn; > - int ret = -1; > + int ret = -EINVAL; Can you either make a similar change to the powerpc version of walk_system_ram_range() in arch/powerpc/mm/mem.c or explain why it's not needed? It *seems* like we'd want both versions of walk_system_ram_range() to behave similarly in this respect. > start = (u64) start_pfn << PAGE_SHIFT; > end = ((u64)(start_pfn + nr_pages) << PAGE_SHIFT) - 1; > _
On 1/25/19 1:02 PM, Bjorn Helgaas wrote: >> @@ -453,7 +453,7 @@ int walk_system_ram_range(unsigned long >> unsigned long flags; >> struct resource res; >> unsigned long pfn, end_pfn; >> - int ret = -1; >> + int ret = -EINVAL; > Can you either make a similar change to the powerpc version of > walk_system_ram_range() in arch/powerpc/mm/mem.c or explain why it's > not needed? It *seems* like we'd want both versions of > walk_system_ram_range() to behave similarly in this respect. Sure. A quick grep shows powerpc being the only other implementation. I'll just add this hunk: > diff -puN arch/powerpc/mm/mem.c~memory-hotplug-walk_system_ram_range-returns-neg-1 arch/powerpc/mm/mem.c > --- a/arch/powerpc/mm/mem.c~memory-hotplug-walk_system_ram_range-returns-neg-1 2019-01-25 12:57:00.000004446 -0800 > +++ b/arch/powerpc/mm/mem.c 2019-01-25 12:58:13.215004263 -0800 > @@ -188,7 +188,7 @@ walk_system_ram_range(unsigned long star > struct memblock_region *reg; > unsigned long end_pfn = start_pfn + nr_pages; > unsigned long tstart, tend; > - int ret = -1; > + int ret = -EINVAL; I'll also dust off the ol' cross-compiler and make sure I didn't fat-finger anything.
On Fri, Jan 25, 2019 at 3:10 PM Dave Hansen <dave.hansen@intel.com> wrote: > > On 1/25/19 1:02 PM, Bjorn Helgaas wrote: > >> @@ -453,7 +453,7 @@ int walk_system_ram_range(unsigned long > >> unsigned long flags; > >> struct resource res; > >> unsigned long pfn, end_pfn; > >> - int ret = -1; > >> + int ret = -EINVAL; > > Can you either make a similar change to the powerpc version of > > walk_system_ram_range() in arch/powerpc/mm/mem.c or explain why it's > > not needed? It *seems* like we'd want both versions of > > walk_system_ram_range() to behave similarly in this respect. > > Sure. A quick grep shows powerpc being the only other implementation. > I'll just add this hunk: > > > diff -puN arch/powerpc/mm/mem.c~memory-hotplug-walk_system_ram_range-returns-neg-1 arch/powerpc/mm/mem.c > > --- a/arch/powerpc/mm/mem.c~memory-hotplug-walk_system_ram_range-returns-neg-1 2019-01-25 12:57:00.000004446 -0800 > > +++ b/arch/powerpc/mm/mem.c 2019-01-25 12:58:13.215004263 -0800 > > @@ -188,7 +188,7 @@ walk_system_ram_range(unsigned long star > > struct memblock_region *reg; > > unsigned long end_pfn = start_pfn + nr_pages; > > unsigned long tstart, tend; > > - int ret = -1; > > + int ret = -EINVAL; > > I'll also dust off the ol' cross-compiler and make sure I didn't > fat-finger anything. Sounds good. Then add my Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
Dave Hansen <dave.hansen@intel.com> writes: > On 1/25/19 1:02 PM, Bjorn Helgaas wrote: >>> @@ -453,7 +453,7 @@ int walk_system_ram_range(unsigned long >>> unsigned long flags; >>> struct resource res; >>> unsigned long pfn, end_pfn; >>> - int ret = -1; >>> + int ret = -EINVAL; >> Can you either make a similar change to the powerpc version of >> walk_system_ram_range() in arch/powerpc/mm/mem.c or explain why it's >> not needed? It *seems* like we'd want both versions of >> walk_system_ram_range() to behave similarly in this respect. > > Sure. A quick grep shows powerpc being the only other implementation. Ugh gross, why are we reimplementing it? ... Oh right, memblock vs iomem. We should fix that one day :/ > I'll just add this hunk: > >> diff -puN arch/powerpc/mm/mem.c~memory-hotplug-walk_system_ram_range-returns-neg-1 arch/powerpc/mm/mem.c >> --- a/arch/powerpc/mm/mem.c~memory-hotplug-walk_system_ram_range-returns-neg-1 2019-01-25 12:57:00.000004446 -0800 >> +++ b/arch/powerpc/mm/mem.c 2019-01-25 12:58:13.215004263 -0800 >> @@ -188,7 +188,7 @@ walk_system_ram_range(unsigned long star >> struct memblock_region *reg; >> unsigned long end_pfn = start_pfn + nr_pages; >> unsigned long tstart, tend; >> - int ret = -1; >> + int ret = -EINVAL; > > I'll also dust off the ol' cross-compiler and make sure I didn't > fat-finger anything. Modern Fedora & Ubuntu have packaged cross toolchains. Otherwise there's the kernel.org ones, or bootlin has versions with libc if you need it. Patch looks fine. That value could only get to userspace if we have no memory, which would be interesting. Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc) cheers
diff -puN kernel/resource.c~memory-hotplug-walk_system_ram_range-returns-neg-1 kernel/resource.c --- a/kernel/resource.c~memory-hotplug-walk_system_ram_range-returns-neg-1 2019-01-24 15:13:13.950199540 -0800 +++ b/kernel/resource.c 2019-01-24 15:13:13.954199540 -0800 @@ -375,7 +375,7 @@ static int __walk_iomem_res_desc(resourc int (*func)(struct resource *, void *)) { struct resource res; - int ret = -1; + int ret = -EINVAL; while (start < end && !find_next_iomem_res(start, end, flags, desc, first_lvl, &res)) { @@ -453,7 +453,7 @@ int walk_system_ram_range(unsigned long unsigned long flags; struct resource res; unsigned long pfn, end_pfn; - int ret = -1; + int ret = -EINVAL; start = (u64) start_pfn << PAGE_SHIFT; end = ((u64)(start_pfn + nr_pages) << PAGE_SHIFT) - 1;