diff mbox series

[-next] dma-debug: Use %pa to format phys_addr_t

Message ID 20230329071405.1568021-1-geert+renesas@glider.be
State New
Headers
Series [-next] dma-debug: Use %pa to format phys_addr_t |

Commit Message

Geert Uytterhoeven March 29, 2023, 7:14 a.m. UTC 
  On 32-bit without LPAE:

    kernel/dma/debug.c: In function ‘debug_dma_dump_mappings’:
    kernel/dma/debug.c:537:7: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 9 has type ‘phys_addr_t’ {aka ‘unsigned int’} [-Wformat=]
    kernel/dma/debug.c: In function ‘dump_show’:
    kernel/dma/debug.c:568:59: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 11 has type ‘phys_addr_t’ {aka ‘unsigned int’} [-Wformat=]

Fixes: bd89d69a529fbef3 ("dma-debug: add cacheline to user/kernel space dump messages")
Reported-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/r/202303160548.ReyuTsGD-lkp@intel.com
Reported-by: noreply@ellerman.id.au
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 kernel/dma/debug.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Jerry Snitselaar March 29, 2023, 3:03 p.m. UTC | #1 
On Wed, Mar 29, 2023 at 09:14:05AM +0200, Geert Uytterhoeven wrote:
> On 32-bit without LPAE:
> 
>     kernel/dma/debug.c: In function ‘debug_dma_dump_mappings’:
>     kernel/dma/debug.c:537:7: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 9 has type ‘phys_addr_t’ {aka ‘unsigned int’} [-Wformat=]
>     kernel/dma/debug.c: In function ‘dump_show’:
>     kernel/dma/debug.c:568:59: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 11 has type ‘phys_addr_t’ {aka ‘unsigned int’} [-Wformat=]
> 
> Fixes: bd89d69a529fbef3 ("dma-debug: add cacheline to user/kernel space dump messages")
> Reported-by: kernel test robot <lkp@intel.com>
> Link: https://lore.kernel.org/r/202303160548.ReyuTsGD-lkp@intel.com
> Reported-by: noreply@ellerman.id.au
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  kernel/dma/debug.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
> index 676142072d997b6a..f190651bcaddc9e9 100644
> --- a/kernel/dma/debug.c
> +++ b/kernel/dma/debug.c
> @@ -534,11 +534,11 @@ void debug_dma_dump_mappings(struct device *dev)
>  			if (!dev || dev == entry->dev) {
>  				cln = to_cacheline_number(entry);
>  				dev_info(entry->dev,
> -					 "%s idx %d P=%llx N=%lx D=%llx L=%llx cln=%llx %s %s\n",
> +					 "%s idx %d P=%llx N=%lx D=%llx L=%llx cln=%pa %s %s\n",
>  					 type2name[entry->type], idx,
>  					 phys_addr(entry), entry->pfn,
>  					 entry->dev_addr, entry->size,
> -					 cln, dir2name[entry->direction],
> +					 &cln, dir2name[entry->direction],

Won't this just print out the address of cln declared at the beginning of debug_dma_dump_mappings() each time, instead of the value
returned from to_cacheline_entry() ?

>  					 maperr2str[entry->map_err_type]);
>  			}
>  		}
> @@ -565,13 +565,13 @@ static int dump_show(struct seq_file *seq, void *v)
>  		list_for_each_entry(entry, &bucket->list, list) {
>  			cln = to_cacheline_number(entry);
>  			seq_printf(seq,
> -				   "%s %s %s idx %d P=%llx N=%lx D=%llx L=%llx cln=%llx %s %s\n",
> +				   "%s %s %s idx %d P=%llx N=%lx D=%llx L=%llx cln=%pa %s %s\n",
>  				   dev_driver_string(entry->dev),
>  				   dev_name(entry->dev),
>  				   type2name[entry->type], idx,
>  				   phys_addr(entry), entry->pfn,
>  				   entry->dev_addr, entry->size,
> -				   cln, dir2name[entry->direction],
> +				   &cln, dir2name[entry->direction],

same for dump_show()

>  				   maperr2str[entry->map_err_type]);
>  		}
>  		spin_unlock_irqrestore(&bucket->lock, flags);
> -- 
> 2.34.1
>
Geert Uytterhoeven March 29, 2023, 3:34 p.m. UTC | #2 
Hi Jerry,

On Wed, Mar 29, 2023 at 5:03 PM Jerry Snitselaar <jsnitsel@redhat.com> wrote:
> On Wed, Mar 29, 2023 at 09:14:05AM +0200, Geert Uytterhoeven wrote:
> > On 32-bit without LPAE:
> >
> >     kernel/dma/debug.c: In function ‘debug_dma_dump_mappings’:
> >     kernel/dma/debug.c:537:7: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 9 has type ‘phys_addr_t’ {aka ‘unsigned int’} [-Wformat=]
> >     kernel/dma/debug.c: In function ‘dump_show’:
> >     kernel/dma/debug.c:568:59: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 11 has type ‘phys_addr_t’ {aka ‘unsigned int’} [-Wformat=]
> >
> > Fixes: bd89d69a529fbef3 ("dma-debug: add cacheline to user/kernel space dump messages")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Link: https://lore.kernel.org/r/202303160548.ReyuTsGD-lkp@intel.com
> > Reported-by: noreply@ellerman.id.au
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
=
> > --- a/kernel/dma/debug.c
> > +++ b/kernel/dma/debug.c
> > @@ -534,11 +534,11 @@ void debug_dma_dump_mappings(struct device *dev)
> >                       if (!dev || dev == entry->dev) {
> >                               cln = to_cacheline_number(entry);
> >                               dev_info(entry->dev,
> > -                                      "%s idx %d P=%llx N=%lx D=%llx L=%llx cln=%llx %s %s\n",
> > +                                      "%s idx %d P=%llx N=%lx D=%llx L=%llx cln=%pa %s %s\n",
> >                                        type2name[entry->type], idx,
> >                                        phys_addr(entry), entry->pfn,
> >                                        entry->dev_addr, entry->size,
> > -                                      cln, dir2name[entry->direction],
> > +                                      &cln, dir2name[entry->direction],
>
> Won't this just print out the address of cln declared at the beginning of debug_dma_dump_mappings() each time, instead of the value
> returned from to_cacheline_entry() ?

The physical address is passed by reference, cfr.
https://elixir.bootlin.com/linux/latest/source/Documentation/core-api/printk-formats.rst#L231

Gr{oetje,eeting}s,

                        Geert
Jerry Snitselaar March 29, 2023, 3:38 p.m. UTC | #3 
On Wed, Mar 29, 2023 at 05:34:23PM +0200, Geert Uytterhoeven wrote:
> Hi Jerry,
> 
> On Wed, Mar 29, 2023 at 5:03 PM Jerry Snitselaar <jsnitsel@redhat.com> wrote:
> > On Wed, Mar 29, 2023 at 09:14:05AM +0200, Geert Uytterhoeven wrote:
> > > On 32-bit without LPAE:
> > >
> > >     kernel/dma/debug.c: In function ‘debug_dma_dump_mappings’:
> > >     kernel/dma/debug.c:537:7: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 9 has type ‘phys_addr_t’ {aka ‘unsigned int’} [-Wformat=]
> > >     kernel/dma/debug.c: In function ‘dump_show’:
> > >     kernel/dma/debug.c:568:59: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 11 has type ‘phys_addr_t’ {aka ‘unsigned int’} [-Wformat=]
> > >
> > > Fixes: bd89d69a529fbef3 ("dma-debug: add cacheline to user/kernel space dump messages")
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Link: https://lore.kernel.org/r/202303160548.ReyuTsGD-lkp@intel.com
> > > Reported-by: noreply@ellerman.id.au
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> =
> > > --- a/kernel/dma/debug.c
> > > +++ b/kernel/dma/debug.c
> > > @@ -534,11 +534,11 @@ void debug_dma_dump_mappings(struct device *dev)
> > >                       if (!dev || dev == entry->dev) {
> > >                               cln = to_cacheline_number(entry);
> > >                               dev_info(entry->dev,
> > > -                                      "%s idx %d P=%llx N=%lx D=%llx L=%llx cln=%llx %s %s\n",
> > > +                                      "%s idx %d P=%llx N=%lx D=%llx L=%llx cln=%pa %s %s\n",
> > >                                        type2name[entry->type], idx,
> > >                                        phys_addr(entry), entry->pfn,
> > >                                        entry->dev_addr, entry->size,
> > > -                                      cln, dir2name[entry->direction],
> > > +                                      &cln, dir2name[entry->direction],
> >
> > Won't this just print out the address of cln declared at the beginning of debug_dma_dump_mappings() each time, instead of the value
> > returned from to_cacheline_entry() ?
> 
> The physical address is passed by reference, cfr.
> https://elixir.bootlin.com/linux/latest/source/Documentation/core-api/printk-formats.rst#L231
> 
> Gr{oetje,eeting}s,
> 
>                         Geert

Got it. Thanks Geert

snits

> 
> -- 
> 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
Christoph Hellwig March 29, 2023, 11:50 p.m. UTC | #4 
I've applied this as it is the first fix I got into my mailbox for it.

Arnd sent one a little later, which uses %pap instead of %pa, which
confused me a bit.  The documentation seems to allow both without
any recommendation which one to use, which confuses me even further.
Arnd Bergmann March 30, 2023, 6:51 a.m. UTC | #5 
On Thu, Mar 30, 2023, at 01:50, Christoph Hellwig wrote:
> I've applied this as it is the first fix I got into my mailbox for it.
>
> Arnd sent one a little later, which uses %pap instead of %pa, which
> confused me a bit.  The documentation seems to allow both without
> any recommendation which one to use, which confuses me even further.

Indeed, I wasn't aware of this either, apparently aaf07621b8bb
("vsprintf: add %pad extension for dma_addr_t use") introduced the
%pap form at the same time as %pad but made it the same as the already
existing %pa. I also see that the %pa form is a lot more common than
%pap, so it's probably better to go with %pa for new users anyway.

    Arnd
Geert Uytterhoeven March 30, 2023, 7:07 a.m. UTC | #6 
Hi Christoph,

On Thu, Mar 30, 2023 at 1:50 AM Christoph Hellwig <hch@infradead.org> wrote:
> I've applied this as it is the first fix I got into my mailbox for it.

Thank you!

> Arnd sent one a little later, which uses %pap instead of %pa, which
> confused me a bit.  The documentation seems to allow both without
> any recommendation which one to use, which confuses me even further.

The "%pa" class supports two variants: "%pad" (for dma_addr_t) and
"%pap" (for phys_addr_t). "%pa" is a shortcut for the latter, cfr.
https://elixir.bootlin.com/linux/latest/source/lib/vsprintf.c#L1792

The default exists because "%pa" was fist, cfr. commit aaf07621b8bbfdc0
("vsprintf: add %pad extension for dma_addr_t use"), and I don't think
we can easily remove it. The default is shorter (saves one byte, even
compared to %llx"), so why not use it?

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
index 676142072d997b6a..f190651bcaddc9e9 100644
--- a/kernel/dma/debug.c
+++ b/kernel/dma/debug.c
@@ -534,11 +534,11 @@  void debug_dma_dump_mappings(struct device *dev)
 			if (!dev || dev == entry->dev) {
 				cln = to_cacheline_number(entry);
 				dev_info(entry->dev,
-					 "%s idx %d P=%llx N=%lx D=%llx L=%llx cln=%llx %s %s\n",
+					 "%s idx %d P=%llx N=%lx D=%llx L=%llx cln=%pa %s %s\n",
 					 type2name[entry->type], idx,
 					 phys_addr(entry), entry->pfn,
 					 entry->dev_addr, entry->size,
-					 cln, dir2name[entry->direction],
+					 &cln, dir2name[entry->direction],
 					 maperr2str[entry->map_err_type]);
 			}
 		}
@@ -565,13 +565,13 @@  static int dump_show(struct seq_file *seq, void *v)
 		list_for_each_entry(entry, &bucket->list, list) {
 			cln = to_cacheline_number(entry);
 			seq_printf(seq,
-				   "%s %s %s idx %d P=%llx N=%lx D=%llx L=%llx cln=%llx %s %s\n",
+				   "%s %s %s idx %d P=%llx N=%lx D=%llx L=%llx cln=%pa %s %s\n",
 				   dev_driver_string(entry->dev),
 				   dev_name(entry->dev),
 				   type2name[entry->type], idx,
 				   phys_addr(entry), entry->pfn,
 				   entry->dev_addr, entry->size,
-				   cln, dir2name[entry->direction],
+				   &cln, dir2name[entry->direction],
 				   maperr2str[entry->map_err_type]);
 		}
 		spin_unlock_irqrestore(&bucket->lock, flags);