Messages in this thread Patch in this message | | | From | Thomas Gleixner <> | Subject | Re: [PATCH v2 1/2] mm: fix the pfn calculation mistake in __ioremap_check_ram | Date | Sat, 19 Jun 2021 23:22:08 +0200 |
| |
Yaohui!
On Fri, Jun 11 2021 at 12:21, Yaohui Wang wrote:
A few formal things upfront. The prefix of the subject is incorrect. It should be "x86/ioremap:" git log $FILE helps to figure that out.
Looking at the Signed-off-by chain below this misses either a
From: Ben Luo <luoben@linux.alibaba.com>
right at the top of the changelog or a Co-Developed-by tag. See Documentation/process/
> In arch/x86/mm/ioremap.c:__ioremap_check_ram, the original pfn > wrapping
Just "In __ioremap_check_ram() ..." please. The file name is uninteresting and we want the '()' at the end of the symbol so it's obvious that this is a function.
> calculation may cause the pfn range to ignore the very start page, if > res->start is not page-aligned, or the very end page, if res->end is not > page aligned. > > So start_pfn should wrap down the res->start address, and end_pfn should > wrap up the res->end address. This makes the pfn range completely > contain [res->start, res->end] ram range. This check is more strict and is > more reasonable.
This lacks a "Fixes:" tag
> Signed-off-by: Ben Luo <luoben@linux.alibaba.com> > Signed-off-by: Yahui Wang <yaohuiwang@linux.alibaba.com> > --- > arch/x86/mm/ioremap.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c > index 9e5ccc56f..79adf0d2d 100644 > --- a/arch/x86/mm/ioremap.c > +++ b/arch/x86/mm/ioremap.c > @@ -74,8 +74,8 @@ static unsigned int __ioremap_check_ram(struct resource *res) > if ((res->flags & IORESOURCE_SYSTEM_RAM) != IORESOURCE_SYSTEM_RAM) > return 0; > > - start_pfn = (res->start + PAGE_SIZE - 1) >> PAGE_SHIFT; > - stop_pfn = (res->end + 1) >> PAGE_SHIFT; > + start_pfn = res->start >> PAGE_SHIFT; > + stop_pfn = (res->end + PAGE_SIZE) >> PAGE_SHIFT;
Please make that:
start_pfn = PFN_DOWN(res->start); stop_pfn = PFN_UP(res->end);
which gives you the first and the last PFN of that range. That obviously requires to fix the below as well, but that code is unreadable anyway.
> if (stop_pfn > start_pfn) { > for (i = 0; i < (stop_pfn - start_pfn); ++i) > if (pfn_valid(start_pfn + i) &&
npages = stop_pfn - start_pfn + 1; for (i = 0; i < npages; i++) { if (.....) } you get the idea, right?
Thanks,
tglx
| |