On Wed, Jan 31, 2024 at 12:48:38PM +0100, Arnd Bergmann wrote: > On Wed, Jan 31, 2024, at 06:51, Yan Zhao wrote: > > This is a tiny fix to pfn_to_virt() for some platforms. > > > > The original implementaion of pfn_to_virt() takes PFN instead of PA as the > > input to macro __va, with PAGE_SHIFT applying to the converted VA, which > > is not right under most conditions, especially when there's an offset in > > __va. > > > > > > Yan Zhao (4): > > asm-generic/page.h: apply page shift to PFN instead of VA in > > pfn_to_virt > > csky: apply page shift to PFN instead of VA in pfn_to_virt > > Hexagon: apply page shift to PFN instead of VA in pfn_to_virt > > openrisc: apply page shift to PFN instead of VA in pfn_to_virt > > Nice catch, this is clearly a correct fix, and I can take > the series through the asm-generic tree if we want to take > this approach. > > I made a couple of interesting observations looking at this patch > though: > > - this function is only used in architecture specific code on > m68k, riscv and s390, though a couple of other architectures > have the same definition. > > - There is another function that does the same thing called > pfn_to_kaddr(), which is defined on arm, arm64, csky, > loongarch, mips, nios2, powerpc, s390, sh, sparc and x86, > as well as yet another pfn_va() on parisc. > > - the asm-generic/page.h file used to be included by h8300, c6x > and blackfin, all of which are now gone. It has no users left > and can just as well get removed, unless we find a new use > for it. > > Since it looks like the four broken functions you fix > don't have a single caller, maybe it would be better to > just remove them all? > > How exactly did you notice the function being wrong, > did you try to add a user somewhere, or just read through > the code? I came across them when I was debugging an unexpected kernel page fault on x86, and I was not sure whether page_to_virt() was compiled in asm-generic/page.h or linux/mm.h. Though finally, it turned out that the one in linux/mm.h was used, which yielded the right result and the unexpected kernel page fault in my case was not related to page_to_virt(), it did lead me to noticing that the pfn_to_virt() in asm-generic/page.h and other 3 archs did not look right. Yes, unlike virt_to_pfn() which still has a caller in openrisc (among csky, Hexagon, openrisc), pfn_to_virt() now does not have a caller in the 3 archs. Though both virt_to_pfn() and pfn_to_virt() are referenced in asm-generic/page.h, I also not sure if we need to remove the asm-generic/page.h which may serve as a template to future archs ? So, either way looks good to me :)
