Received-SPF: pass (google.com: domain of linux-kernel+bounces-46379-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-144-ge5821d614e-fm-20240125.002-ge5821d61
Precedence: bulk
MIME-Version: 1.0
Message-Id: <5e55b5c0-6c8d-45b4-ac04-cf694bcb08d3@app.fastmail.com>
In-Reply-To: <20240131055159.2506-1-yan.y.zhao@intel.com>
References: <20240131055159.2506-1-yan.y.zhao@intel.com>
Date: Wed, 31 Jan 2024 12:48:38 +0100
From: "Arnd Bergmann" <arnd@arndb.de>
To: "Yan Zhao" <yan.y.zhao@intel.com>,
 "Linus Walleij" <linus.walleij@linaro.org>, guoren <guoren@kernel.org>,
 "Brian Cain" <bcain@quicinc.com>, "Jonas Bonn" <jonas@southpole.se>,
 "Stefan Kristiansson" <stefan.kristiansson@saunalahti.fi>,
 "Stafford Horne" <shorne@gmail.com>
Cc: Linux-Arch <linux-arch@vger.kernel.org>, linux-kernel@vger.kernel.org,
 "linux-csky@vger.kernel.org" <linux-csky@vger.kernel.org>,
 linux-hexagon@vger.kernel.org,
 "linux-openrisc@vger.kernel.org" <linux-openrisc@vger.kernel.org>
Subject: Re: [PATCH 0/4] apply page shift to PFN instead of VA in pfn_to_virt
Content-Type: text/plain

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