Received-SPF: pass (google.com: domain of linux-kernel+bounces-47435-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 1 Feb 2024 08:01:53 +0800
From: Yan Zhao <yan.y.zhao@intel.com>
To: Arnd Bergmann <arnd@arndb.de>
CC: 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>, 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
Message-ID: <ZbrfcTaiuu2gaa2A@yzhao56-desk.sh.intel.com>
Reply-To: Yan Zhao <yan.y.zhao@intel.com>
References: <20240131055159.2506-1-yan.y.zhao@intel.com>
 <5e55b5c0-6c8d-45b4-ac04-cf694bcb08d3@app.fastmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <5e55b5c0-6c8d-45b4-ac04-cf694bcb08d3@app.fastmail.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?u2hySYtTMDKlnZSDlUimw7tvm3zF4xJfOMGAaCCj9rr2IquDiZGACLhwJ1Sa?=
 =?us-ascii?Q?TaOJvq4NWFb2x5Px2/gmAYqrShR1/hv1MBCpNHlblhmfmnhvt4hB4qLKUWGL?=
 =?us-ascii?Q?qM0BQ3M0DWcz+0PNSnPcZFgfd+BZyDS9m/NLWVntJdEUXa3LDosRT3hADLgK?=
 =?us-ascii?Q?4DL8VP5mWjnJVTM7WFRtsKg/YJf4ppNBtv5inAHP2Hs5iC7RsmOcUd+SPkfM?=
 =?us-ascii?Q?eK/k6pC5Jxvd32MofULhTS4Y5t+Wme63n40sMbgfddBqNapz6zAfkxIRExjo?=
 =?us-ascii?Q?Pk/04y2vaZqTDTSvbq2MSdlj8JdHv7+XAl/lXVTbumJBt/znK3jL/sdTiocn?=
 =?us-ascii?Q?EJFVnr5uPHhDvAAv9iLAncyiGBs+2Q5txt0ksmx2+J5IIYDz5PC/c4bCVRAS?=
 =?us-ascii?Q?1u+T6RsdG7trr9xglcxbkjnmE5D8PpD8lO7zZhPvW9rqmpZC3ee4i1+cr93H?=
 =?us-ascii?Q?rCbj+uSvfnaJGirBnsgXK9wEklWQqE/qnHWAZalrb7blrQsIwzQWWolrPpk/?=
 =?us-ascii?Q?3dRzMXJsd+dacSSxwUcZ8bU/Cse0CPJyHXycThM3d2trb+BB1cZcK9ROpbd7?=
 =?us-ascii?Q?RGM2StqLv+Vdtvddb5WK10dbToGdQEeeutaGlQQrHlhpp4cAIiG+gbgEr+9d?=
 =?us-ascii?Q?SCRipo7Jb2WJnN9aV+4h15z1LDRW0lB4/ofz9SaffNXzZmcFjdRcWmA37pjX?=
 =?us-ascii?Q?9TzuZrXHzNas7CEVhGZmSKb+zcbf7zsJZfgfIIb3rS7pJBsFZk4/3ZXx3olV?=
 =?us-ascii?Q?nsVncow2iZWspHZMtXqr7AQKKsYQtoZHGPo+n+iilObYu+KL02g3Mm1neDwF?=
 =?us-ascii?Q?U59hdZIs8HgVknS6RWExvJAUkVu8UG1Lho7MsZarIN+RtSrIR4IMQp9xIoG5?=
 =?us-ascii?Q?B2JPgE3/4ykmt0ByxctmHJCGIjTGTqOqGpcybFx/Lw0BtvxIO1cKAJE0X1Of?=
 =?us-ascii?Q?25NlsTSVc5ZXkppgeW8IXSjYWSDbRT/pHb//6by+rJj/xQdre1b6o2m+qHZH?=
 =?us-ascii?Q?FEwe5OK7A22pQHmICfL3cHiBIZPs7zT6DfAvkNTwFv9WDwCuYTxzj+6iPLSt?=
 =?us-ascii?Q?oMWpBrQapGpCrCG1wHQOMMwqBIrcVv3SuYVcEfKT4B9+MSp+YXEoS7hcd5uu?=
 =?us-ascii?Q?Z+sg6kPaD0TCmuj5s8tpc9Kugs0Zo96otBK7uTbrcz+2Gd/SzV2yAFj76Hx/?=
 =?us-ascii?Q?ZH9lHuhLyLzwOhd7LXklFb4AVsjI6q8331C8QeXoX5e9sq0t0gw+OaSXjJ5v?=
 =?us-ascii?Q?R74DpWEGPphauENAjxNbi9QfubtKDlDXvYRpT3LxKkkVbW42iLdyhzUnch8K?=
 =?us-ascii?Q?RXVv8bHzkctSvsqomxVRdVVOzh4oGKybLarsQq+NbiULO9vsZ+zfc2NmXh+f?=
 =?us-ascii?Q?oZ0hT8WIDUuwAYJU4HJ8o1Yd7uHshQCBZgCljwQUDHy91cVLhZ2KAIeaQ2r3?=
 =?us-ascii?Q?6BGVMG3ScSqZnZN84yGREn/PbW1c9CDyrQn5cfMNdjv/2aze3BYkV9EYM81D?=
 =?us-ascii?Q?4gXQ+uYaaGeCm0iIRp7w0kNEmitNgD7A8puHa9nYcI3k2Ws9VDhGwvB+8imO?=
 =?us-ascii?Q?l5bFX2KywyNatS7coP6ckr2YTXf5Cb0bTwMoRXsw?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3ec47e45-646f-45d8-516c-08dc22bd1f60
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Feb 2024 00:31:25.2214
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: EkDRDpDbcXSuFoy16QBXQBM+2XF/Pot6/0//oG+tUTAth7eNqMO32mG+s+W8/HEtXyeEIbI/HUfE0n3+H/orHA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB4645
X-OriginatorOrg: intel.com

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