[PATCH] rust: Return Option from page_align and ensure no usize overflow

[PATCH] rust: Return Option from page_align and ensure no usize overflow

[Brendan Shephard]

Changed page_align() to return Option<usize> which allows for validation
of the provided addr value. This ensures that any value that is provided
within one PAGE_SIZE of usize::MAX will not panic and instead page_align
will return None.

Callers of page_align() should raise a EINVAL when they receive None
from page_align().

Signed-off-by: Brendan Shephard <bshephar@bne-home.net>
---
 rust/kernel/page.rs | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
index 432fc0297d4a..b78473b67003 100644
--- a/rust/kernel/page.rs
+++ b/rust/kernel/page.rs
@@ -26,13 +26,12 @@
 pub const PAGE_MASK: usize = !(PAGE_SIZE - 1);

 /// Round up the given number to the next multiple of [`PAGE_SIZE`].
-///
-/// It is incorrect to pass an address where the next multiple of [`PAGE_SIZE`] doesn't fit in a
-/// [`usize`].
-pub const fn page_align(addr: usize) -> usize {
-    // Parentheses around `PAGE_SIZE - 1` to avoid triggering overflow sanitizers in the wrong
-    // cases.
-    (addr + (PAGE_SIZE - 1)) & PAGE_MASK
+/// Return None in cases where the next multiple of [`PAGE_SIZE`] would overflow a [`usize`]
+pub const fn page_align(addr: usize) -> Option<usize> {
+    if let Some(sum) = addr.checked_add(PAGE_SIZE - 1) {
+        return Some(sum & PAGE_MASK);
+    }
+    None
 }

 /// Representation of a non-owning reference to a [`Page`].
--
2.51.1

Re: [PATCH] rust: Return Option from page_align and ensure no usize overflow

[Alexandre Courbot]

On Thu Nov 27, 2025 at 10:07 PM JST, Brendan Shephard wrote:
> Changed page_align() to return Option<usize> which allows for validation

Please use imperative style as per the patch submission guidelines:
"Change page_align() ...".

https://docs.kernel.org/process/submitting-patches.html

> of the provided addr value. This ensures that any value that is provided
> within one PAGE_SIZE of usize::MAX will not panic and instead page_align
> will return None.
>
> Callers of page_align() should raise a EINVAL when they receive None
> from page_align().

Callers will return the error that is relevant to them. :) This
sentence is not needed.

>
> Signed-off-by: Brendan Shephard <bshephar@bne-home.net>
> ---
>  rust/kernel/page.rs | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
> index 432fc0297d4a..b78473b67003 100644
> --- a/rust/kernel/page.rs
> +++ b/rust/kernel/page.rs
> @@ -26,13 +26,12 @@
>  pub const PAGE_MASK: usize = !(PAGE_SIZE - 1);
>
>  /// Round up the given number to the next multiple of [`PAGE_SIZE`].
> -///
> -/// It is incorrect to pass an address where the next multiple of [`PAGE_SIZE`] doesn't fit in a
> -/// [`usize`].
> -pub const fn page_align(addr: usize) -> usize {
> -    // Parentheses around `PAGE_SIZE - 1` to avoid triggering overflow sanitizers in the wrong
> -    // cases.
> -    (addr + (PAGE_SIZE - 1)) & PAGE_MASK
> +/// Return None in cases where the next multiple of [`PAGE_SIZE`] would overflow a [`usize`]

You can doclink to [`None`].

Nit: dot at end of sentence missing.

> +pub const fn page_align(addr: usize) -> Option<usize> {
> +    if let Some(sum) = addr.checked_add(PAGE_SIZE - 1) {
> +        return Some(sum & PAGE_MASK);
> +    }
> +    None

A more idiomatic way to do this:

    addr.checked_add(PAGE_SIZE - 1).map(|sum| sum & PAGE_MASK)

Re: [PATCH] rust: Return Option from page_align and ensure no usize overflow

[Brendan Shephard]

On Thu, Nov 27, 2025 at 10:42:31PM +0900, Alexandre Courbot wrote:
> On Thu Nov 27, 2025 at 10:07 PM JST, Brendan Shephard wrote:
> > Changed page_align() to return Option<usize> which allows for validation
> 
> Please use imperative style as per the patch submission guidelines:
> "Change page_align() ...".
> 
> https://docs.kernel.org/process/submitting-patches.html
> 

Eh, my bad. Yeah, I'll fix that in v2.

> > of the provided addr value. This ensures that any value that is provided
> > within one PAGE_SIZE of usize::MAX will not panic and instead page_align
> > will return None.
> >
> > Callers of page_align() should raise a EINVAL when they receive None
> > from page_align().
> 
> Callers will return the error that is relevant to them. :) This
> sentence is not needed.
> 
Ack, I'll remove that part.
> >
> > Signed-off-by: Brendan Shephard <bshephar@bne-home.net>
> > ---
> >  rust/kernel/page.rs | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
> > index 432fc0297d4a..b78473b67003 100644
> > --- a/rust/kernel/page.rs
> > +++ b/rust/kernel/page.rs
> > @@ -26,13 +26,12 @@
> >  pub const PAGE_MASK: usize = !(PAGE_SIZE - 1);
> >
> >  /// Round up the given number to the next multiple of [`PAGE_SIZE`].
> > -///
> > -/// It is incorrect to pass an address where the next multiple of [`PAGE_SIZE`] doesn't fit in a
> > -/// [`usize`].
> > -pub const fn page_align(addr: usize) -> usize {
> > -    // Parentheses around `PAGE_SIZE - 1` to avoid triggering overflow sanitizers in the wrong
> > -    // cases.
> > -    (addr + (PAGE_SIZE - 1)) & PAGE_MASK
> > +/// Return None in cases where the next multiple of [`PAGE_SIZE`] would overflow a [`usize`]
> 
> You can doclink to [`None`].
> 
> Nit: dot at end of sentence missing.
>
Will fix this in v2

> > +pub const fn page_align(addr: usize) -> Option<usize> {
> > +    if let Some(sum) = addr.checked_add(PAGE_SIZE - 1) {
> > +        return Some(sum & PAGE_MASK);
> > +    }
> > +    None
> 
> A more idiomatic way to do this:
> 
>     addr.checked_add(PAGE_SIZE - 1).map(|sum| sum & PAGE_MASK)
> 
>

That would be nice, but alas since this is a const fn, we can't use map here.
I assume that was done for optimisation to ensure this could be referenced
at compile time. So I'm not sure we want to remove that optimisation here.

Re: [PATCH] rust: Return Option from page_align and ensure no usize overflow

[Alexandre Courbot]

On Thu Nov 27, 2025 at 11:18 PM JST, Brendan Shephard wrote:
> On Thu, Nov 27, 2025 at 10:42:31PM +0900, Alexandre Courbot wrote:
>> On Thu Nov 27, 2025 at 10:07 PM JST, Brendan Shephard wrote:
>> > Changed page_align() to return Option<usize> which allows for validation
>> 
>> Please use imperative style as per the patch submission guidelines:
>> "Change page_align() ...".
>> 
>> https://docs.kernel.org/process/submitting-patches.html
>> 
>
> Eh, my bad. Yeah, I'll fix that in v2.
>
>> > of the provided addr value. This ensures that any value that is provided
>> > within one PAGE_SIZE of usize::MAX will not panic and instead page_align
>> > will return None.
>> >
>> > Callers of page_align() should raise a EINVAL when they receive None
>> > from page_align().
>> 
>> Callers will return the error that is relevant to them. :) This
>> sentence is not needed.
>> 
> Ack, I'll remove that part.
>> >
>> > Signed-off-by: Brendan Shephard <bshephar@bne-home.net>
>> > ---
>> >  rust/kernel/page.rs | 13 ++++++-------
>> >  1 file changed, 6 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
>> > index 432fc0297d4a..b78473b67003 100644
>> > --- a/rust/kernel/page.rs
>> > +++ b/rust/kernel/page.rs
>> > @@ -26,13 +26,12 @@
>> >  pub const PAGE_MASK: usize = !(PAGE_SIZE - 1);
>> >
>> >  /// Round up the given number to the next multiple of [`PAGE_SIZE`].
>> > -///
>> > -/// It is incorrect to pass an address where the next multiple of [`PAGE_SIZE`] doesn't fit in a
>> > -/// [`usize`].
>> > -pub const fn page_align(addr: usize) -> usize {
>> > -    // Parentheses around `PAGE_SIZE - 1` to avoid triggering overflow sanitizers in the wrong
>> > -    // cases.
>> > -    (addr + (PAGE_SIZE - 1)) & PAGE_MASK
>> > +/// Return None in cases where the next multiple of [`PAGE_SIZE`] would overflow a [`usize`]
>> 
>> You can doclink to [`None`].
>> 
>> Nit: dot at end of sentence missing.
>>
> Will fix this in v2
>
>> > +pub const fn page_align(addr: usize) -> Option<usize> {
>> > +    if let Some(sum) = addr.checked_add(PAGE_SIZE - 1) {
>> > +        return Some(sum & PAGE_MASK);
>> > +    }
>> > +    None
>> 
>> A more idiomatic way to do this:
>> 
>>     addr.checked_add(PAGE_SIZE - 1).map(|sum| sum & PAGE_MASK)
>> 
>>
>
> That would be nice, but alas since this is a const fn, we can't use map here.
> I assume that was done for optimisation to ensure this could be referenced
> at compile time. So I'm not sure we want to remove that optimisation here.

Ah, indeed. In that case, can we make the if arms symmetric:

    if let Some(sum) = addr.checked_add(PAGE_SIZE - 1) {
        Some(sum & PAGE_MASK);
    } else {
        None
    }

Re: [PATCH] rust: Return Option from page_align and ensure no usize overflow

[Miguel Ojeda]

On Fri, Nov 28, 2025 at 1:29 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> Ah, indeed. In that case, can we make the if arms symmetric:
>
>     if let Some(sum) = addr.checked_add(PAGE_SIZE - 1) {
>         Some(sum & PAGE_MASK);
>     } else {
>         None
>     }

Early return style makes the happy path less indented and is more
consistent with the C side. However, here the happy case would be the
indented case, so `let else` would be better:

    let Some(sum) = addr.checked_add(PAGE_SIZE - 1) else {
        return None;
    };

    Some(sum & PAGE_MASK)

Cheers,
Miguel

Re: [PATCH] rust: Return Option from page_align and ensure no usize overflow

[Brendan Shephard]

On Fri, Nov 28, 2025 at 03:12:13AM +0100, Miguel Ojeda wrote:
> On Fri, Nov 28, 2025 at 1:29 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
> >
> > Ah, indeed. In that case, can we make the if arms symmetric:
> >
> >     if let Some(sum) = addr.checked_add(PAGE_SIZE - 1) {
> >         Some(sum & PAGE_MASK);
> >     } else {
> >         None
> >     }
> 
> Early return style makes the happy path less indented and is more
> consistent with the C side. However, here the happy case would be the
> indented case, so `let else` would be better:
> 
>     let Some(sum) = addr.checked_add(PAGE_SIZE - 1) else {
>         return None;
>     };
> 
>     Some(sum & PAGE_MASK)
> 
> Cheers,
> Miguel
> 

A wildcard entrant to this conversation. We could do a match instead if
you all might prefer that?

match addr.checked_add(PAGE_SIZE - 1) {
  Some(v) => Some(v & PAGE_MASK),
  None => None,
}

Something about that match syntax doesn't appeal to me though. I think I
prefer Miguel's suggestion here where we flip the happy and sad paths.
Make the sad path the early return and happy path the default. But I do
see at least a couple of `match` examples here, so it's probably worth a
mention to see what you think about it as an alternative?

Re: [PATCH] rust: Return Option from page_align and ensure no usize overflow

[Alexandre Courbot]

On Fri Nov 28, 2025 at 2:29 PM JST, Brendan Shephard wrote:
> On Fri, Nov 28, 2025 at 03:12:13AM +0100, Miguel Ojeda wrote:
>> On Fri, Nov 28, 2025 at 1:29 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
>> >
>> > Ah, indeed. In that case, can we make the if arms symmetric:
>> >
>> >     if let Some(sum) = addr.checked_add(PAGE_SIZE - 1) {
>> >         Some(sum & PAGE_MASK);
>> >     } else {
>> >         None
>> >     }
>> 
>> Early return style makes the happy path less indented and is more
>> consistent with the C side. However, here the happy case would be the
>> indented case, so `let else` would be better:
>> 
>>     let Some(sum) = addr.checked_add(PAGE_SIZE - 1) else {
>>         return None;
>>     };
>> 
>>     Some(sum & PAGE_MASK)
>> 
>> Cheers,
>> Miguel
>> 
>
> A wildcard entrant to this conversation. We could do a match instead if
> you all might prefer that?
>
> match addr.checked_add(PAGE_SIZE - 1) {
>   Some(v) => Some(v & PAGE_MASK),
>   None => None,
> }

Clippy will likely suggest to turn this into an `if { } else { }`, which
puts us back to where we come from. Miguel's suggestion of returning
early in the failure path looks good to me as well.

Re: [PATCH] rust: Return Option from page_align and ensure no usize overflow

[Alice Ryhl]

On Fri, Nov 28, 2025 at 7:51 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> On Fri Nov 28, 2025 at 2:29 PM JST, Brendan Shephard wrote:
> > On Fri, Nov 28, 2025 at 03:12:13AM +0100, Miguel Ojeda wrote:
> >> On Fri, Nov 28, 2025 at 1:29 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
> >> >
> >> > Ah, indeed. In that case, can we make the if arms symmetric:
> >> >
> >> >     if let Some(sum) = addr.checked_add(PAGE_SIZE - 1) {
> >> >         Some(sum & PAGE_MASK);
> >> >     } else {
> >> >         None
> >> >     }
> >>
> >> Early return style makes the happy path less indented and is more
> >> consistent with the C side. However, here the happy case would be the
> >> indented case, so `let else` would be better:
> >>
> >>     let Some(sum) = addr.checked_add(PAGE_SIZE - 1) else {
> >>         return None;
> >>     };
> >>
> >>     Some(sum & PAGE_MASK)
> >>
> >> Cheers,
> >> Miguel
> >>
> >
> > A wildcard entrant to this conversation. We could do a match instead if
> > you all might prefer that?
> >
> > match addr.checked_add(PAGE_SIZE - 1) {
> >   Some(v) => Some(v & PAGE_MASK),
> >   None => None,
> > }
>
> Clippy will likely suggest to turn this into an `if { } else { }`, which
> puts us back to where we come from. Miguel's suggestion of returning
> early in the failure path looks good to me as well.

How about using the question mark operator?

Some(addr.checked_add(PAGE_SIZE - 1)? & PAGE_MASK)

Re: [PATCH] rust: Return Option from page_align and ensure no usize overflow

[Alexandre Courbot]

On Fri Nov 28, 2025 at 6:09 PM JST, Alice Ryhl wrote:
> On Fri, Nov 28, 2025 at 7:51 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
>>
>> On Fri Nov 28, 2025 at 2:29 PM JST, Brendan Shephard wrote:
>> > On Fri, Nov 28, 2025 at 03:12:13AM +0100, Miguel Ojeda wrote:
>> >> On Fri, Nov 28, 2025 at 1:29 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
>> >> >
>> >> > Ah, indeed. In that case, can we make the if arms symmetric:
>> >> >
>> >> >     if let Some(sum) = addr.checked_add(PAGE_SIZE - 1) {
>> >> >         Some(sum & PAGE_MASK);
>> >> >     } else {
>> >> >         None
>> >> >     }
>> >>
>> >> Early return style makes the happy path less indented and is more
>> >> consistent with the C side. However, here the happy case would be the
>> >> indented case, so `let else` would be better:
>> >>
>> >>     let Some(sum) = addr.checked_add(PAGE_SIZE - 1) else {
>> >>         return None;
>> >>     };
>> >>
>> >>     Some(sum & PAGE_MASK)
>> >>
>> >> Cheers,
>> >> Miguel
>> >>
>> >
>> > A wildcard entrant to this conversation. We could do a match instead if
>> > you all might prefer that?
>> >
>> > match addr.checked_add(PAGE_SIZE - 1) {
>> >   Some(v) => Some(v & PAGE_MASK),
>> >   None => None,
>> > }
>>
>> Clippy will likely suggest to turn this into an `if { } else { }`, which
>> puts us back to where we come from. Miguel's suggestion of returning
>> early in the failure path looks good to me as well.
>
> How about using the question mark operator?
>
> Some(addr.checked_add(PAGE_SIZE - 1)? & PAGE_MASK)

That's unfortunately not usable yet in const contexts:

    error: `Try` is not yet stable as a const trait
      --> ../rust/kernel/page.rs:35:10
      |
    35 |     Some(addr.checked_add(PAGE_SIZE - 1)? & PAGE_MASK)
      |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      |
    help: add `#![feature(const_try)]` to the crate attributes to enable

Re: [PATCH] rust: Return Option from page_align and ensure no usize overflow

[Daniel Almeida]

> On 27 Nov 2025, at 10:07, Brendan Shephard <bshephar@bne-home.net> wrote:
> 
> Changed page_align() to return Option<usize> which allows for validation

nit: imperative voice here

> of the provided addr value. This ensures that any value that is provided
> within one PAGE_SIZE of usize::MAX will not panic and instead page_align
> will return None.
> 
> Callers of page_align() should raise a EINVAL when they receive None
> from page_align().
> 
> Signed-off-by: Brendan Shephard <bshephar@bne-home.net>
> ---
> rust/kernel/page.rs | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
> index 432fc0297d4a..b78473b67003 100644
> --- a/rust/kernel/page.rs
> +++ b/rust/kernel/page.rs
> @@ -26,13 +26,12 @@
> pub const PAGE_MASK: usize = !(PAGE_SIZE - 1);
> 
> /// Round up the given number to the next multiple of [`PAGE_SIZE`].
> -///
> -/// It is incorrect to pass an address where the next multiple of [`PAGE_SIZE`] doesn't fit in a
> -/// [`usize`].

Can you document the return value? I’d find it weird that this returns Option otherwise.

> -pub const fn page_align(addr: usize) -> usize {
> -    // Parentheses around `PAGE_SIZE - 1` to avoid triggering overflow sanitizers in the wrong
> -    // cases.
> -    (addr + (PAGE_SIZE - 1)) & PAGE_MASK
> +/// Return None in cases where the next multiple of [`PAGE_SIZE`] would overflow a [`usize`]
> +pub const fn page_align(addr: usize) -> Option<usize> {
> +    if let Some(sum) = addr.checked_add(PAGE_SIZE - 1) {
> +        return Some(sum & PAGE_MASK);
> +    }
> +    None
> }
> 
> /// Representation of a non-owning reference to a [`Page`].
> --
> 2.51.1
> 

With the change above:

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH] rust: Return Option from page_align and ensure no usize overflow

[Brendan Shephard]

On Thu, Nov 27, 2025 at 10:44:16AM -0300, Daniel Almeida wrote:
> 
> 
> > On 27 Nov 2025, at 10:07, Brendan Shephard <bshephar@bne-home.net> wrote:
> > 
> > Changed page_align() to return Option<usize> which allows for validation
> 
> nit: imperative voice here
> 
Yeah, my bad. I'll fix that in v2.

> > of the provided addr value. This ensures that any value that is provided
> > within one PAGE_SIZE of usize::MAX will not panic and instead page_align
> > will return None.
> > 
> > Callers of page_align() should raise a EINVAL when they receive None
> > from page_align().
> > 
> > Signed-off-by: Brendan Shephard <bshephar@bne-home.net>
> > ---
> > rust/kernel/page.rs | 13 ++++++-------
> > 1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
> > index 432fc0297d4a..b78473b67003 100644
> > --- a/rust/kernel/page.rs
> > +++ b/rust/kernel/page.rs
> > @@ -26,13 +26,12 @@
> > pub const PAGE_MASK: usize = !(PAGE_SIZE - 1);
> > 
> > /// Round up the given number to the next multiple of [`PAGE_SIZE`].
> > -///
> > -/// It is incorrect to pass an address where the next multiple of [`PAGE_SIZE`] doesn't fit in a
> > -/// [`usize`].
> 
> Can you document the return value? I’d find it weird that this returns Option otherwise.
>
Sounds like a good plan. I'll document both return Options in v2. None
if the resulting value would overflow a usize::MAX, or Some usize that
is page aligned.

> > -pub const fn page_align(addr: usize) -> usize {
> > -    // Parentheses around `PAGE_SIZE - 1` to avoid triggering overflow sanitizers in the wrong
> > -    // cases.
> > -    (addr + (PAGE_SIZE - 1)) & PAGE_MASK
> > +/// Return None in cases where the next multiple of [`PAGE_SIZE`] would overflow a [`usize`]
> > +pub const fn page_align(addr: usize) -> Option<usize> {
> > +    if let Some(sum) = addr.checked_add(PAGE_SIZE - 1) {
> > +        return Some(sum & PAGE_MASK);
> > +    }
> > +    None
> > }
> > 
> > /// Representation of a non-owning reference to a [`Page`].
> > --
> > 2.51.1
> > 
> 
> With the change above:
> 
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> 

Re: [PATCH v2] rust: Return Option from page_align and ensure no usize overflow

[Brendan Shephard]

Changes in v2:
  - Reworded commit message to follow the imperative form.
  - Expanded the documentation to explain the `Some` and `None` return cases.
  - Added a period at the end of the documentation comment.

Change `page_align()` to return `Option<usize>` to allow validation
of the provided `addr` value. This ensures that any value that is
within one `PAGE_SIZE` of `usize::MAX` will not panic, and instead
returns `None` to indicate overflow.

Signed-off-by: Brendan Shephard <bshephar@bne-home.net>
---
 rust/kernel/page.rs | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
index 432fc0297d4a..d5cc340c0689 100644
--- a/rust/kernel/page.rs
+++ b/rust/kernel/page.rs
@@ -26,13 +26,13 @@
 pub const PAGE_MASK: usize = !(PAGE_SIZE - 1);
 
 /// Round up the given number to the next multiple of [`PAGE_SIZE`].
-///
-/// It is incorrect to pass an address where the next multiple of [`PAGE_SIZE`] doesn't fit in a
-/// [`usize`].
-pub const fn page_align(addr: usize) -> usize {
-    // Parentheses around `PAGE_SIZE - 1` to avoid triggering overflow sanitizers in the wrong
-    // cases.
-    (addr + (PAGE_SIZE - 1)) & PAGE_MASK
+/// Return Some [`usize`] that is page aligned. Or None in cases where the next multiple of
+/// [`PAGE_SIZE`] would overflow a [`usize`].
+pub const fn page_align(addr: usize) -> Option<usize> {
+    if let Some(sum) = addr.checked_add(PAGE_SIZE - 1) {
+        return Some(sum & PAGE_MASK);
+    }
+    None
 }
 
 /// Representation of a non-owning reference to a [`Page`].
-- 
2.51.1

Re: [PATCH v2] rust: Return Option from page_align and ensure no usize overflow

[Miguel Ojeda]

On Thu, Nov 27, 2025 at 3:41 PM Brendan Shephard <bshephar@bne-home.net> wrote:
>
> Changes in v2:
>   - Reworded commit message to follow the imperative form.
>   - Expanded the documentation to explain the `Some` and `None` return cases.
>   - Added a period at the end of the documentation comment.

This shouldn't be in the commit message. It should instead be placed
below the first `---` line (the one below the Signed-off-by).

Also, I would recommend not replying to the previous version but
starting a new thread instead (it is good to provide a lore.kernel.org
to the previous version in the change log).

In addition, if possible, it is nice to use `--base` to tell Git to
provide the base commit.

> +/// Return Some [`usize`] that is page aligned. Or None in cases where the next multiple of
> +/// [`PAGE_SIZE`] would overflow a [`usize`].

Missing intra-doc link (this was feedback in v1). In addition, this
reads a bit strangely, and it looks like the first line of the
description wasn't removed? That would make the "short description"
(the first paragraph) quite long. Please generate the docs to see if
it looks good when generated.

More importantly, please add some examples (doctests). They are tested
as KUnit tests automatically. For new APIs, we always try to do add
examples, and in cases where there are edge cases like here, it
clarifies quite a bit, i.e. please provide a representative example of
the `Some` case and the `None` case.

Thanks!

Cheers,
Miguel

Re: [PATCH v2] rust: Return Option from page_align and ensure no usize overflow

[Brendan Shephard]

On Thu, Nov 27, 2025 at 05:24:54PM +0100, Miguel Ojeda wrote:
> On Thu, Nov 27, 2025 at 3:41 PM Brendan Shephard <bshephar@bne-home.net> wrote:
> >
> > Changes in v2:
> >   - Reworded commit message to follow the imperative form.
> >   - Expanded the documentation to explain the `Some` and `None` return cases.
> >   - Added a period at the end of the documentation comment.
> 
> This shouldn't be in the commit message. It should instead be placed
> below the first `---` line (the one below the Signed-off-by).
> 
> Also, I would recommend not replying to the previous version but
> starting a new thread instead (it is good to provide a lore.kernel.org
> to the previous version in the change log).
> 
> In addition, if possible, it is nice to use `--base` to tell Git to
> provide the base commit.
> 
> > +/// Return Some [`usize`] that is page aligned. Or None in cases where the next multiple of
> > +/// [`PAGE_SIZE`] would overflow a [`usize`].
> 
> Missing intra-doc link (this was feedback in v1). In addition, this
> reads a bit strangely, and it looks like the first line of the
> description wasn't removed? That would make the "short description"
> (the first paragraph) quite long. Please generate the docs to see if
> it looks good when generated.
> 
> More importantly, please add some examples (doctests). They are tested
> as KUnit tests automatically. For new APIs, we always try to do add
> examples, and in cases where there are edge cases like here, it
> clarifies quite a bit, i.e. please provide a representative example of
> the `Some` case and the `None` case.
> 
> Thanks!
> 
> Cheers,
> Miguel

This is all really good feedback Miguel, thanks for the pointers. I
really like the idea of adding examples. Let me work on that, and then
I'll send a V3 as a new thread with references to this one via the
lore.kernel.org link.