[PATCH 1/2] gpu: nova-core: check for overflow to DMATRFBASE1

[PATCH 1/2] gpu: nova-core: check for overflow to DMATRFBASE1

[Timur Tabi]

The NV_PFALCON_FALCON_DMATRFBASE/1 register pair supports DMA address
up to 49 bits only, but the write to DMATRFBASE1 could exceed that.
To mitigate, check first that the DMA address will fit.

Fixes: 69f5cd67ce41 ("gpu: nova-core: add falcon register definitions and base code")
Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 drivers/gpu/nova-core/falcon.rs | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
index 82c661aef594..fe5abf64dd2b 100644
--- a/drivers/gpu/nova-core/falcon.rs
+++ b/drivers/gpu/nova-core/falcon.rs
@@ -493,7 +493,11 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
             Some(_) => (),
         };
 
-        // Set up the base source DMA address.
+        // Set up the base source DMA address.  DMATRFBASE only supports a 49-bit address.
+        if dma_start > kernel::dma::DmaMask::new::<49>().value() {
+            dev_err!(self.dev, "DMA address {:#x} exceeds 49 bits\n", dma_start);
+            return Err(ERANGE);
+        }
 
         regs::NV_PFALCON_FALCON_DMATRFBASE::default()
             // CAST: `as u32` is used on purpose since we do want to strip the upper bits, which

base-commit: 4348796233e736147e2e79c58784d0a9fb48867d
-- 
2.52.0

[PATCH 2/2] gpu: nova-core: add missing newlines to several print strings

[Timur Tabi]

Although the dev_xx!() macro calls do not technically require terminating
newlines for the format strings, they should be added any way to maintain
consistency, both within Rust code and with the C versions.

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 drivers/gpu/nova-core/falcon.rs           | 6 +++---
 drivers/gpu/nova-core/falcon/hal/ga102.rs | 4 ++--
 drivers/gpu/nova-core/gpu.rs              | 2 +-
 drivers/gpu/nova-core/gsp/sequencer.rs    | 6 +++---
 drivers/gpu/nova-core/vbios.rs            | 2 +-
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
index fe5abf64dd2b..c1cb31c856b5 100644
--- a/drivers/gpu/nova-core/falcon.rs
+++ b/drivers/gpu/nova-core/falcon.rs
@@ -466,7 +466,7 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
         if dma_start % DmaAddress::from(DMA_LEN) > 0 {
             dev_err!(
                 self.dev,
-                "DMA transfer start addresses must be a multiple of {}",
+                "DMA transfer start addresses must be a multiple of {}\n",
                 DMA_LEN
             );
             return Err(EINVAL);
@@ -483,11 +483,11 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
             .and_then(|size| size.checked_add(load_offsets.src_start))
         {
             None => {
-                dev_err!(self.dev, "DMA transfer length overflow");
+                dev_err!(self.dev, "DMA transfer length overflow\n");
                 return Err(EOVERFLOW);
             }
             Some(upper_bound) if usize::from_safe_cast(upper_bound) > fw.size() => {
-                dev_err!(self.dev, "DMA transfer goes beyond range of DMA object");
+                dev_err!(self.dev, "DMA transfer goes beyond range of DMA object\n");
                 return Err(EINVAL);
             }
             Some(_) => (),
diff --git a/drivers/gpu/nova-core/falcon/hal/ga102.rs b/drivers/gpu/nova-core/falcon/hal/ga102.rs
index 69a7a95cac16..0bdfe45a2d03 100644
--- a/drivers/gpu/nova-core/falcon/hal/ga102.rs
+++ b/drivers/gpu/nova-core/falcon/hal/ga102.rs
@@ -52,7 +52,7 @@ fn signature_reg_fuse_version_ga102(
     let ucode_idx = match usize::from(ucode_id) {
         ucode_id @ 1..=regs::NV_FUSE_OPT_FPF_SIZE => ucode_id - 1,
         _ => {
-            dev_err!(dev, "invalid ucode id {:#x}", ucode_id);
+            dev_err!(dev, "invalid ucode id {:#x}\n", ucode_id);
             return Err(EINVAL);
         }
     };
@@ -66,7 +66,7 @@ fn signature_reg_fuse_version_ga102(
     } else if engine_id_mask & 0x0400 != 0 {
         regs::NV_FUSE_OPT_FPF_GSP_UCODE1_VERSION::read(bar, ucode_idx).data()
     } else {
-        dev_err!(dev, "unexpected engine_id_mask {:#x}", engine_id_mask);
+        dev_err!(dev, "unexpected engine_id_mask {:#x}\n", engine_id_mask);
         return Err(EINVAL);
     };
 
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 50d76092fbdd..9b042ef1a308 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -268,7 +268,7 @@ pub(crate) fn new<'a>(
             // We must wait for GFW_BOOT completion before doing any significant setup on the GPU.
             _: {
                 gfw::wait_gfw_boot_completion(bar)
-                    .inspect_err(|_| dev_err!(pdev.as_ref(), "GFW boot did not complete"))?;
+                    .inspect_err(|_| dev_err!(pdev.as_ref(), "GFW boot did not complete\n"))?;
             },
 
             sysmem_flush: SysmemFlush::register(pdev.as_ref(), bar, spec.chipset)?,
diff --git a/drivers/gpu/nova-core/gsp/sequencer.rs b/drivers/gpu/nova-core/gsp/sequencer.rs
index d78a30fbb70f..d965ffe9ba89 100644
--- a/drivers/gpu/nova-core/gsp/sequencer.rs
+++ b/drivers/gpu/nova-core/gsp/sequencer.rs
@@ -121,7 +121,7 @@ pub(crate) fn new(data: &[u8], dev: &device::Device) -> Result<(Self, usize)> {
         };
 
         if data.len() < size {
-            dev_err!(dev, "Data is not enough for command");
+            dev_err!(dev, "Data is not enough for command\n");
             return Err(EINVAL);
         }
 
@@ -320,7 +320,7 @@ fn next(&mut self) -> Option<Self::Item> {
 
         cmd_result.map_or_else(
             |_err| {
-                dev_err!(self.dev, "Error parsing command at offset {}", offset);
+                dev_err!(self.dev, "Error parsing command at offset {}\n", offset);
                 None
             },
             |(cmd, size)| {
@@ -382,7 +382,7 @@ pub(crate) fn run(cmdq: &mut Cmdq, params: GspSequencerParams<'a>) -> Result {
             dev: params.dev,
         };
 
-        dev_dbg!(sequencer.dev, "Running CPU Sequencer commands");
+        dev_dbg!(sequencer.dev, "Running CPU Sequencer commands\n");
 
         for cmd_result in sequencer.iter() {
             match cmd_result {
diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 7c26e4a2d61c..e4eae9385f47 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -790,7 +790,7 @@ fn falcon_data_ptr(&self) -> Result<u32> {
         // read the 4 bytes at the offset specified in the token
         let offset = usize::from(token.data_offset);
         let bytes: [u8; 4] = self.base.data[offset..offset + 4].try_into().map_err(|_| {
-            dev_err!(self.base.dev, "Failed to convert data slice to array");
+            dev_err!(self.base.dev, "Failed to convert data slice to array\n");
             EINVAL
         })?;
 
-- 
2.52.0

Re: [PATCH 1/2] gpu: nova-core: check for overflow to DMATRFBASE1

[John Hubbard]

On 1/6/26 5:24 PM, Timur Tabi wrote:
> The NV_PFALCON_FALCON_DMATRFBASE/1 register pair supports DMA address
> up to 49 bits only, but the write to DMATRFBASE1 could exceed that.
> To mitigate, check first that the DMA address will fit.
> 
> Fixes: 69f5cd67ce41 ("gpu: nova-core: add falcon register definitions and base code")
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> ---
>  drivers/gpu/nova-core/falcon.rs | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> index 82c661aef594..fe5abf64dd2b 100644
> --- a/drivers/gpu/nova-core/falcon.rs
> +++ b/drivers/gpu/nova-core/falcon.rs
> @@ -493,7 +493,11 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
>              Some(_) => (),
>          };
>  
> -        // Set up the base source DMA address.
> +        // Set up the base source DMA address.  DMATRFBASE only supports a 49-bit address.

Your commit log said it accurately, so I think we should use similar wording
here. So maybe:

   // Set up the base source DMA address.  The DMATRFBASE/1 register pair
   // only supports a 49-bit address.

However, one could argue that only the DMATRFBASE base matters
when considering overflow. But even so, it's slightly confusing
at first glance, to refer to a 32-bit register as only supporting
49-bit addresses. Of course yes, they can look at the register
which is shifted up, but still.

Either way is fine, I'm just fussing over nearly nothing here,
so:

Reviewed-by: John Hubbard <jhubbard@nvidia.com>


thanks,
-- 
John Hubbard

> +        if dma_start > kernel::dma::DmaMask::new::<49>().value() {
> +            dev_err!(self.dev, "DMA address {:#x} exceeds 49 bits\n", dma_start);
> +            return Err(ERANGE);
> +        }
>  
>          regs::NV_PFALCON_FALCON_DMATRFBASE::default()
>              // CAST: `as u32` is used on purpose since we do want to strip the upper bits, which
> 
> base-commit: 4348796233e736147e2e79c58784d0a9fb48867d

Re: [PATCH 1/2] gpu: nova-core: check for overflow to DMATRFBASE1

[Timur Tabi]

On Tue, 2026-01-06 at 17:56 -0800, John Hubbard wrote:
> Your commit log said it accurately, so I think we should use similar wording
> here. So maybe:
> 
>    // Set up the base source DMA address.  The DMATRFBASE/1 register pair
>    // only supports a 49-bit address.

Yes, this is better.

Re: [PATCH 2/2] gpu: nova-core: add missing newlines to several print strings

[John Hubbard]

On 1/6/26 5:24 PM, Timur Tabi wrote:
> Although the dev_xx!() macro calls do not technically require terminating
> newlines for the format strings, they should be added any way to maintain
> consistency, both within Rust code and with the C versions.

I'd *tentatively* prefer to *omit* the \n's, because they are 
stripped out (and then re-added) as part of the printing
call stack, and so it seems nice to have less dead code on
the screen.

On the other hand, the output always gets a newline, so seeing
it on screen is actually informative in a way.

And yes consistency is important. But which way to head
toward? I checked the Rust for Linux code in drm-rust-next,
and we have:

Codebase                      WITH \n    WITHOUT \n    % with \n
----------------------------------------------------------------
samples/rust (dev_*!)              30            10          75%
samples/rust (pr_*!)               20             7          74%
nova-core (dev_*!) [after patch]   39            32          55%
nova-core (dev_*!) [before patch]  32            39          45%
nova (dev_*!)                       0             0           --
tyr (dev_*!)                        3             5          38%
pwm (dev_*!)                        3             6          33%
binder (pr_*!)                     17            62          22%

So there is not yet a clear convention visible in the code.

Miguel, Alice, Gary et al, is there actually a preference already?


thanks,
-- 
John Hubbard


> 
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> ---
>  drivers/gpu/nova-core/falcon.rs           | 6 +++---
>  drivers/gpu/nova-core/falcon/hal/ga102.rs | 4 ++--
>  drivers/gpu/nova-core/gpu.rs              | 2 +-
>  drivers/gpu/nova-core/gsp/sequencer.rs    | 6 +++---
>  drivers/gpu/nova-core/vbios.rs            | 2 +-
>  5 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> index fe5abf64dd2b..c1cb31c856b5 100644
> --- a/drivers/gpu/nova-core/falcon.rs
> +++ b/drivers/gpu/nova-core/falcon.rs
> @@ -466,7 +466,7 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
>          if dma_start % DmaAddress::from(DMA_LEN) > 0 {
>              dev_err!(
>                  self.dev,
> -                "DMA transfer start addresses must be a multiple of {}",
> +                "DMA transfer start addresses must be a multiple of {}\n",
>                  DMA_LEN
>              );
>              return Err(EINVAL);
> @@ -483,11 +483,11 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
>              .and_then(|size| size.checked_add(load_offsets.src_start))
>          {
>              None => {
> -                dev_err!(self.dev, "DMA transfer length overflow");
> +                dev_err!(self.dev, "DMA transfer length overflow\n");
>                  return Err(EOVERFLOW);
>              }
>              Some(upper_bound) if usize::from_safe_cast(upper_bound) > fw.size() => {
> -                dev_err!(self.dev, "DMA transfer goes beyond range of DMA object");
> +                dev_err!(self.dev, "DMA transfer goes beyond range of DMA object\n");
>                  return Err(EINVAL);
>              }
>              Some(_) => (),
> diff --git a/drivers/gpu/nova-core/falcon/hal/ga102.rs b/drivers/gpu/nova-core/falcon/hal/ga102.rs
> index 69a7a95cac16..0bdfe45a2d03 100644
> --- a/drivers/gpu/nova-core/falcon/hal/ga102.rs
> +++ b/drivers/gpu/nova-core/falcon/hal/ga102.rs
> @@ -52,7 +52,7 @@ fn signature_reg_fuse_version_ga102(
>      let ucode_idx = match usize::from(ucode_id) {
>          ucode_id @ 1..=regs::NV_FUSE_OPT_FPF_SIZE => ucode_id - 1,
>          _ => {
> -            dev_err!(dev, "invalid ucode id {:#x}", ucode_id);
> +            dev_err!(dev, "invalid ucode id {:#x}\n", ucode_id);
>              return Err(EINVAL);
>          }
>      };
> @@ -66,7 +66,7 @@ fn signature_reg_fuse_version_ga102(
>      } else if engine_id_mask & 0x0400 != 0 {
>          regs::NV_FUSE_OPT_FPF_GSP_UCODE1_VERSION::read(bar, ucode_idx).data()
>      } else {
> -        dev_err!(dev, "unexpected engine_id_mask {:#x}", engine_id_mask);
> +        dev_err!(dev, "unexpected engine_id_mask {:#x}\n", engine_id_mask);
>          return Err(EINVAL);
>      };
>  
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index 50d76092fbdd..9b042ef1a308 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -268,7 +268,7 @@ pub(crate) fn new<'a>(
>              // We must wait for GFW_BOOT completion before doing any significant setup on the GPU.
>              _: {
>                  gfw::wait_gfw_boot_completion(bar)
> -                    .inspect_err(|_| dev_err!(pdev.as_ref(), "GFW boot did not complete"))?;
> +                    .inspect_err(|_| dev_err!(pdev.as_ref(), "GFW boot did not complete\n"))?;
>              },
>  
>              sysmem_flush: SysmemFlush::register(pdev.as_ref(), bar, spec.chipset)?,
> diff --git a/drivers/gpu/nova-core/gsp/sequencer.rs b/drivers/gpu/nova-core/gsp/sequencer.rs
> index d78a30fbb70f..d965ffe9ba89 100644
> --- a/drivers/gpu/nova-core/gsp/sequencer.rs
> +++ b/drivers/gpu/nova-core/gsp/sequencer.rs
> @@ -121,7 +121,7 @@ pub(crate) fn new(data: &[u8], dev: &device::Device) -> Result<(Self, usize)> {
>          };
>  
>          if data.len() < size {
> -            dev_err!(dev, "Data is not enough for command");
> +            dev_err!(dev, "Data is not enough for command\n");
>              return Err(EINVAL);
>          }
>  
> @@ -320,7 +320,7 @@ fn next(&mut self) -> Option<Self::Item> {
>  
>          cmd_result.map_or_else(
>              |_err| {
> -                dev_err!(self.dev, "Error parsing command at offset {}", offset);
> +                dev_err!(self.dev, "Error parsing command at offset {}\n", offset);
>                  None
>              },
>              |(cmd, size)| {
> @@ -382,7 +382,7 @@ pub(crate) fn run(cmdq: &mut Cmdq, params: GspSequencerParams<'a>) -> Result {
>              dev: params.dev,
>          };
>  
> -        dev_dbg!(sequencer.dev, "Running CPU Sequencer commands");
> +        dev_dbg!(sequencer.dev, "Running CPU Sequencer commands\n");
>  
>          for cmd_result in sequencer.iter() {
>              match cmd_result {
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 7c26e4a2d61c..e4eae9385f47 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -790,7 +790,7 @@ fn falcon_data_ptr(&self) -> Result<u32> {
>          // read the 4 bytes at the offset specified in the token
>          let offset = usize::from(token.data_offset);
>          let bytes: [u8; 4] = self.base.data[offset..offset + 4].try_into().map_err(|_| {
> -            dev_err!(self.base.dev, "Failed to convert data slice to array");
> +            dev_err!(self.base.dev, "Failed to convert data slice to array\n");
>              EINVAL
>          })?;
>  

Re: [PATCH 2/2] gpu: nova-core: add missing newlines to several print strings

[Timur Tabi]

On Tue, 2026-01-06 at 18:10 -0800, John Hubbard wrote:
> nova-core (dev_*!) [after patch]   39            32          55%
> nova-core (dev_*!) [before patch]  32            39          45%

This can't be right, because I checked thoroughly to make sure that my patch fixed ALL dev_xx
format strings.

Re: [PATCH 2/2] gpu: nova-core: add missing newlines to several print strings

[John Hubbard]

On 1/6/26 6:21 PM, Timur Tabi wrote:
> On Tue, 2026-01-06 at 18:10 -0800, John Hubbard wrote:
>> nova-core (dev_*!) [after patch]   39            32          55%
>> nova-core (dev_*!) [before patch]  32            39          45%
> 
> This can't be right, because I checked thoroughly to make sure that my patch fixed ALL dev_xx
> format strings.

Did you? haha :)

drivers/gpu/nova-core/fb.rs:81-85

                dev_warn!(
                    &self.device,
                    "failed to unregister sysmem flush page: {:?}",
                    e
                )

drivers/gpu/nova-core/gsp/boot.rs:83-87

            dev_err!(
                dev,
                "FWSEC-FRTS returned with error code {:#x}",
                frts_status
            );

drivers/gpu/nova-core/gsp/cmdq.rs:616-620

            dev_err!(
                self.dev,
                "GSP RPC: receive: Call {} - bad checksum",
                header.sequence()
            );

drivers/gpu/nova-core/gsp/sequencer.rs:391-395

                    dev_err!(
                        sequencer.dev,
                        "Error running command at index {}",
                        sequencer.seq_info.cmd_index
                    );

drivers/gpu/nova-core/gsp/sequencer.rs:401-404

        dev_dbg!(
            sequencer.dev,
            "CPU Sequencer commands completed successfully"
        );

However, you *are* correct about the "this can't be right". I had a bug
in my search script. Updated numbers do actually lean much more
toward using the \n on screen:

Codebase                      WITH \n    WITHOUT \n    % with \n
----------------------------------------------------------------
samples/rust (dev_*!)              39             1          98%
samples/rust (pr_*!)               29             6          83%
nova-core (dev_*!)                 68             5          93%
nova (dev_*!)                       0             0           --
tyr (dev_*!)                        4             3          57%
pwm (dev_*!)                        7             2          78%
binder (pr_*!)                     22            57          28%



thanks,
-- 
John Hubbard

Re: [PATCH 1/2] gpu: nova-core: check for overflow to DMATRFBASE1

[Joel Fernandes]

> On Jan 6, 2026, at 8:24 PM, Timur Tabi <ttabi@nvidia.com> wrote:
> 
> The NV_PFALCON_FALCON_DMATRFBASE/1 register pair supports DMA address
> up to 49 bits only, but the write to DMATRFBASE1 could exceed that.
> To mitigate, check first that the DMA address will fit.
> 
> Fixes: 69f5cd67ce41 ("gpu: nova-core: add falcon register definitions and base code")
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

Thanks.



> ---
> drivers/gpu/nova-core/falcon.rs | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> index 82c661aef594..fe5abf64dd2b 100644
> --- a/drivers/gpu/nova-core/falcon.rs
> +++ b/drivers/gpu/nova-core/falcon.rs
> @@ -493,7 +493,11 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
>             Some(_) => (),
>         };
> 
> -        // Set up the base source DMA address.
> +        // Set up the base source DMA address.  DMATRFBASE only supports a 49-bit address.
> +        if dma_start > kernel::dma::DmaMask::new::<49>().value() {
> +            dev_err!(self.dev, "DMA address {:#x} exceeds 49 bits\n", dma_start);
> +            return Err(ERANGE);
> +        }
> 
>         regs::NV_PFALCON_FALCON_DMATRFBASE::default()
>             // CAST: `as u32` is used on purpose since we do want to strip the upper bits, which
> 
> base-commit: 4348796233e736147e2e79c58784d0a9fb48867d
> --
> 2.52.0
> 

Re: [PATCH 2/2] gpu: nova-core: add missing newlines to several print strings

[Miguel Ojeda]

On Wed, Jan 7, 2026 at 3:10 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> So there is not yet a clear convention visible in the code.
>
> Miguel, Alice, Gary et al, is there actually a preference already?

They are supposed to have the `\n`.

For a bit of context: when we discussed the printing a long time ago,
I originally had:

    info!("foo");

instead of:

    pr_info!("foo\n");

i.e. both the prefix and the newline were assumed, but we were asked
to keep it close to the C side, which is fair.

Cheers,
Miguel

Re: [PATCH 2/2] gpu: nova-core: add missing newlines to several print strings

[Miguel Ojeda]

On Wed, Jan 7, 2026 at 3:39 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> samples/rust (pr_*!)               29             6          83%

2 of these 6 are expected, 4 aren't (`cont` level follows).

I have had a good first issue for these:

    https://github.com/Rust-for-Linux/linux/issues/1139

I just added a few more pointers to see if it helps people
contributing, and also a link to your table since it is a recent
search.

I also linked our other issue about having a `checkpatch` warning for
this, since most (but not all) should be finished in `\n`:

    https://github.com/Rust-for-Linux/linux/issues/1140

and the patch that would be nice to review/test/revive to see if we
can manage to land it:

    https://lore.kernel.org/rust-for-linux/20250207-checkpatch-newline2-v4-1-26d8e80d0059@invicto.ai/

(Or we do something like our past ksquirrel bot which is probably just
easier, to be honest)

Cheers,
Miguel

Re: [PATCH 2/2] gpu: nova-core: add missing newlines to several print strings

[Gary Guo]

On Wed, 7 Jan 2026 10:54:42 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Wed, Jan 7, 2026 at 3:10 AM John Hubbard <jhubbard@nvidia.com> wrote:
> >
> > So there is not yet a clear convention visible in the code.
> >
> > Miguel, Alice, Gary et al, is there actually a preference already?  
> 
> They are supposed to have the `\n`.
> 
> For a bit of context: when we discussed the printing a long time ago,
> I originally had:
> 
>     info!("foo");
> 
> instead of:
> 
>     pr_info!("foo\n");
> 
> i.e. both the prefix and the newline were assumed, but we were asked
> to keep it close to the C side, which is fair.

In very early days (before RfL is upstreamed), I had a prototype print
macro that is designed like this:

	info!("foo"); // pr_info("foo\n");
	info!(target: dev, "foo"); // dev_info(dev, "foo\n");
	info!(target: dev, once, "foo"); // dev_info_once(dev, "foo\n");
	info!(target: dev, ratelimited, "foo"); // dev_info_ratelimited(dev, "foo\n");

There's a trait that is implemented for anything that can be used as a
printing target.

I still think this is superior than just having our macro mimicking the C
side (and as a result, having a lot of macros rather than just one for
each level).

I think with Rust printing machinary, `pr_cont` is rarely useful, instead
of calling `pr_info` followed by multiple `pr_cont`, you can just have a
custom `Display` implementation and print it in one go. This is also free
from racing against another print and have your lines broken into two
parts.

I would be much in favour of vouching deleteing `pr_cont` entirely from
Rust codebase and always have newlines automatically added.

Best,
Gary

Re: [PATCH 2/2] gpu: nova-core: add missing newlines to several print strings

[Danilo Krummrich]

(Cc: Greg, Rafael)

On Thu Jan 8, 2026 at 1:55 PM CET, Gary Guo wrote:
> In very early days (before RfL is upstreamed), I had a prototype print
> macro that is designed like this:
>
> 	info!("foo"); // pr_info("foo\n");
> 	info!(target: dev, "foo"); // dev_info(dev, "foo\n");
> 	info!(target: dev, once, "foo"); // dev_info_once(dev, "foo\n");
> 	info!(target: dev, ratelimited, "foo"); // dev_info_ratelimited(dev, "foo\n");
>
> There's a trait that is implemented for anything that can be used as a
> printing target.
>
> I still think this is superior than just having our macro mimicking the C
> side (and as a result, having a lot of macros rather than just one for
> each level).

Why do you think this syntax is superior?

One disadvantage for maintainers and reviewers would be that it is less
convinient to grep for pr_*() vs dev_*(), which is something that people
regularly get wrong. I.e. it regularly happens that people use pr_*() where they
actually print in the context of a specific device.

> I think with Rust printing machinary, `pr_cont` is rarely useful, instead
> of calling `pr_info` followed by multiple `pr_cont`, you can just have a
> custom `Display` implementation and print it in one go. This is also free
> from racing against another print and have your lines broken into two
> parts.

This I fully agree with.

> I would be much in favour of vouching deleteing `pr_cont` entirely from
> Rust codebase and always have newlines automatically added.

I don't think it is a good idea to always add newlines. It might be fine if you
only do C code or only do Rust code, but if you are switching back and forth, it
is a horrible inconsistency for muscle memory.

I'm pretty sure that this would turn into a generator for trivial fixup patches
either removing or adding the trailing '\n'. :)

- Danilo

Re: [PATCH 2/2] gpu: nova-core: add missing newlines to several print strings

[Alice Ryhl]

On Thu, Jan 8, 2026 at 2:36 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> (Cc: Greg, Rafael)
>
> On Thu Jan 8, 2026 at 1:55 PM CET, Gary Guo wrote:
> > In very early days (before RfL is upstreamed), I had a prototype print
> > macro that is designed like this:
> >
> >       info!("foo"); // pr_info("foo\n");
> >       info!(target: dev, "foo"); // dev_info(dev, "foo\n");
> >       info!(target: dev, once, "foo"); // dev_info_once(dev, "foo\n");
> >       info!(target: dev, ratelimited, "foo"); // dev_info_ratelimited(dev, "foo\n");
> >
> > There's a trait that is implemented for anything that can be used as a
> > printing target.
> >
> > I still think this is superior than just having our macro mimicking the C
> > side (and as a result, having a lot of macros rather than just one for
> > each level).
>
> Why do you think this syntax is superior?
>
> One disadvantage for maintainers and reviewers would be that it is less
> convinient to grep for pr_*() vs dev_*(), which is something that people
> regularly get wrong. I.e. it regularly happens that people use pr_*() where they
> actually print in the context of a specific device.
>
> > I think with Rust printing machinary, `pr_cont` is rarely useful, instead
> > of calling `pr_info` followed by multiple `pr_cont`, you can just have a
> > custom `Display` implementation and print it in one go. This is also free
> > from racing against another print and have your lines broken into two
> > parts.
>
> This I fully agree with.
>
> > I would be much in favour of vouching deleteing `pr_cont` entirely from
> > Rust codebase and always have newlines automatically added.
>
> I don't think it is a good idea to always add newlines. It might be fine if you
> only do C code or only do Rust code, but if you are switching back and forth, it
> is a horrible inconsistency for muscle memory.
>
> I'm pretty sure that this would turn into a generator for trivial fixup patches
> either removing or adding the trailing '\n'. :)

What about triggering a build failure if you forget the newline?

For cases where the \n is intentionally omitted for use with pr_cont!,
we can add separate syntax for making this explicit:

pr_info!("hello! thi", cont);
pr_cont!("s tring is split\n");

This way, you can't forget the newline, but you can still omit it if
you really want to use pr_cont.

Alice

Re: [PATCH 2/2] gpu: nova-core: add missing newlines to several print strings

[Gary Guo]

On Thu, 08 Jan 2026 14:36:21 +0100
"Danilo Krummrich" <dakr@kernel.org> wrote:

> (Cc: Greg, Rafael)
> 
> On Thu Jan 8, 2026 at 1:55 PM CET, Gary Guo wrote:
> > In very early days (before RfL is upstreamed), I had a prototype print
> > macro that is designed like this:
> >
> > 	info!("foo"); // pr_info("foo\n");
> > 	info!(target: dev, "foo"); // dev_info(dev, "foo\n");
> > 	info!(target: dev, once, "foo"); // dev_info_once(dev, "foo\n");
> > 	info!(target: dev, ratelimited, "foo"); // dev_info_ratelimited(dev, "foo\n");
> >
> > There's a trait that is implemented for anything that can be used as a
> > printing target.
> >
> > I still think this is superior than just having our macro mimicking the C
> > side (and as a result, having a lot of macros rather than just one for
> > each level).  
> 
> Why do you think this syntax is superior?
> 
> One disadvantage for maintainers and reviewers would be that it is less
> convinient to grep for pr_*() vs dev_*(), which is something that people
> regularly get wrong. I.e. it regularly happens that people use pr_*() where they
> actually print in the context of a specific device.
> 

One solution is perhaps to always ask people to specify a device so they
won't mix things up?

If they want to print without device then they can refer to `()` or
perhaps another marker type `info!(NoDevice, "...")`.

> > I think with Rust printing machinary, `pr_cont` is rarely useful, instead
> > of calling `pr_info` followed by multiple `pr_cont`, you can just have a
> > custom `Display` implementation and print it in one go. This is also free
> > from racing against another print and have your lines broken into two
> > parts.  
> 
> This I fully agree with.
> 
> > I would be much in favour of vouching deleteing `pr_cont` entirely from
> > Rust codebase and always have newlines automatically added.  
> 
> I don't think it is a good idea to always add newlines. It might be fine if you
> only do C code or only do Rust code, but if you are switching back and forth, it
> is a horrible inconsistency for muscle memory.
> 
> I'm pretty sure that this would turn into a generator for trivial fixup patches
> either removing or adding the trailing '\n'. :)

Well, we've seen that people will forget either way. :)

More seriously, printk actually strips out your newline. The new line at
the end of print gets turned into a flag on whether that log record is
continuable.

If we say "pr_cont" is not used in Rust, then it does not make sense to
leave any log entries continuable -- I would imagine in the future we
might even want to bypass all va-args machinary and just push the record
to the printk ringbuffer. Adding a newline just to have it stripped
doesn't sound ideal.

Best,
Gary