drm/tyr: suppress unread field warnings

Currently these warnings, as errors, are preventing Tyr driver from building:

Quoted 3 lines · Deborah Brouwer

Currently these warnings, as errors, are preventing Tyr driver from building:

From what I understand, this is only reproducible with Rust 1.80.0 (unsupported upstream), and was fixed in Rust 1.80.1. Did you reproduce it elsewhere? I asked Mark where his toolchain comes to see what could be done there (e.g. in his distribution).

If it is just in Rust 1.80.0, and if a distribution still uses that one, and for some reason they don't want to use/backport the fix in 1.80.1, then we could consider something more global, i.e. ignoring that lint altogether for that version, since it also fails for another trait in bounded.rs (that is why I knew about this).

(By the way, in any case, we would need a proper explanation of this full context in the commit message.)

Thanks!

Cheers,
Miguel

From what I understand, this is only reproducible with Rust 1.80.0 (unsupported upstream), and was fixed in Rust 1.80.1. Did you reproduce it elsewhere? I asked Mark where his toolchain comes to see what could be done there (e.g. in his distribution).

If it is just in Rust 1.80.0, and if a distribution still uses that one, and for some reason they don't want to use/backport the fix in 1.80.1, then we could consider something more global, i.e. ignoring that lint altogether for that version, since it also fails for another trait in bounded.rs (that is why I knew about this).

Quoted 5 lines · Miguel Ojeda

From what I understand, this is only reproducible with Rust 1.80.0 (unsupported upstream), and was fixed in Rust 1.80.1. Did you reproduce it elsewhere? I asked Mark where his toolchain comes to see what could be done there (e.g. in his distribution).

Quoted 5 lines

If it is just in Rust 1.80.0, and if a distribution still uses that one, and for some reason they don't want to use/backport the fix in 1.80.1, then we could consider something more global, i.e. ignoring that lint altogether for that version, since it also fails for another trait in bounded.rs (that is why I knew about this).

As I said on the other thread I'm seeing this with what appears to be 1.85.0 in Debian stable.

If I run 1.85.0, I *instead* see the following ones from clippy:

CLIPPY [M] drivers/gpu/drm/tyr/tyr.o
warning: operator precedence can trip the unwary
--> drivers/gpu/drm/tyr/gpu.rs:51:13

   |
51 |             shader_present | u64::from(regs::GPU_SHADER_PRESENT_HI.read(dev, iomem)?) << 32;
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider parenthesizing your expression: `shader_present | (u64::from(regs::GPU_SHADER_PRESENT_HI.read(dev, iomem)?) << 32)`
   |

= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#precedence
= note: -W clippy::precedence implied by -W clippy::all
= help: to override -W clippy::all add #[allow(clippy::precedence)]

Quoted 6 lines · Danilo Krummrich

If I run 1.85.0, I *instead* see the following ones from clippy:

CLIPPY [M] drivers/gpu/drm/tyr/tyr.o
warning: operator precedence can trip the unwary
--> drivers/gpu/drm/tyr/gpu.rs:51:13

Quoted 7 lines

   |
51 |             shader_present | u64::from(regs::GPU_SHADER_PRESENT_HI.read(dev, iomem)?) << 32;
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider parenthesizing your expression: `shader_present | (u64::from(regs::GPU_SHADER_PRESENT_HI.read(dev, iomem)?) << 32)`
   |

= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#precedence
= note: -W clippy::precedence implied by -W clippy::all
= help: to override -W clippy::all add #[allow(clippy::precedence)]

I took a look at this -- Rust 1.85.0 extended the precedence lint to catch these, and then because it was too much, they split part of it into a new one called precedence_bits which is not enabled by default, which landed in Rust 1.86.0 (and is not in Rust 1.85.1, sadly).

    https://github.com/rust-lang/rust-clippy/pull/13743
    https://github.com/rust-lang/rust-clippy/issues/14097
    https://github.com/rust-lang/rust-clippy/pull/14115

I see others in the kernel have written similar expressions with parenthesis, e.g. for Rust in ioctl.rs, and I see a ton of those in C files too.

Personally, I don't mind seeing the parenthesis, but I know others may feel like it is obvious.

Now, seeing quite a bunch of parenthesis for this in C files, from kernel devs and crypto stuff and all over drivers and so on and so forth, means others definitely do not find it obvious or prefer to be explicit for extra clarity.

So we can just clean this one up and later enable precedence_bits for newer versions. Or if we decided it is too much, then I guess I could allow precedence on Rust 1.85.0 only.

Cc'ing others in case they want to give their opinion.

Having a lint that happens on just one random version is inconvenient, so I think we should take at least one of these two actions:

1. Disable clippy::precedence on 1.85.0 only.
2. Enable the clippy::precedence_bits lint.

But I am ok with enabling clippy::precedence_bits.

Quoted 4 lines · Alice Ryhl

Having a lint that happens on just one random version is inconvenient, so I think we should take at least one of these two actions:

Quoted 2 lines

1. Disable clippy::precedence on 1.85.0 only.
2. Enable the clippy::precedence_bits lint.

Yeah, in case it wasn't clear, those are the two options I mentioned.

Quoted 1 line

But I am ok with enabling clippy::precedence_bits.

Great, let's see how others feel about the parenthesis...

As I said on the other thread I'm seeing this with what appears to be 1.85.0 in Debian stable.

Currently these warnings, as errors, are preventing Tyr driver from building:

error: field device is never read
--> drivers/gpu/drm/tyr/driver.rs:37:5

   |
36 | pub(crate) struct TyrDriver {
   |                   --------- field in this struct
37 |     device: ARef<TyrDevice>,
   |     ^^^^^^
   |

= note: -D dead-code implied by -D warnings
= help: to override -D warnings add #[allow(dead_code)]

error: fields mali and sram are never read
--> drivers/gpu/drm/tyr/driver.rs:196:5

    |
195 | struct Regulators {
    |        ---------- fields in this struct
196 |     mali: Regulator<regulator::Enabled>,
    |     ^^^^
197 |     sram: Regulator<regulator::Enabled>,
    |     ^^^^

error: aborting due to 2 previous errors

Suppress these errors so that the Tyr driver will build.

Signed-off-by Deborah Brouwer <[email protected]>

Quoted 24 lines · Deborah Brouwer

Currently these warnings, as errors, are preventing Tyr driver from building:

error: field device is never read
--> drivers/gpu/drm/tyr/driver.rs:37:5

   |
36 | pub(crate) struct TyrDriver {
   |                   --------- field in this struct
37 |     device: ARef<TyrDevice>,
   |     ^^^^^^
   |

= note: -D dead-code implied by -D warnings
= help: to override -D warnings add #[allow(dead_code)]

error: fields mali and sram are never read
--> drivers/gpu/drm/tyr/driver.rs:196:5

    |
195 | struct Regulators {
    |        ---------- fields in this struct
196 |     mali: Regulator<regulator::Enabled>,
    |     ^^^^
197 |     sram: Regulator<regulator::Enabled>,
    |     ^^^^

Quoted 5 lines

error: aborting due to 2 previous errors

Suppress these errors so that the Tyr driver will build.

Signed-off-by Deborah Brouwer <[email protected]>

I still don't understand why I couldn't reproduce it myself, but assuming it's not just an 1.80.0 issue, below is my review:

Quoted 10 lines

 drivers/gpu/drm/tyr/driver.rs | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/tyr/driver.rs +0 -0

index 2a45d0288825..04c865cb4397 100644
@@ -34,6 +34,7 @@
 

drivers/gpu/drm/tyr/driver.rs line 38

Quoted 4 lines

drivers/gpu/drm/tyr/driver.rs +1 -0

 #[pin_data(PinnedDrop)]
 pub(crate) struct TyrDriver {
+    #[allow(dead_code)]
     device: ARef<TyrDevice>,

Let's use #[expect(dead_code)] so we remember to remove this when a user is added.

drivers/gpu/drm/tyr/driver.rs struct Clocks { · line 200

Quoted 11 lines

drivers/gpu/drm/tyr/driver.rs +2 -0

 }
 

@@ -193,6 +194,8 @@ struct Clocks {
 
 #[pin_data]
 struct Regulators {
+    #[allow(dead_code)]
     mali: Regulator<regulator::Enabled>,
+    #[allow(dead_code)]
     sram: Regulator<regulator::Enabled>,

I don't think we intend to ever use these fields - they exist only for their destructor. In that case, please prefix them with an underscore instead:

#[pin_data]
struct Regulators {
_mali: Regulator<regulator::Enabled>,
_sram: Regulator<regulator::Enabled>,
}

Alice

Quoted 25 lines · Alice Ryhl

Quoted 24 lines · Deborah Brouwer

Currently these warnings, as errors, are preventing Tyr driver from building:

error: field device is never read
--> drivers/gpu/drm/tyr/driver.rs:37:5

   |
36 | pub(crate) struct TyrDriver {
   |                   --------- field in this struct
37 |     device: ARef<TyrDevice>,
   |     ^^^^^^
   |

= note: -D dead-code implied by -D warnings
= help: to override -D warnings add #[allow(dead_code)]

error: fields mali and sram are never read
--> drivers/gpu/drm/tyr/driver.rs:196:5

    |
195 | struct Regulators {
    |        ---------- fields in this struct
196 |     mali: Regulator<regulator::Enabled>,
    |     ^^^^
197 |     sram: Regulator<regulator::Enabled>,
    |     ^^^^

Quoted 8 lines

Quoted 5 lines

error: aborting due to 2 previous errors

Suppress these errors so that the Tyr driver will build.

Signed-off-by Deborah Brouwer <[email protected]>

I still don't understand why I couldn't reproduce it myself, but assuming it's not just an 1.80.0 issue, below is my review:

I think the problem is not actually the rust compiler version, but commit "0242623384c7 rust: driver: let probe() return impl PinInit<Self, Error>"

Tyr probe() used to return a fully initialized Pin<KBox<Self>>, so the fields existed in an allocated struct which I suppose counted as “reading” the fields. But now Tyr probe() returns just a PinInit closure which doesn’t count as reading these fields.

Quoted 9 lines

Quoted 9 lines

 drivers/gpu/drm/tyr/driver.rs | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/tyr/driver.rs +0 -0

index 2a45d0288825..04c865cb4397 100644
@@ -34,6 +34,7 @@
 

Quoted 7 lines

drivers/gpu/drm/tyr/driver.rs line 38

Quoted 4 lines

drivers/gpu/drm/tyr/driver.rs +1 -0

 #[pin_data(PinnedDrop)]
 pub(crate) struct TyrDriver {
+    #[allow(dead_code)]
     device: ARef<TyrDevice>,

Let's use #[expect(dead_code)] so we remember to remove this when a user is added.

Using #[expect(dead_code)] also fails with 'unfulfilled lint expectation'. So I could keep #[allow(dead_code)] or maybe just use an underscore _driver too with a comment to explain the issue. What do you think?

Quoted 24 lines · Deborah Brouwer

Currently these warnings, as errors, are preventing Tyr driver from building:

error: field device is never read
--> drivers/gpu/drm/tyr/driver.rs:37:5

   |
36 | pub(crate) struct TyrDriver {
   |                   --------- field in this struct
37 |     device: ARef<TyrDevice>,
   |     ^^^^^^
   |

= note: -D dead-code implied by -D warnings
= help: to override -D warnings add #[allow(dead_code)]

error: fields mali and sram are never read
--> drivers/gpu/drm/tyr/driver.rs:196:5

    |
195 | struct Regulators {
    |        ---------- fields in this struct
196 |     mali: Regulator<regulator::Enabled>,
    |     ^^^^
197 |     sram: Regulator<regulator::Enabled>,
    |     ^^^^

Quoted 5 lines

error: aborting due to 2 previous errors

Suppress these errors so that the Tyr driver will build.

Signed-off-by Deborah Brouwer <[email protected]>

I still don't understand why I couldn't reproduce it myself, but assuming it's not just an 1.80.0 issue, below is my review:

drivers/gpu/drm/tyr/driver.rs line 38

Quoted 13 lines

 drivers/gpu/drm/tyr/driver.rs | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/tyr/driver.rs +1 -0

index 2a45d0288825..04c865cb4397 100644
@@ -34,6 +34,7 @@
 
 #[pin_data(PinnedDrop)]
 pub(crate) struct TyrDriver {
+    #[allow(dead_code)]
     device: ARef<TyrDevice>,

Let's use #[expect(dead_code)] so we remember to remove this when a user is added.

drivers/gpu/drm/tyr/driver.rs struct Clocks { · line 200

Quoted 10 lines

drivers/gpu/drm/tyr/driver.rs +2 -0

 }
 

@@ -193,6 +194,8 @@ struct Clocks {
 
 #[pin_data]
 struct Regulators {
+    #[allow(dead_code)]
     mali: Regulator<regulator::Enabled>,
+    #[allow(dead_code)]
     sram: Regulator<regulator::Enabled>,

I don't think we intend to ever use these fields - they exist only for their destructor. In that case, please prefix them with an underscore instead:

#[pin_data]
struct Regulators {
_mali: Regulator<regulator::Enabled>,
_sram: Regulator<regulator::Enabled>,
}

Quoted 12 lines · Alice Ryhl

drivers/gpu/drm/tyr/driver.rs struct Clocks { · line 200

Quoted 6 lines · Deborah Brouwer

#[pin_data]
struct Regulators {

drivers/gpu/drm/tyr/driver.rs +2 -0

+    #[allow(dead_code)]
     mali: Regulator<regulator::Enabled>,
+    #[allow(dead_code)]
     sram: Regulator<regulator::Enabled>,

I don't think we intend to ever use these fields - they exist only for their destructor. In that case, please prefix them with an underscore instead:

Quoted 5 lines

#[pin_data]
struct Regulators {
_mali: Regulator<regulator::Enabled>,
_sram: Regulator<regulator::Enabled>,
}

I wonder if it makes sense to request a feature to mute dead_code lint on certain types which we know are there to represent a registration or resource enablement.

Currently rustc's dead_code lint has a builtin exception for PhantomData, but I think a lot other types should have the same treatment, this can be useful even for other core types, e.g PhantomPinned.

I wonder if it makes sense to request a feature to mute dead_code lint on certain types which we know are there to represent a registration or resource enablement.

Currently rustc's dead_code lint has a builtin exception for PhantomData, but I think a lot other types should have the same treatment, this can be useful even for other core types, e.g PhantomPinned.