diff mbox series

[net-next,v6,1/6] rust: sizes: add commonly used constants

Message ID 20240820225719.91410-2-fujita.tomonori@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: phy: add Applied Micro QT2025 PHY driver | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 6 maintainers not CCed: wedsonaf@gmail.com alex.gaynor@gmail.com gary@garyguo.net bjorn3_gh@protonmail.com boqun.feng@gmail.com a.hindborg@samsung.com
netdev/build_clang success Errors and warnings before: 17 this patch: 17
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 22 this patch: 22
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/build_clang_rust fail Link
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-08-21--06-00 (tests: 711)

Commit Message

FUJITA Tomonori Aug. 20, 2024, 10:57 p.m. UTC 
Add rust equivalent to include/linux/sizes.h, makes code more
readable.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/lib.rs   |  1 +
 rust/kernel/sizes.rs | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)
 create mode 100644 rust/kernel/sizes.rs

Comments

Greg Kroah-Hartman Aug. 20, 2024, 11:41 p.m. UTC | #1 
On Tue, Aug 20, 2024 at 10:57:14PM +0000, FUJITA Tomonori wrote:
> Add rust equivalent to include/linux/sizes.h, makes code more
> readable.
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> Reviewed-by: Trevor Gross <tmgross@umich.edu>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/lib.rs   |  1 +
>  rust/kernel/sizes.rs | 26 ++++++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
>  create mode 100644 rust/kernel/sizes.rs
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 274bdc1b0a82..58ed400198bf 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -43,6 +43,7 @@
>  pub mod page;
>  pub mod prelude;
>  pub mod print;
> +pub mod sizes;
>  mod static_assert;
>  #[doc(hidden)]
>  pub mod std_vendor;
> diff --git a/rust/kernel/sizes.rs b/rust/kernel/sizes.rs
> new file mode 100644
> index 000000000000..834c343e4170
> --- /dev/null
> +++ b/rust/kernel/sizes.rs
> @@ -0,0 +1,26 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Commonly used sizes.
> +//!
> +//! C headers: [`include/linux/sizes.h`](srctree/include/linux/sizes.h).
> +
> +/// 0x00000400
> +pub const SZ_1K: usize = bindings::SZ_1K as usize;
> +/// 0x00000800
> +pub const SZ_2K: usize = bindings::SZ_2K as usize;
> +/// 0x00001000
> +pub const SZ_4K: usize = bindings::SZ_4K as usize;
> +/// 0x00002000
> +pub const SZ_8K: usize = bindings::SZ_8K as usize;
> +/// 0x00004000
> +pub const SZ_16K: usize = bindings::SZ_16K as usize;
> +/// 0x00008000
> +pub const SZ_32K: usize = bindings::SZ_32K as usize;
> +/// 0x00010000
> +pub const SZ_64K: usize = bindings::SZ_64K as usize;
> +/// 0x00020000
> +pub const SZ_128K: usize = bindings::SZ_128K as usize;
> +/// 0x00040000
> +pub const SZ_256K: usize = bindings::SZ_256K as usize;
> +/// 0x00080000
> +pub const SZ_512K: usize = bindings::SZ_512K as usize;

Why only some of the values in sizes.h?

And why can't sizes.h be directly translated into rust code without
having to do it "by hand" here?  We do that for other header file
bindings, right?

thanks,

greg k-h
Danilo Krummrich Aug. 20, 2024, 11:54 p.m. UTC | #2 
On 8/21/24 1:41 AM, Greg KH wrote:
> On Tue, Aug 20, 2024 at 10:57:14PM +0000, FUJITA Tomonori wrote:
>> Add rust equivalent to include/linux/sizes.h, makes code more
>> readable.
>>
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
>> Reviewed-by: Trevor Gross <tmgross@umich.edu>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> ---
>>   rust/kernel/lib.rs   |  1 +
>>   rust/kernel/sizes.rs | 26 ++++++++++++++++++++++++++
>>   2 files changed, 27 insertions(+)
>>   create mode 100644 rust/kernel/sizes.rs
>>
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index 274bdc1b0a82..58ed400198bf 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -43,6 +43,7 @@
>>   pub mod page;
>>   pub mod prelude;
>>   pub mod print;
>> +pub mod sizes;
>>   mod static_assert;
>>   #[doc(hidden)]
>>   pub mod std_vendor;
>> diff --git a/rust/kernel/sizes.rs b/rust/kernel/sizes.rs
>> new file mode 100644
>> index 000000000000..834c343e4170
>> --- /dev/null
>> +++ b/rust/kernel/sizes.rs
>> @@ -0,0 +1,26 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Commonly used sizes.
>> +//!
>> +//! C headers: [`include/linux/sizes.h`](srctree/include/linux/sizes.h).
>> +
>> +/// 0x00000400
>> +pub const SZ_1K: usize = bindings::SZ_1K as usize;
>> +/// 0x00000800
>> +pub const SZ_2K: usize = bindings::SZ_2K as usize;
>> +/// 0x00001000
>> +pub const SZ_4K: usize = bindings::SZ_4K as usize;
>> +/// 0x00002000
>> +pub const SZ_8K: usize = bindings::SZ_8K as usize;
>> +/// 0x00004000
>> +pub const SZ_16K: usize = bindings::SZ_16K as usize;
>> +/// 0x00008000
>> +pub const SZ_32K: usize = bindings::SZ_32K as usize;
>> +/// 0x00010000
>> +pub const SZ_64K: usize = bindings::SZ_64K as usize;
>> +/// 0x00020000
>> +pub const SZ_128K: usize = bindings::SZ_128K as usize;
>> +/// 0x00040000
>> +pub const SZ_256K: usize = bindings::SZ_256K as usize;
>> +/// 0x00080000
>> +pub const SZ_512K: usize = bindings::SZ_512K as usize;
> 
> Why only some of the values in sizes.h?
> 
> And why can't sizes.h be directly translated into rust code without
> having to do it "by hand" here?  We do that for other header file
> bindings, right?

Those are all generated bindings from C headers, e.g. `bindings::SZ_2K `, but
bindgen isn't guaranteed to assign the type we want (`usize` in this case), plus
for size constants having the `bindings::` prefix doesn't really add any value.

Rust could also define those without using bindings at all, but this was already
discussed in earlier versions of this series.

- Danilo

> 
> thanks,
> 
> greg k-h
>
FUJITA Tomonori Aug. 21, 2024, 12:14 a.m. UTC | #3 
On Wed, 21 Aug 2024 07:41:24 +0800
Greg KH <gregkh@linuxfoundation.org> wrote:

>> +/// 0x00000400
>> +pub const SZ_1K: usize = bindings::SZ_1K as usize;
>> +/// 0x00000800
>> +pub const SZ_2K: usize = bindings::SZ_2K as usize;
>> +/// 0x00001000
>> +pub const SZ_4K: usize = bindings::SZ_4K as usize;
>> +/// 0x00002000
>> +pub const SZ_8K: usize = bindings::SZ_8K as usize;
>> +/// 0x00004000
>> +pub const SZ_16K: usize = bindings::SZ_16K as usize;
>> +/// 0x00008000
>> +pub const SZ_32K: usize = bindings::SZ_32K as usize;
>> +/// 0x00010000
>> +pub const SZ_64K: usize = bindings::SZ_64K as usize;
>> +/// 0x00020000
>> +pub const SZ_128K: usize = bindings::SZ_128K as usize;
>> +/// 0x00040000
>> +pub const SZ_256K: usize = bindings::SZ_256K as usize;
>> +/// 0x00080000
>> +pub const SZ_512K: usize = bindings::SZ_512K as usize;
> 
> Why only some of the values in sizes.h?

Because this driver needs only some SZ_*K and looks like SZ_*K are
more widely used. But we can add more anytime.

> And why can't sizes.h be directly translated into rust code without
> having to do it "by hand" here?  We do that for other header file
> bindings, right?

No. bindings::* are generated from C headers and kernel crates
give them to drivers by hand. For example, rust/kernel/page.rs has:

pub const PAGE_SIZE: usize = bindings::PAGE_SIZE;
Greg Kroah-Hartman Aug. 21, 2024, 12:22 a.m. UTC | #4 
On Wed, Aug 21, 2024 at 01:54:43AM +0200, Danilo Krummrich wrote:
> On 8/21/24 1:41 AM, Greg KH wrote:
> > On Tue, Aug 20, 2024 at 10:57:14PM +0000, FUJITA Tomonori wrote:
> > > Add rust equivalent to include/linux/sizes.h, makes code more
> > > readable.
> > > 
> > > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > > Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> > > Reviewed-by: Trevor Gross <tmgross@umich.edu>
> > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > > ---
> > >   rust/kernel/lib.rs   |  1 +
> > >   rust/kernel/sizes.rs | 26 ++++++++++++++++++++++++++
> > >   2 files changed, 27 insertions(+)
> > >   create mode 100644 rust/kernel/sizes.rs
> > > 
> > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > > index 274bdc1b0a82..58ed400198bf 100644
> > > --- a/rust/kernel/lib.rs
> > > +++ b/rust/kernel/lib.rs
> > > @@ -43,6 +43,7 @@
> > >   pub mod page;
> > >   pub mod prelude;
> > >   pub mod print;
> > > +pub mod sizes;
> > >   mod static_assert;
> > >   #[doc(hidden)]
> > >   pub mod std_vendor;
> > > diff --git a/rust/kernel/sizes.rs b/rust/kernel/sizes.rs
> > > new file mode 100644
> > > index 000000000000..834c343e4170
> > > --- /dev/null
> > > +++ b/rust/kernel/sizes.rs
> > > @@ -0,0 +1,26 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +//! Commonly used sizes.
> > > +//!
> > > +//! C headers: [`include/linux/sizes.h`](srctree/include/linux/sizes.h).
> > > +
> > > +/// 0x00000400
> > > +pub const SZ_1K: usize = bindings::SZ_1K as usize;
> > > +/// 0x00000800
> > > +pub const SZ_2K: usize = bindings::SZ_2K as usize;
> > > +/// 0x00001000
> > > +pub const SZ_4K: usize = bindings::SZ_4K as usize;
> > > +/// 0x00002000
> > > +pub const SZ_8K: usize = bindings::SZ_8K as usize;
> > > +/// 0x00004000
> > > +pub const SZ_16K: usize = bindings::SZ_16K as usize;
> > > +/// 0x00008000
> > > +pub const SZ_32K: usize = bindings::SZ_32K as usize;
> > > +/// 0x00010000
> > > +pub const SZ_64K: usize = bindings::SZ_64K as usize;
> > > +/// 0x00020000
> > > +pub const SZ_128K: usize = bindings::SZ_128K as usize;
> > > +/// 0x00040000
> > > +pub const SZ_256K: usize = bindings::SZ_256K as usize;
> > > +/// 0x00080000
> > > +pub const SZ_512K: usize = bindings::SZ_512K as usize;
> > 
> > Why only some of the values in sizes.h?
> > 
> > And why can't sizes.h be directly translated into rust code without
> > having to do it "by hand" here?  We do that for other header file
> > bindings, right?
> 
> Those are all generated bindings from C headers, e.g. `bindings::SZ_2K `, but
> bindgen isn't guaranteed to assign the type we want (`usize` in this case), plus
> for size constants having the `bindings::` prefix doesn't really add any value.
> 
> Rust could also define those without using bindings at all, but this was already
> discussed in earlier versions of this series.

Then this information should probably go in the changelog text so that
others don't keep asking the same question :)

thanks,

greg k-h
diff mbox series

Patch

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 274bdc1b0a82..58ed400198bf 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -43,6 +43,7 @@ 
 pub mod page;
 pub mod prelude;
 pub mod print;
+pub mod sizes;
 mod static_assert;
 #[doc(hidden)]
 pub mod std_vendor;
diff --git a/rust/kernel/sizes.rs b/rust/kernel/sizes.rs
new file mode 100644
index 000000000000..834c343e4170
--- /dev/null
+++ b/rust/kernel/sizes.rs
@@ -0,0 +1,26 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+//! Commonly used sizes.
+//!
+//! C headers: [`include/linux/sizes.h`](srctree/include/linux/sizes.h).
+
+/// 0x00000400
+pub const SZ_1K: usize = bindings::SZ_1K as usize;
+/// 0x00000800
+pub const SZ_2K: usize = bindings::SZ_2K as usize;
+/// 0x00001000
+pub const SZ_4K: usize = bindings::SZ_4K as usize;
+/// 0x00002000
+pub const SZ_8K: usize = bindings::SZ_8K as usize;
+/// 0x00004000
+pub const SZ_16K: usize = bindings::SZ_16K as usize;
+/// 0x00008000
+pub const SZ_32K: usize = bindings::SZ_32K as usize;
+/// 0x00010000
+pub const SZ_64K: usize = bindings::SZ_64K as usize;
+/// 0x00020000
+pub const SZ_128K: usize = bindings::SZ_128K as usize;
+/// 0x00040000
+pub const SZ_256K: usize = bindings::SZ_256K as usize;
+/// 0x00080000
+pub const SZ_512K: usize = bindings::SZ_512K as usize;