Add KUNIT tests to make sure the macro is working correctly.
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
rust/kernel/bitfield.rs | 321 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 321 insertions(+)
diff --git a/rust/kernel/bitfield.rs b/rust/kernel/bitfield.rs
index fed19918c3b9..9a20bcd2eb60 100644
--- a/rust/kernel/bitfield.rs
+++ b/rust/kernel/bitfield.rs
@@ -402,3 +402,324 @@ fn default() -> Self {
}
};
}
+
+#[::kernel::macros::kunit_tests(kernel_bitfield)]
+mod tests {
+ use core::convert::TryFrom;
+
+ // Enum types for testing => and ?=> conversions
+ #[derive(Debug, Clone, Copy, PartialEq)]
+ enum MemoryType {
+ Unmapped = 0,
+ Normal = 1,
+ Device = 2,
+ Reserved = 3,
+ }
+
+ impl Default for MemoryType {
+ fn default() -> Self {
+ MemoryType::Unmapped
+ }
+ }
+
+ impl TryFrom<u8> for MemoryType {
+ type Error = u8;
+ fn try_from(value: u8) -> Result<Self, Self::Error> {
+ match value {
+ 0 => Ok(MemoryType::Unmapped),
+ 1 => Ok(MemoryType::Normal),
+ 2 => Ok(MemoryType::Device),
+ 3 => Ok(MemoryType::Reserved),
+ _ => Err(value),
+ }
+ }
+ }
+
+ impl From<MemoryType> for u64 {
+ fn from(mt: MemoryType) -> u64 {
+ mt as u64
+ }
+ }
+
+ #[derive(Debug, Clone, Copy, PartialEq)]
+ enum Priority {
+ Low = 0,
+ Medium = 1,
+ High = 2,
+ Critical = 3,
+ }
+
+ impl Default for Priority {
+ fn default() -> Self {
+ Priority::Low
+ }
+ }
+
+ impl From<u8> for Priority {
+ fn from(value: u8) -> Self {
+ match value & 0x3 {
+ 0 => Priority::Low,
+ 1 => Priority::Medium,
+ 2 => Priority::High,
+ _ => Priority::Critical,
+ }
+ }
+ }
+
+ impl From<Priority> for u16 {
+ fn from(p: Priority) -> u16 {
+ p as u16
+ }
+ }
+
+ bitfield! {
+ struct TestPageTableEntry(u64) {
+ 0:0 present as bool;
+ 1:1 writable as bool;
+ 11:9 available as u8;
+ 13:12 mem_type as u8 ?=> MemoryType;
+ 17:14 extended_type as u8 ?=> MemoryType; // For testing failures
+ 51:12 pfn as u64;
+ 51:12 pfn_overlap as u64;
+ 61:52 available2 as u16;
+ }
+ }
+
+ bitfield! {
+ struct TestControlRegister(u16) {
+ 0:0 enable as bool;
+ 3:1 mode as u8;
+ 5:4 priority as u8 => Priority;
+ 7:4 priority_nibble as u8;
+ 15:8 channel as u8;
+ }
+ }
+
+ bitfield! {
+ struct TestStatusRegister(u8) {
+ 0:0 ready as bool;
+ 1:1 error as bool;
+ 3:2 state as u8;
+ 7:4 reserved as u8;
+ 7:0 full_byte as u8; // For entire register
+ }
+ }
+
+ #[test]
+ fn test_single_bits() {
+ let mut pte = TestPageTableEntry::default();
+
+ assert!(!pte.present());
+ assert!(!pte.writable());
+
+ pte = pte.set_present(true);
+ assert!(pte.present());
+
+ pte = pte.set_writable(true);
+ assert!(pte.writable());
+
+ pte = pte.set_writable(false);
+ assert!(!pte.writable());
+
+ assert_eq!(pte.available(), 0);
+ pte = pte.set_available(0x5);
+ assert_eq!(pte.available(), 0x5);
+ }
+
+ #[test]
+ fn test_range_fields() {
+ let mut pte = TestPageTableEntry::default();
+
+ pte = pte.set_pfn(0x123456);
+ assert_eq!(pte.pfn(), 0x123456);
+ // Test overlapping field reads same value
+ assert_eq!(pte.pfn_overlap(), 0x123456);
+
+ pte = pte.set_available(0x7);
+ assert_eq!(pte.available(), 0x7);
+
+ pte = pte.set_available2(0x3FF);
+ assert_eq!(pte.available2(), 0x3FF);
+
+ // Test TryFrom with ?=> for MemoryType
+ pte = pte.set_mem_type(MemoryType::Device);
+ assert_eq!(pte.mem_type(), Ok(MemoryType::Device));
+
+ pte = pte.set_mem_type(MemoryType::Normal);
+ assert_eq!(pte.mem_type(), Ok(MemoryType::Normal));
+
+ // Test all valid values for mem_type
+ pte = pte.set_mem_type(MemoryType::Reserved); // Valid value: 3
+ assert_eq!(pte.mem_type(), Ok(MemoryType::Reserved));
+
+ // Test failure case using extended_type field which has 4 bits (0-15)
+ // MemoryType only handles 0-3, so values 4-15 should return Err
+ let mut raw = pte.raw();
+ // Set bits 17:14 to 7 (invalid for MemoryType)
+ raw = (raw & !::kernel::bits::genmask_u64(14..=17)) | (0x7 << 14);
+ let invalid_pte = TestPageTableEntry::new(raw);
+ // Should return Err with the invalid value
+ assert_eq!(invalid_pte.extended_type(), Err(0x7));
+
+ // Test a valid value after testing invalid to ensure both cases work
+ // Set bits 17:14 to 2 (valid: Device)
+ raw = (raw & !::kernel::bits::genmask_u64(14..=17)) | (0x2 << 14);
+ let valid_pte = TestPageTableEntry::new(raw);
+ assert_eq!(valid_pte.extended_type(), Ok(MemoryType::Device));
+
+ let max_pfn = ::kernel::bits::genmask_u64(0..=39);
+ pte = pte.set_pfn(max_pfn);
+ assert_eq!(pte.pfn(), max_pfn);
+ assert_eq!(pte.pfn_overlap(), max_pfn);
+ }
+
+ #[test]
+ fn test_builder_pattern() {
+ let pte = TestPageTableEntry::default()
+ .set_present(true)
+ .set_writable(true)
+ .set_available(0x7)
+ .set_pfn(0xABCDEF)
+ .set_mem_type(MemoryType::Reserved)
+ .set_available2(0x3FF);
+
+ assert!(pte.present());
+ assert!(pte.writable());
+ assert_eq!(pte.available(), 0x7);
+ assert_eq!(pte.pfn(), 0xABCDEF);
+ assert_eq!(pte.pfn_overlap(), 0xABCDEF);
+ assert_eq!(pte.mem_type(), Ok(MemoryType::Reserved));
+ assert_eq!(pte.available2(), 0x3FF);
+ }
+
+ #[test]
+ fn test_raw_operations() {
+ let raw_value = 0x3FF0000003123E03u64;
+
+ let pte = TestPageTableEntry::new(raw_value);
+ assert_eq!(pte.raw(), raw_value);
+
+ assert!(pte.present());
+ assert!(pte.writable());
+ assert_eq!(pte.available(), 0x7);
+ assert_eq!(pte.pfn(), 0x3123);
+ assert_eq!(pte.pfn_overlap(), 0x3123);
+ assert_eq!(pte.mem_type(), Ok(MemoryType::Reserved));
+ assert_eq!(pte.available2(), 0x3FF);
+
+ // Test using direct constructor syntax TestStruct(value)
+ let pte2 = TestPageTableEntry::new(raw_value);
+ assert_eq!(pte2.raw(), raw_value);
+ }
+
+ #[test]
+ fn test_u16_bitfield() {
+ let mut ctrl = TestControlRegister::default();
+
+ assert!(!ctrl.enable());
+ assert_eq!(ctrl.mode(), 0);
+ assert_eq!(ctrl.priority(), Priority::Low);
+ assert_eq!(ctrl.priority_nibble(), 0);
+ assert_eq!(ctrl.channel(), 0);
+
+ ctrl = ctrl.set_enable(true);
+ assert!(ctrl.enable());
+
+ ctrl = ctrl.set_mode(0x5);
+ assert_eq!(ctrl.mode(), 0x5);
+
+ // Test From conversion with =>
+ ctrl = ctrl.set_priority(Priority::High);
+ assert_eq!(ctrl.priority(), Priority::High);
+ assert_eq!(ctrl.priority_nibble(), 0x2); // High = 2 in bits 5:4
+
+ ctrl = ctrl.set_channel(0xAB);
+ assert_eq!(ctrl.channel(), 0xAB);
+
+ // Test overlapping fields
+ ctrl = ctrl.set_priority_nibble(0xF);
+ assert_eq!(ctrl.priority_nibble(), 0xF);
+ assert_eq!(ctrl.priority(), Priority::Critical); // bits 5:4 = 0x3
+
+ let ctrl2 = TestControlRegister::default()
+ .set_enable(true)
+ .set_mode(0x3)
+ .set_priority(Priority::Medium)
+ .set_channel(0x42);
+
+ assert!(ctrl2.enable());
+ assert_eq!(ctrl2.mode(), 0x3);
+ assert_eq!(ctrl2.priority(), Priority::Medium);
+ assert_eq!(ctrl2.channel(), 0x42);
+
+ let raw_value: u16 = 0x4217;
+ let ctrl3 = TestControlRegister::new(raw_value);
+ assert_eq!(ctrl3.raw(), raw_value);
+ assert!(ctrl3.enable());
+ assert_eq!(ctrl3.priority(), Priority::Medium);
+ assert_eq!(ctrl3.priority_nibble(), 0x1);
+ assert_eq!(ctrl3.channel(), 0x42);
+ }
+
+ #[test]
+ fn test_u8_bitfield() {
+ let mut status = TestStatusRegister::default();
+
+ assert!(!status.ready());
+ assert!(!status.error());
+ assert_eq!(status.state(), 0);
+ assert_eq!(status.reserved(), 0);
+ assert_eq!(status.full_byte(), 0);
+
+ status = status.set_ready(true);
+ assert!(status.ready());
+ assert_eq!(status.full_byte(), 0x01);
+
+ status = status.set_error(true);
+ assert!(status.error());
+ assert_eq!(status.full_byte(), 0x03);
+
+ status = status.set_state(0x3);
+ assert_eq!(status.state(), 0x3);
+ assert_eq!(status.full_byte(), 0x0F);
+
+ status = status.set_reserved(0xA);
+ assert_eq!(status.reserved(), 0xA);
+ assert_eq!(status.full_byte(), 0xAF);
+
+ // Test overlapping field
+ status = status.set_full_byte(0x55);
+ assert_eq!(status.full_byte(), 0x55);
+ assert!(status.ready());
+ assert!(!status.error());
+ assert_eq!(status.state(), 0x1);
+ assert_eq!(status.reserved(), 0x5);
+
+ let status2 = TestStatusRegister::default()
+ .set_ready(true)
+ .set_state(0x2)
+ .set_reserved(0x5);
+
+ assert!(status2.ready());
+ assert!(!status2.error());
+ assert_eq!(status2.state(), 0x2);
+ assert_eq!(status2.reserved(), 0x5);
+ assert_eq!(status2.full_byte(), 0x59);
+
+ let raw_value: u8 = 0x59;
+ let status3 = TestStatusRegister::new(raw_value);
+ assert_eq!(status3.raw(), raw_value);
+ assert!(status3.ready());
+ assert!(!status3.error());
+ assert_eq!(status3.state(), 0x2);
+ assert_eq!(status3.reserved(), 0x5);
+ assert_eq!(status3.full_byte(), 0x59);
+
+ let status4 = TestStatusRegister::new(0xFF);
+ assert!(status4.ready());
+ assert!(status4.error());
+ assert_eq!(status4.state(), 0x3);
+ assert_eq!(status4.reserved(), 0xF);
+ assert_eq!(status4.full_byte(), 0xFF);
+ }
+}
--
2.34.1On Tue Sep 30, 2025 at 11:45 PM JST, Joel Fernandes wrote:
> Add KUNIT tests to make sure the macro is working correctly.
>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> ---
> rust/kernel/bitfield.rs | 321 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 321 insertions(+)
>
> diff --git a/rust/kernel/bitfield.rs b/rust/kernel/bitfield.rs
> index fed19918c3b9..9a20bcd2eb60 100644
> --- a/rust/kernel/bitfield.rs
> +++ b/rust/kernel/bitfield.rs
> @@ -402,3 +402,324 @@ fn default() -> Self {
> }
> };
> }
> +
> +#[::kernel::macros::kunit_tests(kernel_bitfield)]
> +mod tests {
> + use core::convert::TryFrom;
> +
> + // Enum types for testing => and ?=> conversions
> + #[derive(Debug, Clone, Copy, PartialEq)]
> + enum MemoryType {
> + Unmapped = 0,
> + Normal = 1,
> + Device = 2,
> + Reserved = 3,
> + }
> +
> + impl Default for MemoryType {
> + fn default() -> Self {
> + MemoryType::Unmapped
> + }
> + }
Tip: you can add `Default` to the `#[derive]` marker of `MemoryType` and
mark the variant you want as default with `#[default]` instead of
providing a full impl block:
#[derive(Debug, Default, Clone, Copy, PartialEq)]
enum MemoryType {
#[default]
Unmapped = 0,
Normal = 1,
Device = 2,
Reserved = 3,
}
> +
> + impl TryFrom<u8> for MemoryType {
> + type Error = u8;
> + fn try_from(value: u8) -> Result<Self, Self::Error> {
> + match value {
> + 0 => Ok(MemoryType::Unmapped),
> + 1 => Ok(MemoryType::Normal),
> + 2 => Ok(MemoryType::Device),
> + 3 => Ok(MemoryType::Reserved),
> + _ => Err(value),
> + }
> + }
> + }
> +
> + impl From<MemoryType> for u64 {
> + fn from(mt: MemoryType) -> u64 {
> + mt as u64
> + }
> + }
> +
> + #[derive(Debug, Clone, Copy, PartialEq)]
> + enum Priority {
> + Low = 0,
> + Medium = 1,
> + High = 2,
> + Critical = 3,
> + }
> +
> + impl Default for Priority {
> + fn default() -> Self {
> + Priority::Low
> + }
> + }
> +
> + impl From<u8> for Priority {
> + fn from(value: u8) -> Self {
> + match value & 0x3 {
> + 0 => Priority::Low,
> + 1 => Priority::Medium,
> + 2 => Priority::High,
> + _ => Priority::Critical,
> + }
> + }
> + }
> +
> + impl From<Priority> for u16 {
> + fn from(p: Priority) -> u16 {
> + p as u16
> + }
> + }
> +
> + bitfield! {
> + struct TestPageTableEntry(u64) {
> + 0:0 present as bool;
> + 1:1 writable as bool;
> + 11:9 available as u8;
> + 13:12 mem_type as u8 ?=> MemoryType;
> + 17:14 extended_type as u8 ?=> MemoryType; // For testing failures
> + 51:12 pfn as u64;
> + 51:12 pfn_overlap as u64;
> + 61:52 available2 as u16;
> + }
> + }
> +
> + bitfield! {
> + struct TestControlRegister(u16) {
> + 0:0 enable as bool;
> + 3:1 mode as u8;
> + 5:4 priority as u8 => Priority;
> + 7:4 priority_nibble as u8;
> + 15:8 channel as u8;
> + }
> + }
> +
> + bitfield! {
> + struct TestStatusRegister(u8) {
> + 0:0 ready as bool;
> + 1:1 error as bool;
> + 3:2 state as u8;
> + 7:4 reserved as u8;
> + 7:0 full_byte as u8; // For entire register
> + }
> + }
> +
> + #[test]
> + fn test_single_bits() {
> + let mut pte = TestPageTableEntry::default();
> +
> + assert!(!pte.present());
> + assert!(!pte.writable());
> +
> + pte = pte.set_present(true);
> + assert!(pte.present());
> +
> + pte = pte.set_writable(true);
> + assert!(pte.writable());
> +
> + pte = pte.set_writable(false);
> + assert!(!pte.writable());
> +
> + assert_eq!(pte.available(), 0);
> + pte = pte.set_available(0x5);
> + assert_eq!(pte.available(), 0x5);
I'd suggest testing the actual raw value of the register on top of
invoking the getter. That way you also test that:
- The right field is actually written (i.e. if the offset is off by one,
the getter will return the expected result even though the bitfield
has the wrong value),
- No other field has been affected.
So something like:
pte = pte.set_present(true);
assert!(pte.present());
assert(pte.into(), 0x1u64);
pte = pte.set_writable(true);
assert!(pte.writable());
assert(pte.into(), 0x3u64);
It might look a bit gross, but it is ok since these are not doctests
that users are going to take as a reference, so we case improve test
coverage at the detriment of readability.
On 10/1/2025 9:41 PM, Alexandre Courbot wrote:
> On Tue Sep 30, 2025 at 11:45 PM JST, Joel Fernandes wrote:
>> Add KUNIT tests to make sure the macro is working correctly.
>>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> ---
>> rust/kernel/bitfield.rs | 321 ++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 321 insertions(+)
>>
>> diff --git a/rust/kernel/bitfield.rs b/rust/kernel/bitfield.rs
>> index fed19918c3b9..9a20bcd2eb60 100644
>> --- a/rust/kernel/bitfield.rs
>> +++ b/rust/kernel/bitfield.rs
>> @@ -402,3 +402,324 @@ fn default() -> Self {
>> }
>> };
>> }
>> +
>> +#[::kernel::macros::kunit_tests(kernel_bitfield)]
>> +mod tests {
>> + use core::convert::TryFrom;
>> +
>> + // Enum types for testing => and ?=> conversions
>> + #[derive(Debug, Clone, Copy, PartialEq)]
>> + enum MemoryType {
>> + Unmapped = 0,
>> + Normal = 1,
>> + Device = 2,
>> + Reserved = 3,
>> + }
>> +
>> + impl Default for MemoryType {
>> + fn default() -> Self {
>> + MemoryType::Unmapped
>> + }
>> + }
>
> Tip: you can add `Default` to the `#[derive]` marker of `MemoryType` and
> mark the variant you want as default with `#[default]` instead of
> providing a full impl block:
>
> #[derive(Debug, Default, Clone, Copy, PartialEq)]
> enum MemoryType {
> #[default]
> Unmapped = 0,
> Normal = 1,
> Device = 2,
> Reserved = 3,
> }
>
Good point, changed to this.
>> +
>> + impl TryFrom<u8> for MemoryType {
>> + type Error = u8;
>> + fn try_from(value: u8) -> Result<Self, Self::Error> {
>> + match value {
>> + 0 => Ok(MemoryType::Unmapped),
>> + 1 => Ok(MemoryType::Normal),
>> + 2 => Ok(MemoryType::Device),
>> + 3 => Ok(MemoryType::Reserved),
>> + _ => Err(value),
>> + }
>> + }
>> + }
>> +
>> + impl From<MemoryType> for u64 {
>> + fn from(mt: MemoryType) -> u64 {
>> + mt as u64
>> + }
>> + }
>> +
>> + #[derive(Debug, Clone, Copy, PartialEq)]
>> + enum Priority {
>> + Low = 0,
>> + Medium = 1,
>> + High = 2,
>> + Critical = 3,
>> + }
>> +
>> + impl Default for Priority {
>> + fn default() -> Self {
>> + Priority::Low
>> + }
>> + }
>> +
>> + impl From<u8> for Priority {
>> + fn from(value: u8) -> Self {
>> + match value & 0x3 {
>> + 0 => Priority::Low,
>> + 1 => Priority::Medium,
>> + 2 => Priority::High,
>> + _ => Priority::Critical,
>> + }
>> + }
>> + }
>> +
>> + impl From<Priority> for u16 {
>> + fn from(p: Priority) -> u16 {
>> + p as u16
>> + }
>> + }
>> +
>> + bitfield! {
>> + struct TestPageTableEntry(u64) {
>> + 0:0 present as bool;
>> + 1:1 writable as bool;
>> + 11:9 available as u8;
>> + 13:12 mem_type as u8 ?=> MemoryType;
>> + 17:14 extended_type as u8 ?=> MemoryType; // For testing failures
>> + 51:12 pfn as u64;
>> + 51:12 pfn_overlap as u64;
>> + 61:52 available2 as u16;
>> + }
>> + }
>> +
>> + bitfield! {
>> + struct TestControlRegister(u16) {
>> + 0:0 enable as bool;
>> + 3:1 mode as u8;
>> + 5:4 priority as u8 => Priority;
>> + 7:4 priority_nibble as u8;
>> + 15:8 channel as u8;
>> + }
>> + }
>> +
>> + bitfield! {
>> + struct TestStatusRegister(u8) {
>> + 0:0 ready as bool;
>> + 1:1 error as bool;
>> + 3:2 state as u8;
>> + 7:4 reserved as u8;
>> + 7:0 full_byte as u8; // For entire register
>> + }
>> + }
>> +
>> + #[test]
>> + fn test_single_bits() {
>> + let mut pte = TestPageTableEntry::default();
>> +
>> + assert!(!pte.present());
>> + assert!(!pte.writable());
>> +
>> + pte = pte.set_present(true);
>> + assert!(pte.present());
>> +
>> + pte = pte.set_writable(true);
>> + assert!(pte.writable());
>> +
>> + pte = pte.set_writable(false);
>> + assert!(!pte.writable());
>> +
>> + assert_eq!(pte.available(), 0);
>> + pte = pte.set_available(0x5);
>> + assert_eq!(pte.available(), 0x5);
>
> I'd suggest testing the actual raw value of the register on top of
> invoking the getter. That way you also test that:
Sure, I am actually doing a raw check in a few other tests, but I could do so in
this one as well.
>
> - The right field is actually written (i.e. if the offset is off by one,
> the getter will return the expected result even though the bitfield
> has the wrong value),
> - No other field has been affected.
>
> So something like:
>
> pte = pte.set_present(true);
> assert!(pte.present());
> assert(pte.into(), 0x1u64);
>
> pte = pte.set_writable(true);
> assert!(pte.writable());
> assert(pte.into(), 0x3u64);
>
> It might look a bit gross, but it is ok since these are not doctests
> that users are going to take as a reference, so we case improve test
> coverage at the detriment of readability.
>
Ack. I will add these.
Thanks for the review! (I am assuming with these changes you're Ok with me
carrying your Reviewed-by tag on this patch as well, but please let me know if
there is a concern.)
- Joel
On Sat Oct 4, 2025 at 12:23 AM JST, Joel Fernandes wrote: >> - The right field is actually written (i.e. if the offset is off by one, >> the getter will return the expected result even though the bitfield >> has the wrong value), >> - No other field has been affected. >> >> So something like: >> >> pte = pte.set_present(true); >> assert!(pte.present()); >> assert(pte.into(), 0x1u64); >> >> pte = pte.set_writable(true); >> assert!(pte.writable()); >> assert(pte.into(), 0x3u64); >> >> It might look a bit gross, but it is ok since these are not doctests >> that users are going to take as a reference, so we case improve test >> coverage at the detriment of readability. >> > > Ack. I will add these. > > Thanks for the review! (I am assuming with these changes you're Ok with me > carrying your Reviewed-by tag on this patch as well, but please let me know if > there is a concern.) Please do not add tags that haven't been explicitly given. If we start assuming one another's stance about patches, the trust we can have in these tags is significantly reduced. Doing so also doesn't achieve anything in terms of efficiency; if I am ok with v3 I can give my Reviewed-by on it, and the tag can be picked up along with the patch when it is applied.
> On Oct 3, 2025, at 8:38 PM, Alexandre Courbot <acourbot@nvidia.com> wrote: > > On Sat Oct 4, 2025 at 12:23 AM JST, Joel Fernandes wrote: >>> - The right field is actually written (i.e. if the offset is off by one, >>> the getter will return the expected result even though the bitfield >>> has the wrong value), >>> - No other field has been affected. >>> >>> So something like: >>> >>> pte = pte.set_present(true); >>> assert!(pte.present()); >>> assert(pte.into(), 0x1u64); >>> >>> pte = pte.set_writable(true); >>> assert!(pte.writable()); >>> assert(pte.into(), 0x3u64); >>> >>> It might look a bit gross, but it is ok since these are not doctests >>> that users are going to take as a reference, so we case improve test >>> coverage at the detriment of readability. >>> >> >> Ack. I will add these. >> >> Thanks for the review! (I am assuming with these changes you're Ok with me >> carrying your Reviewed-by tag on this patch as well, but please let me know if >> there is a concern.) > > Please do not add tags that haven't been explicitly given. If we start > assuming one another's stance about patches, the trust we can have in > these tags is significantly reduced. Oh, I thought you told me you reviewed the patch privately, but consider the tag dropped. > > Doing so also doesn't achieve anything in terms of efficiency; if I am > ok with v3 I can give my Reviewed-by on it, and the tag can be picked up > along with the patch when it is applied. Well, it can be efficient. It really depends. I have been contributing upstream for about 15 years if you see the git log, often when someone chats privately with me like you did and they told me they are ok with a patch, I save them the trouble and add their review tag especially after they already added their tag to all my other patches. Surprisingly though this is probably the first time anyone has been pissed off about it. Anyway I will not add your tag henceforth unless you publicly reply (or you let me know otherwise by chat). Anyway thank you for the review of this patch, Joel
On Sat, Oct 4, 2025 at 6:14 PM Joel Fernandes <joelagnelf@nvidia.com> wrote: > > Well, it can be efficient. It really depends. I have been contributing upstream for about 15 years if you see the git log, often when someone chats privately with me like you did and they told me they are ok with a patch, I save them the trouble and add their review tag especially after they already added their tag to all my other patches. Surprisingly though this is probably the first time anyone has been pissed off about it. Anyway I will not add your tag henceforth unless you publicly reply (or you let me know otherwise by chat). If they just say they are OK with a patch, that would just mean an Acked-by, not a Reviewed-by. In any case, the documentation states those tags cannot be added without permission -- if someone gives you a tag privately, it is best that you tell them to please send it in the mailing list instead. That way there is no confusion and others (including tooling, e.g. patchwork and b4) can see it. Thanks! Cheers, Miguel
On 10/6/2025 12:40 PM, Miguel Ojeda wrote: > On Sat, Oct 4, 2025 at 6:14 PM Joel Fernandes <joelagnelf@nvidia.com> wrote: >> >> Well, it can be efficient. It really depends. I have been contributing upstream for about 15 years if you see the git log, often when someone chats privately with me like you did and they told me they are ok with a patch, I save them the trouble and add their review tag especially after they already added their tag to all my other patches. Surprisingly though this is probably the first time anyone has been pissed off about it. Anyway I will not add your tag henceforth unless you publicly reply (or you let me know otherwise by chat). > > If they just say they are OK with a patch, that would just mean an > Acked-by, not a Reviewed-by. >> In any case, the documentation states those tags cannot be added > without permission -- if someone gives you a tag privately, it is best > that you tell them to please send it in the mailing list instead. That > way there is no confusion and others (including tooling, e.g. > patchwork and b4) can see it. Sure, certainly it goes against docs/guidelines to add a tag *without permission*. I don't think anyone disputed that? I don't think I was advocating adding RB tags randomly without consent at all - please don't get me wrong. :-) And no doubts that publicly replying with RB tags is the best way. cheers, - Joel
On 10/2/25 1:41 AM, Alexandre Courbot wrote:
> On Tue Sep 30, 2025 at 11:45 PM JST, Joel Fernandes wrote:
>> Add KUNIT tests to make sure the macro is working correctly.
>>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> ---
>> rust/kernel/bitfield.rs | 321 ++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 321 insertions(+)
>>
>> diff --git a/rust/kernel/bitfield.rs b/rust/kernel/bitfield.rs
>> index fed19918c3b9..9a20bcd2eb60 100644
>> --- a/rust/kernel/bitfield.rs
>> +++ b/rust/kernel/bitfield.rs
>> @@ -402,3 +402,324 @@ fn default() -> Self {
>> }
>> };
>> }
>> +
>> +#[::kernel::macros::kunit_tests(kernel_bitfield)]
>> +mod tests {
>> + use core::convert::TryFrom;
>> +
>> + // Enum types for testing => and ?=> conversions
>> + #[derive(Debug, Clone, Copy, PartialEq)]
>> + enum MemoryType {
>> + Unmapped = 0,
>> + Normal = 1,
>> + Device = 2,
>> + Reserved = 3,
>> + }
>> +
>> + impl Default for MemoryType {
>> + fn default() -> Self {
>> + MemoryType::Unmapped
>> + }
>> + }
> Tip: you can add `Default` to the `#[derive]` marker of `MemoryType` and
> mark the variant you want as default with `#[default]` instead of
> providing a full impl block:
>
> #[derive(Debug, Default, Clone, Copy, PartialEq)]
> enum MemoryType {
> #[default]
> Unmapped = 0,
> Normal = 1,
> Device = 2,
> Reserved = 3,
> }
I would alternatively recommend to provide a `MemoryType::new` impl with
a `const` definition:
```rust
impl MemoryType {
pub const fn new() -> Self {
Self::Unmapped
}
}
impl Default for MemoryType {
fn default() -> Self {
Self::new()
}
}
```
This pattern allows using `MemoryType::new()` in `const` contexts, while
also providing the `Default` impl using the same default. It's somewhat
of a workaround until we get `const` traits.
>> +
>> + impl TryFrom<u8> for MemoryType {
>> + type Error = u8;
>> + fn try_from(value: u8) -> Result<Self, Self::Error> {
>> + match value {
>> + 0 => Ok(MemoryType::Unmapped),
>> + 1 => Ok(MemoryType::Normal),
>> + 2 => Ok(MemoryType::Device),
>> + 3 => Ok(MemoryType::Reserved),
>> + _ => Err(value),
>> + }
>> + }
>> + }
>> +
>> + impl From<MemoryType> for u64 {
>> + fn from(mt: MemoryType) -> u64 {
>> + mt as u64
>> + }
>> + }
>> +
>> + #[derive(Debug, Clone, Copy, PartialEq)]
>> + enum Priority {
>> + Low = 0,
>> + Medium = 1,
>> + High = 2,
>> + Critical = 3,
>> + }
>> +
>> + impl Default for Priority {
>> + fn default() -> Self {
>> + Priority::Low
>> + }
>> + }
>> +
>> + impl From<u8> for Priority {
>> + fn from(value: u8) -> Self {
>> + match value & 0x3 {
>> + 0 => Priority::Low,
>> + 1 => Priority::Medium,
>> + 2 => Priority::High,
>> + _ => Priority::Critical,
>> + }
>> + }
>> + }
>> +
>> + impl From<Priority> for u16 {
>> + fn from(p: Priority) -> u16 {
>> + p as u16
>> + }
>> + }
>> +
>> + bitfield! {
>> + struct TestPageTableEntry(u64) {
>> + 0:0 present as bool;
>> + 1:1 writable as bool;
>> + 11:9 available as u8;
>> + 13:12 mem_type as u8 ?=> MemoryType;
>> + 17:14 extended_type as u8 ?=> MemoryType; // For testing failures
>> + 51:12 pfn as u64;
>> + 51:12 pfn_overlap as u64;
>> + 61:52 available2 as u16;
>> + }
>> + }
>> +
>> + bitfield! {
>> + struct TestControlRegister(u16) {
>> + 0:0 enable as bool;
>> + 3:1 mode as u8;
>> + 5:4 priority as u8 => Priority;
>> + 7:4 priority_nibble as u8;
>> + 15:8 channel as u8;
>> + }
>> + }
>> +
>> + bitfield! {
>> + struct TestStatusRegister(u8) {
>> + 0:0 ready as bool;
>> + 1:1 error as bool;
>> + 3:2 state as u8;
>> + 7:4 reserved as u8;
>> + 7:0 full_byte as u8; // For entire register
>> + }
>> + }
>> +
>> + #[test]
>> + fn test_single_bits() {
>> + let mut pte = TestPageTableEntry::default();
>> +
>> + assert!(!pte.present());
>> + assert!(!pte.writable());
>> +
>> + pte = pte.set_present(true);
>> + assert!(pte.present());
>> +
>> + pte = pte.set_writable(true);
>> + assert!(pte.writable());
>> +
>> + pte = pte.set_writable(false);
>> + assert!(!pte.writable());
>> +
>> + assert_eq!(pte.available(), 0);
>> + pte = pte.set_available(0x5);
>> + assert_eq!(pte.available(), 0x5);
> I'd suggest testing the actual raw value of the register on top of
> invoking the getter. That way you also test that:
>
> - The right field is actually written (i.e. if the offset is off by one,
> the getter will return the expected result even though the bitfield
> has the wrong value),
> - No other field has been affected.
>
> So something like:
>
> pte = pte.set_present(true);
> assert!(pte.present());
> assert(pte.into(), 0x1u64);
>
> pte = pte.set_writable(true);
> assert!(pte.writable());
> assert(pte.into(), 0x3u64);
>
> It might look a bit gross, but it is ok since these are not doctests
> that users are going to take as a reference, so we case improve test
> coverage at the detriment of readability.
>
On Thu Oct 2, 2025 at 11:16 AM JST, Elle Rhumsaa wrote:
> On 10/2/25 1:41 AM, Alexandre Courbot wrote:
>
>> On Tue Sep 30, 2025 at 11:45 PM JST, Joel Fernandes wrote:
>>> Add KUNIT tests to make sure the macro is working correctly.
>>>
>>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>>> ---
>>> rust/kernel/bitfield.rs | 321 ++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 321 insertions(+)
>>>
>>> diff --git a/rust/kernel/bitfield.rs b/rust/kernel/bitfield.rs
>>> index fed19918c3b9..9a20bcd2eb60 100644
>>> --- a/rust/kernel/bitfield.rs
>>> +++ b/rust/kernel/bitfield.rs
>>> @@ -402,3 +402,324 @@ fn default() -> Self {
>>> }
>>> };
>>> }
>>> +
>>> +#[::kernel::macros::kunit_tests(kernel_bitfield)]
>>> +mod tests {
>>> + use core::convert::TryFrom;
>>> +
>>> + // Enum types for testing => and ?=> conversions
>>> + #[derive(Debug, Clone, Copy, PartialEq)]
>>> + enum MemoryType {
>>> + Unmapped = 0,
>>> + Normal = 1,
>>> + Device = 2,
>>> + Reserved = 3,
>>> + }
>>> +
>>> + impl Default for MemoryType {
>>> + fn default() -> Self {
>>> + MemoryType::Unmapped
>>> + }
>>> + }
>> Tip: you can add `Default` to the `#[derive]` marker of `MemoryType` and
>> mark the variant you want as default with `#[default]` instead of
>> providing a full impl block:
>>
>> #[derive(Debug, Default, Clone, Copy, PartialEq)]
>> enum MemoryType {
>> #[default]
>> Unmapped = 0,
>> Normal = 1,
>> Device = 2,
>> Reserved = 3,
>> }
>
> I would alternatively recommend to provide a `MemoryType::new` impl with
> a `const` definition:
>
> ```rust
> impl MemoryType {
> pub const fn new() -> Self {
>
> Self::Unmapped
>
> }
> }
>
> impl Default for MemoryType {
> fn default() -> Self {
> Self::new()
> }
> }
> ```
>
> This pattern allows using `MemoryType::new()` in `const` contexts, while
> also providing the `Default` impl using the same default. It's somewhat
> of a workaround until we get `const` traits.
That's an elegant pattern generally speaking, but I don't think we would
benefit from using it in these unit tests.
On 10/2/25 2:51 AM, Alexandre Courbot wrote:
> On Thu Oct 2, 2025 at 11:16 AM JST, Elle Rhumsaa wrote:
>> On 10/2/25 1:41 AM, Alexandre Courbot wrote:
>>
>>> On Tue Sep 30, 2025 at 11:45 PM JST, Joel Fernandes wrote:
>>>> Add KUNIT tests to make sure the macro is working correctly.
>>>>
>>>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>>>> ---
>>>> rust/kernel/bitfield.rs | 321 ++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 321 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/bitfield.rs b/rust/kernel/bitfield.rs
>>>> index fed19918c3b9..9a20bcd2eb60 100644
>>>> --- a/rust/kernel/bitfield.rs
>>>> +++ b/rust/kernel/bitfield.rs
>>>> @@ -402,3 +402,324 @@ fn default() -> Self {
>>>> }
>>>> };
>>>> }
>>>> +
>>>> +#[::kernel::macros::kunit_tests(kernel_bitfield)]
>>>> +mod tests {
>>>> + use core::convert::TryFrom;
>>>> +
>>>> + // Enum types for testing => and ?=> conversions
>>>> + #[derive(Debug, Clone, Copy, PartialEq)]
>>>> + enum MemoryType {
>>>> + Unmapped = 0,
>>>> + Normal = 1,
>>>> + Device = 2,
>>>> + Reserved = 3,
>>>> + }
>>>> +
>>>> + impl Default for MemoryType {
>>>> + fn default() -> Self {
>>>> + MemoryType::Unmapped
>>>> + }
>>>> + }
>>> Tip: you can add `Default` to the `#[derive]` marker of `MemoryType` and
>>> mark the variant you want as default with `#[default]` instead of
>>> providing a full impl block:
>>>
>>> #[derive(Debug, Default, Clone, Copy, PartialEq)]
>>> enum MemoryType {
>>> #[default]
>>> Unmapped = 0,
>>> Normal = 1,
>>> Device = 2,
>>> Reserved = 3,
>>> }
>> I would alternatively recommend to provide a `MemoryType::new` impl with
>> a `const` definition:
>>
>> ```rust
>> impl MemoryType {
>> pub const fn new() -> Self {
>>
>> Self::Unmapped
>>
>> }
>> }
>>
>> impl Default for MemoryType {
>> fn default() -> Self {
>> Self::new()
>> }
>> }
>> ```
>>
>> This pattern allows using `MemoryType::new()` in `const` contexts, while
>> also providing the `Default` impl using the same default. It's somewhat
>> of a workaround until we get `const` traits.
> That's an elegant pattern generally speaking, but I don't think we would
> benefit from using it in these unit tests.
*facepalm* right, I lost the context that these data structures were
KUNIT-specific. Please disregard.
© 2016 - 2026 Red Hat, Inc.