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
Apply the page shift to PFN to get physical address for final VA.
The macro __va should take physical address instead of PFN as input.
Fixes: 2d78057f0dd4 ("asm-generic/page.h: Make pfn accessors static inlines")
Signed-off-by: Yan Zhao <[email protected]>
---
include/asm-generic/page.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/asm-generic/page.h b/include/asm-generic/page.h
index 9773582fd96e..4f1265207b9a 100644
--- a/include/asm-generic/page.h
+++ b/include/asm-generic/page.h
@@ -81,7 +81,7 @@ static inline unsigned long virt_to_pfn(const void *kaddr)
#define virt_to_pfn virt_to_pfn
static inline void *pfn_to_virt(unsigned long pfn)
{
- return __va(pfn) << PAGE_SHIFT;
+ return __va(pfn << PAGE_SHIFT);
}
#define pfn_to_virt pfn_to_virt
--
2.17.1
Apply the page shift to PFN to get physical address for final VA.
The macro __va should take physical address instead of PFN as input.
Fixes: c1884e1e1164 ("csky: Make pfn accessors static inlines")
Signed-off-by: Yan Zhao <[email protected]>
---
arch/csky/include/asm/page.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/csky/include/asm/page.h b/arch/csky/include/asm/page.h
index 4a0502e324a6..2c4cc7825a7b 100644
--- a/arch/csky/include/asm/page.h
+++ b/arch/csky/include/asm/page.h
@@ -84,7 +84,7 @@ static inline unsigned long virt_to_pfn(const void *kaddr)
static inline void * pfn_to_virt(unsigned long pfn)
{
- return (void *)((unsigned long)__va(pfn) << PAGE_SHIFT);
+ return __va(pfn << PAGE_SHIFT);
}
#define MAP_NR(x) PFN_DOWN((unsigned long)(x) - PAGE_OFFSET - \
--
2.17.1
Apply the page shift to PFN to get physical address for final VA.
The macro __va should take physical address instead of PFN as input.
Fixes: 232ba1630c66 ("openrisc: Make pfn accessors statics inlines")
Signed-off-by: Yan Zhao <[email protected]>
---
arch/openrisc/include/asm/page.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/openrisc/include/asm/page.h b/arch/openrisc/include/asm/page.h
index 44fc1fd56717..55c66f6cb1bd 100644
--- a/arch/openrisc/include/asm/page.h
+++ b/arch/openrisc/include/asm/page.h
@@ -79,7 +79,7 @@ static inline unsigned long virt_to_pfn(const void *kaddr)
static inline void * pfn_to_virt(unsigned long pfn)
{
- return (void *)((unsigned long)__va(pfn) << PAGE_SHIFT);
+ return __va(pfn << PAGE_SHIFT);
}
#define virt_to_page(addr) \
--
2.17.1
Apply the page shift to PFN to get physical address for final VA.
The macro __va should take physical address instead of PFN as input.
Fixes: d6e81532b10d ("Hexagon: Make pfn accessors statics inlines")
Signed-off-by: Yan Zhao <[email protected]>
---
arch/hexagon/include/asm/page.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/hexagon/include/asm/page.h b/arch/hexagon/include/asm/page.h
index 10f1bc07423c..1d341590791a 100644
--- a/arch/hexagon/include/asm/page.h
+++ b/arch/hexagon/include/asm/page.h
@@ -135,7 +135,7 @@ static inline unsigned long virt_to_pfn(const void *kaddr)
static inline void *pfn_to_virt(unsigned long pfn)
{
- return (void *)((unsigned long)__va(pfn) << PAGE_SHIFT);
+ return __va(pfn << PAGE_SHIFT);
}
--
2.17.1
On Wed, Jan 31, 2024 at 7:25 AM Yan Zhao <[email protected]> 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 <[email protected]>
Arnd: I think you can take most of them through the arch tree.
Yours,
Linus Walleij
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
Hi Arnd,
On Thu, Feb 1, 2024 at 11:11 AM Arnd Bergmann <[email protected]> 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-m68korg
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 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 Thu, Feb 1, 2024, at 11:46, Geert Uytterhoeven wrote:
> Hi Arnd,
>
> On Thu, Feb 1, 2024 at 11:11 AM Arnd Bergmann <[email protected]> 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?
Yes. We've already removed half of the nommu architectures
(blackfin, avr32, h8300, m32r, microblaze-nommu) over
the past couple of years, and the remaining ones are pretty
much only there to support existing users.
The only platform one that I see getting real work is
esp32 [1], but that is not a new architecture.
Arnd
[1] https://github.com/jcmvbkbc/linux-xtensa/tree/xtensa-6.8-rc2-esp32
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/[email protected]
Thanks for you guidance :)
On Wed, Jan 31, 2024 at 2:28 PM Yan Zhao <[email protected]> wrote:
>
> Apply the page shift to PFN to get physical address for final VA.
> The macro __va should take physical address instead of PFN as input.
>
> Fixes: c1884e1e1164 ("csky: Make pfn accessors static inlines")
> Signed-off-by: Yan Zhao <[email protected]>
> ---
> arch/csky/include/asm/page.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/csky/include/asm/page.h b/arch/csky/include/asm/page.h
> index 4a0502e324a6..2c4cc7825a7b 100644
> --- a/arch/csky/include/asm/page.h
> +++ b/arch/csky/include/asm/page.h
> @@ -84,7 +84,7 @@ static inline unsigned long virt_to_pfn(const void *kaddr)
>
> static inline void * pfn_to_virt(unsigned long pfn)
> {
> - return (void *)((unsigned long)__va(pfn) << PAGE_SHIFT);
> + return __va(pfn << PAGE_SHIFT);
> }
Reviewed-by: Guo Ren <[email protected]>
>
> #define MAP_NR(x) PFN_DOWN((unsigned long)(x) - PAGE_OFFSET - \
> --
> 2.17.1
>
--
Best Regards
Guo Ren
On Wed, Jan 31, 2024 at 2:27 PM Yan Zhao <[email protected]> wrote:
>
> Apply the page shift to PFN to get physical address for final VA.
> The macro __va should take physical address instead of PFN as input.
>
> Fixes: 2d78057f0dd4 ("asm-generic/page.h: Make pfn accessors static inlines")
> Signed-off-by: Yan Zhao <[email protected]>
> ---
> include/asm-generic/page.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/asm-generic/page.h b/include/asm-generic/page.h
> index 9773582fd96e..4f1265207b9a 100644
> --- a/include/asm-generic/page.h
> +++ b/include/asm-generic/page.h
> @@ -81,7 +81,7 @@ static inline unsigned long virt_to_pfn(const void *kaddr)
> #define virt_to_pfn virt_to_pfn
> static inline void *pfn_to_virt(unsigned long pfn)
> {
> - return __va(pfn) << PAGE_SHIFT;
> + return __va(pfn << PAGE_SHIFT);
Oh, that's a terrible bug; Thx for fixing it.
Reviewed-by: Guo Ren <[email protected]>
> }
> #define pfn_to_virt pfn_to_virt
>
> --
> 2.17.1
>
--
Best Regards
Guo Ren
On Wed, Jan 31, 2024 at 7:27 AM Yan Zhao <[email protected]> wrote:
> Apply the page shift to PFN to get physical address for final VA.
> The macro __va should take physical address instead of PFN as input.
>
> Fixes: 2d78057f0dd4 ("asm-generic/page.h: Make pfn accessors static inlines")
> Signed-off-by: Yan Zhao <[email protected]>
My bug, obviously. :(
Reviewed-by: Linus Walleij <[email protected]>
I thought this was already applied with the other fixes, but maybe it
was missed?
Yours,
Linus Walleij
On Thu, Feb 29, 2024 at 02:34:35PM +0100, Linus Walleij wrote:
> On Wed, Jan 31, 2024 at 7:27 AM Yan Zhao <[email protected]> wrote:
>
> > Apply the page shift to PFN to get physical address for final VA.
> > The macro __va should take physical address instead of PFN as input.
> >
> > Fixes: 2d78057f0dd4 ("asm-generic/page.h: Make pfn accessors static inlines")
> > Signed-off-by: Yan Zhao <[email protected]>
>
> My bug, obviously. :(
> Reviewed-by: Linus Walleij <[email protected]>
>
> I thought this was already applied with the other fixes, but maybe it
> was missed?
>
Hi Linus,
The other 3 in csky/hexagon/openrisc should have been applied in
https://lore.kernel.org/all/[email protected]/.
This one in asm-generic is not, because Arnd said he is going to remove
the header as a whole soon.
I explained it in the change log :)
"The pfn_to_virt() in asm-generic/page.h is not touched in this patch as
it's referenced by page_to_virt() in that header while the whole header is
going to be removed as a whole due to no one including it."
Thanks
Yan