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

 arch/csky/include/asm/page.h     | 2 +-
 arch/hexagon/include/asm/page.h  | 2 +-
 arch/openrisc/include/asm/page.h | 2 +-
 include/asm-generic/page.h       | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)


base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
-- 
2.17.1

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?

    Arnd

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 :)

On Thu, Feb 1, 2024, at 01:01, Yan Zhao wrote:
> On Wed, Jan 31, 2024 at 12:48:38PM +0100, Arnd Bergmann wrote:
>> On Wed, Jan 31, 2024, at 06:51, Yan Zhao wrote:
>> 
>> 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 :)

I think it's fair to assume we won't need asm-generic/page.h any
more, as we likely won't be adding new NOMMU architectures.
I can have a look myself at removing any such unused headers in
include/asm-generic/, it's probably not the only one.

Can you just send a patch to remove the unused pfn_to_virt()
functions?

     Arnd

On Thu, Feb 01, 2024 at 06:46:46AM +0100, Arnd Bergmann wrote:
> On Thu, Feb 1, 2024, at 01:01, Yan Zhao wrote:
> > On Wed, Jan 31, 2024 at 12:48:38PM +0100, Arnd Bergmann wrote:
> >> On Wed, Jan 31, 2024, at 06:51, Yan Zhao wrote:
> >> 
> >> 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 :)
> 
> I think it's fair to assume we won't need asm-generic/page.h any
> more, as we likely won't be adding new NOMMU architectures.
> I can have a look myself at removing any such unused headers in
> include/asm-generic/, it's probably not the only one.
> 
> Can you just send a patch to remove the unused pfn_to_virt()
> functions?
Ok. I'll do it!
BTW: do you think it's also good to keep this fixing series though we'll
remove the unused function later?
So if people want to revert the removal some day, they can get right one.

Or maybe I'm just paranoid, and explanation about the fix in the commit
message of patch for function removal is enough.

What's your preference? :)

On Fri, Feb 2, 2024, at 02:02, Yan Zhao wrote:
> On Thu, Feb 01, 2024 at 06:46:46AM +0100, Arnd Bergmann wrote:
>> 
>> I think it's fair to assume we won't need asm-generic/page.h any
>> more, as we likely won't be adding new NOMMU architectures.
>> I can have a look myself at removing any such unused headers in
>> include/asm-generic/, it's probably not the only one.
>> 
>> Can you just send a patch to remove the unused pfn_to_virt()
>> functions?
> Ok. I'll do it!
> BTW: do you think it's also good to keep this fixing series though we'll
> remove the unused function later?
> So if people want to revert the removal some day, they can get right one.
>
> Or maybe I'm just paranoid, and explanation about the fix in the commit
> message of patch for function removal is enough.
>
> What's your preference? :)

I would just remove it, there is no point in having both
pfn_to_kaddr() and pfn_to_virt() when they do the exact
same thing aside from this bug.

Just do a single patch for all architectures, no need to
have three or four identical ones when I'm going to merge
them all through the same tree anyway.

Just make sure you explain in the changelog what the bug was
and how you noticed it, in case anyone is ever tempted to
bring the function back.

    Arnd

On Fri, Feb 02, 2024 at 08:04:34AM +0100, Arnd Bergmann wrote:
> On Fri, Feb 2, 2024, at 02:02, Yan Zhao wrote:
> > On Thu, Feb 01, 2024 at 06:46:46AM +0100, Arnd Bergmann wrote:
> >> 
> >> I think it's fair to assume we won't need asm-generic/page.h any
> >> more, as we likely won't be adding new NOMMU architectures.
> >> I can have a look myself at removing any such unused headers in
> >> include/asm-generic/, it's probably not the only one.
> >> 
> >> Can you just send a patch to remove the unused pfn_to_virt()
> >> functions?
> > Ok. I'll do it!
> > BTW: do you think it's also good to keep this fixing series though we'll
> > remove the unused function later?
> > So if people want to revert the removal some day, they can get right one.
> >
> > Or maybe I'm just paranoid, and explanation about the fix in the commit
> > message of patch for function removal is enough.
> >
> > What's your preference? :)
> 
> I would just remove it, there is no point in having both
> pfn_to_kaddr() and pfn_to_virt() when they do the exact
> same thing aside from this bug.
>
> Just do a single patch for all architectures, no need to
> have three or four identical ones when I'm going to merge
> them all through the same tree anyway.
> 
> Just make sure you explain in the changelog what the bug was
> and how you noticed it, in case anyone is ever tempted to
> bring the function back.
Done.
https://lore.kernel.org/all/20240202140550.9886-1-yan.y.zhao@intel.com
Thanks for you guidance :)

Hi Arnd,

On Thu, Feb 1, 2024 at 11:11 AM Arnd Bergmann <arnd@arndb.de> wrote:
> I think it's fair to assume we won't need asm-generic/page.h any
> more, as we likely won't be adding new NOMMU architectures.

So you think riscv-nommu (k210) was the last one we will ever see?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

On Wed, Jan 31, 2024 at 7:25 AM Yan Zhao <yan.y.zhao@intel.com> 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.

Ooops that's right, I wonder why I got it wrong.
Arithmetic made it not regress :/
Thank you so much for fixing this Yan!

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Arnd: I think you can take most of them through the arch tree.

Yours,
Linus Walleij