mm/early_ioremap: Print the starting physical address in __early_ioremap()

[PATCH] mm/early_ioremap: Print the starting physical address in __early_ioremap()

Posted by Hou Wenlong 1 month ago

The debug WARN() printing occurs after the while loop, so the
'phys_addr' reflects the last physical address rather than the actual
starting physical address, which is not useful for debugging. To
simplify, the WARN() statement could be moved up before the loop instead
of introducing a new variable to record the original 'phys_addr' value.
Additionally, swap the print order of 'slot_virt[slot]' and 'offset', as
this will enhance output readability.

Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
---
 mm/early_ioremap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/early_ioremap.c b/mm/early_ioremap.c
index ff35b84a7b50..3fdde074c9da 100644
--- a/mm/early_ioremap.c
+++ b/mm/early_ioremap.c
@@ -139,6 +139,9 @@ __early_ioremap(resource_size_t phys_addr, unsigned long size, pgprot_t prot)
 	if (WARN_ON(nrpages > NR_FIX_BTMAPS))
 		return NULL;
 
+	WARN(early_ioremap_debug, "%s(%pa, %08lx) [%d] => %08lx + %08lx\n",
+	     __func__, &phys_addr, size, slot, slot_virt[slot], offset);
+
 	/*
 	 * Ok, go for it..
 	 */
@@ -152,8 +155,6 @@ __early_ioremap(resource_size_t phys_addr, unsigned long size, pgprot_t prot)
 		--idx;
 		--nrpages;
 	}
-	WARN(early_ioremap_debug, "%s(%pa, %08lx) [%d] => %08lx + %08lx\n",
-	     __func__, &phys_addr, size, slot, offset, slot_virt[slot]);
 
 	prev_map[slot] = (void __iomem *)(offset + slot_virt[slot]);
 	return prev_map[slot];
-- 
2.31.1

Re: [PATCH] mm/early_ioremap: Print the starting physical address in __early_ioremap()

Posted by Mike Rapoport 4 weeks ago

On Fri, Jan 09, 2026 at 09:31:51PM +0800, Hou Wenlong wrote:
> The debug WARN() printing occurs after the while loop, so the
> 'phys_addr' reflects the last physical address rather than the actual
> starting physical address, which is not useful for debugging. To
> simplify, the WARN() statement could be moved up before the loop instead
> of introducing a new variable to record the original 'phys_addr' value.
> Additionally, swap the print order of 'slot_virt[slot]' and 'offset', as
> this will enhance output readability.
> 
> Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>

Reviewed-by: Mike Rapoport (Microsoft) <rppt@kernel.org>

> ---
>  mm/early_ioremap.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/early_ioremap.c b/mm/early_ioremap.c
> index ff35b84a7b50..3fdde074c9da 100644
> --- a/mm/early_ioremap.c
> +++ b/mm/early_ioremap.c
> @@ -139,6 +139,9 @@ __early_ioremap(resource_size_t phys_addr, unsigned long size, pgprot_t prot)
>  	if (WARN_ON(nrpages > NR_FIX_BTMAPS))
>  		return NULL;
>  
> +	WARN(early_ioremap_debug, "%s(%pa, %08lx) [%d] => %08lx + %08lx\n",
> +	     __func__, &phys_addr, size, slot, slot_virt[slot], offset);
> +
>  	/*
>  	 * Ok, go for it..
>  	 */
> @@ -152,8 +155,6 @@ __early_ioremap(resource_size_t phys_addr, unsigned long size, pgprot_t prot)
>  		--idx;
>  		--nrpages;
>  	}
> -	WARN(early_ioremap_debug, "%s(%pa, %08lx) [%d] => %08lx + %08lx\n",
> -	     __func__, &phys_addr, size, slot, offset, slot_virt[slot]);
>  
>  	prev_map[slot] = (void __iomem *)(offset + slot_virt[slot]);
>  	return prev_map[slot];
> -- 
> 2.31.1
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH] mm/early_ioremap: Print the starting physical address in __early_ioremap()

Posted by Andrew Morton 4 weeks ago

On Fri,  9 Jan 2026 21:31:51 +0800 Hou Wenlong <houwenlong.hwl@antgroup.com> wrote:

> The debug WARN() printing occurs after the while loop, so the
> 'phys_addr' reflects the last physical address rather than the actual
> starting physical address, which is not useful for debugging. To
> simplify, the WARN() statement could be moved up before the loop instead
> of introducing a new variable to record the original 'phys_addr' value.
> Additionally, swap the print order of 'slot_virt[slot]' and 'offset', as
> this will enhance output readability.
> 

yep, thanks.

> --- a/mm/early_ioremap.c
> +++ b/mm/early_ioremap.c
> @@ -139,6 +139,9 @@ __early_ioremap(resource_size_t phys_addr, unsigned long size, pgprot_t prot)
>  	if (WARN_ON(nrpages > NR_FIX_BTMAPS))
>  		return NULL;
>  
> +	WARN(early_ioremap_debug, "%s(%pa, %08lx) [%d] => %08lx + %08lx\n",
> +	     __func__, &phys_addr, size, slot, slot_virt[slot], offset);
> +
>  	/*
>  	 * Ok, go for it..
>  	 */
> @@ -152,8 +155,6 @@ __early_ioremap(resource_size_t phys_addr, unsigned long size, pgprot_t prot)
>  		--idx;
>  		--nrpages;
>  	}
> -	WARN(early_ioremap_debug, "%s(%pa, %08lx) [%d] => %08lx + %08lx\n",
> -	     __func__, &phys_addr, size, slot, offset, slot_virt[slot]);

It's strange that this code uses WARN when nothing is wrong.  The rest
of the function uses WARN appropriately.  But that's off-topic!

Reviewed-by: Andrew Morton <akpm@linux-foundation.org>

Re: [PATCH] mm/early_ioremap: Print the starting physical address in __early_ioremap()

Posted by Mike Rapoport 4 weeks ago

On Sat, Jan 10, 2026 at 03:57:21PM -0800, Andrew Morton wrote:
> On Fri,  9 Jan 2026 21:31:51 +0800 Hou Wenlong <houwenlong.hwl@antgroup.com> wrote:
> 
> > The debug WARN() printing occurs after the while loop, so the
> > 'phys_addr' reflects the last physical address rather than the actual
> > starting physical address, which is not useful for debugging. To
> > simplify, the WARN() statement could be moved up before the loop instead
> > of introducing a new variable to record the original 'phys_addr' value.
> > Additionally, swap the print order of 'slot_virt[slot]' and 'offset', as
> > this will enhance output readability.
> > 
> 
> yep, thanks.
> 
> > --- a/mm/early_ioremap.c
> > +++ b/mm/early_ioremap.c
> > @@ -139,6 +139,9 @@ __early_ioremap(resource_size_t phys_addr, unsigned long size, pgprot_t prot)
> >  	if (WARN_ON(nrpages > NR_FIX_BTMAPS))
> >  		return NULL;
> >  
> > +	WARN(early_ioremap_debug, "%s(%pa, %08lx) [%d] => %08lx + %08lx\n",
> > +	     __func__, &phys_addr, size, slot, slot_virt[slot], offset);
> > +
> >  	/*
> >  	 * Ok, go for it..
> >  	 */
> > @@ -152,8 +155,6 @@ __early_ioremap(resource_size_t phys_addr, unsigned long size, pgprot_t prot)
> >  		--idx;
> >  		--nrpages;
> >  	}
> > -	WARN(early_ioremap_debug, "%s(%pa, %08lx) [%d] => %08lx + %08lx\n",
> > -	     __func__, &phys_addr, size, slot, offset, slot_virt[slot]);
> 
> It's strange that this code uses WARN when nothing is wrong.  The rest
> of the function uses WARN appropriately.  But that's off-topic!

It seems an historic thing. A nice cleanup would be to replace
WARN(early_ioremap_debug) with pr_warn() + dump_stack().
 
> Reviewed-by: Andrew Morton <akpm@linux-foundation.org>
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH] mm/early_ioremap: Print the starting physical address in __early_ioremap()

Posted by Hou Wenlong 3 weeks, 6 days ago

On Sun, Jan 11, 2026 at 11:15:45AM +0200, Mike Rapoport wrote:
> On Sat, Jan 10, 2026 at 03:57:21PM -0800, Andrew Morton wrote:
> > On Fri,  9 Jan 2026 21:31:51 +0800 Hou Wenlong <houwenlong.hwl@antgroup.com> wrote:
> > 
> > > The debug WARN() printing occurs after the while loop, so the
> > > 'phys_addr' reflects the last physical address rather than the actual
> > > starting physical address, which is not useful for debugging. To
> > > simplify, the WARN() statement could be moved up before the loop instead
> > > of introducing a new variable to record the original 'phys_addr' value.
> > > Additionally, swap the print order of 'slot_virt[slot]' and 'offset', as
> > > this will enhance output readability.
> > > 
> > 
> > yep, thanks.
> > 
> > > --- a/mm/early_ioremap.c
> > > +++ b/mm/early_ioremap.c
> > > @@ -139,6 +139,9 @@ __early_ioremap(resource_size_t phys_addr, unsigned long size, pgprot_t prot)
> > >  	if (WARN_ON(nrpages > NR_FIX_BTMAPS))
> > >  		return NULL;
> > >  
> > > +	WARN(early_ioremap_debug, "%s(%pa, %08lx) [%d] => %08lx + %08lx\n",
> > > +	     __func__, &phys_addr, size, slot, slot_virt[slot], offset);
> > > +
> > >  	/*
> > >  	 * Ok, go for it..
> > >  	 */
> > > @@ -152,8 +155,6 @@ __early_ioremap(resource_size_t phys_addr, unsigned long size, pgprot_t prot)
> > >  		--idx;
> > >  		--nrpages;
> > >  	}
> > > -	WARN(early_ioremap_debug, "%s(%pa, %08lx) [%d] => %08lx + %08lx\n",
> > > -	     __func__, &phys_addr, size, slot, offset, slot_virt[slot]);
> > 
> > It's strange that this code uses WARN when nothing is wrong.  The rest
> > of the function uses WARN appropriately.  But that's off-topic!
> 
> It seems an historic thing. A nice cleanup would be to replace
> WARN(early_ioremap_debug) with pr_warn() + dump_stack().
>
Yes, I see that the initial code uses WARN(), so I didn't change it, as
cleanup is a separate concern and may not be well-received. However, I
can make the change as you suggested.

Thanks!

> > Reviewed-by: Andrew Morton <akpm@linux-foundation.org>
> > 
> 
> -- 
> Sincerely yours,
> Mike.