Clippy no longer complains about this lint.

Signed-off-by: Onur Özkan <work@onurozkan.dev>
---
 rust/kernel/cpufreq.rs | 1 -
 1 file changed, 1 deletion(-)

diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index 11b03e9d7e89..97de9b0573da 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -907,7 +907,6 @@ fn register_em(_policy: &mut Policy) {
 /// or CPUs, so it is safe to share it.
 unsafe impl<T: Driver> Sync for Registration<T> {}

-#[allow(clippy::non_send_fields_in_send_ty)]
 /// SAFETY: Registration with and unregistration from the cpufreq subsystem can happen from any
 /// thread.
 unsafe impl<T: Driver> Send for Registration<T> {}
--
2.50.0

On Sat, Jun 28, 2025 at 6:10 AM Onur Özkan <work@onurozkan.dev> wrote:
>
> Clippy no longer complains about this lint.

Do you have more context? For instance, do you know since when it no
longer complains, or why was the reason for the change? i.e. why we
had the `allow` in the first place, so that we know we don't need it
anymore?

For instance, please how I reasoned about it in commit 5e7c9b84ad08
("rust: sync: remove unneeded
`#[allow(clippy::non_send_fields_in_send_ty)]`").

(It may happen to be the same reason, or not.)

Thanks!

Cheers,
Miguel

On Sat, 28 Jun 2025 09:13:50 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Sat, Jun 28, 2025 at 6:10 AM Onur Özkan <work@onurozkan.dev> wrote:
> >
> > Clippy no longer complains about this lint.
> 
> Do you have more context? For instance, do you know since when it no
> longer complains, or why was the reason for the change? i.e. why we
> had the `allow` in the first place, so that we know we don't need it
> anymore?
> 
> For instance, please how I reasoned about it in commit 5e7c9b84ad08
> ("rust: sync: remove unneeded
> `#[allow(clippy::non_send_fields_in_send_ty)]`").
> 
> (It may happen to be the same reason, or not.)
> 
> Thanks!
> 
> Cheers,
> Miguel

It doesn't seem to be the same reason. I rebased over
c6af9a1191d042839e56abff69e8b0302d117988 (the exact commit where that
lint was added) but still Clippy did not complain about it on the
MSRV. So it was either a leftover, or there is a version between
1.78 and the current stable where Clippy did complain. I can dig into it
more during the week if you would like.

IMO, we should require people to add a comment explaining the reason
for adding these lint rules to the codebase. It would make both reading
and modifying the code much simpler and clearer.

Regards,
Onur

On Sat, Jun 28, 2025 at 12:30 PM Onur <work@onurozkan.dev> wrote:
>
> It doesn't seem to be the same reason. I rebased over
> c6af9a1191d042839e56abff69e8b0302d117988 (the exact commit where that
> lint was added) but still Clippy did not complain about it on the
> MSRV. So it was either a leftover, or there is a version between
> 1.78 and the current stable where Clippy did complain. I can dig into it
> more during the week if you would like.

Are you sure? The lint is actually disabled, as I mention in 5e7c9b84ad08.

From a quick test, I enabled it in that file, and I get the warning.

Thus it seems to me Clippy would still complain about it just fine.

It doesn't mean we shouldn't remove it, though.

> IMO, we should require people to add a comment explaining the reason
> for adding these lint rules to the codebase. It would make both reading
> and modifying the code much simpler and clearer.

Do you mean using the lint reasons feature? IIRC we discussed at some
point doing that when the feature was added (we enabled it for the
`expect` side of things).

For non-obvious cases or uncommon lints, it would be definitely nice
(a comment is also OK). I am not sure if it is worth enforcing it for
every single case, though.

It would be nice if `clippy::allow_attributes_without_reason` could be
enabled just for `allow`, or ignore it for certain lints.

Cheers,
Miguel

On Sat, 28 Jun 2025 14:18:53 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:


> > IMO, we should require people to add a comment explaining the reason
> > for adding these lint rules to the codebase. It would make both
> > reading and modifying the code much simpler and clearer.
> 
> Do you mean using the lint reasons feature? IIRC we discussed at some
> point doing that when the feature was added (we enabled it for the
> `expect` side of things).

Yeah, I meant that it't taking more effort than it should, like digging
through historical changes in the relevant parts of the source code,
trying to figuring out whether it was just a false positive or if there
was a specific reason behind it, etc.

On Sat, Jun 28, 2025 at 3:04 PM Onur <work@onurozkan.dev> wrote:
>
> Yeah, I meant that it't taking more effort than it should, like digging
> through historical changes in the relevant parts of the source code,
> trying to figuring out whether it was just a false positive or if there
> was a specific reason behind it, etc.

Yeah, that is a big part of kernel development, especially on the
maintenance side :)

I definitely agree that a good comment in the source code is better
than going through Git history, and the kernel sometimes has had some
things documented in the Git log that should have been in the source
code instead. It happens.

However, in some cases like this one it is not clear it would help.
For instance, here the lint reason message could have been something
that made sense back then when the lint was enabled, and yet we would
still have had to notice the lint got disabled later on, so we would
end up still going into the Git log.

`expect` is great to mitigate some of that -- sadly we cannot use it
as much as we would like due to sometimes being conditional to an arch
or the kernel config or the Rust version. (And your first patch may
have some cases that perhaps we cannot convert due to that -- I didn't
check)

Cheers,
Miguel

On Sat, 28 Jun 2025 14:18:53 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Sat, Jun 28, 2025 at 12:30 PM Onur <work@onurozkan.dev> wrote:
> >
> > It doesn't seem to be the same reason. I rebased over
> > c6af9a1191d042839e56abff69e8b0302d117988 (the exact commit where
> > that lint was added) but still Clippy did not complain about it on
> > the MSRV. So it was either a leftover, or there is a version between
> > 1.78 and the current stable where Clippy did complain. I can dig
> > into it more during the week if you would like.
> 
> Are you sure? The lint is actually disabled, as I mention in
> 5e7c9b84ad08.
> 
> From a quick test, I enabled it in that file, and I get the warning.
> 
> Thus it seems to me Clippy would still complain about it just fine.

Yes, I am sure. Just to clarify, I am not testing 5e7c9b84ad08. I am
testing c6af9a1191d042839e56abff69e8b0302d117988 where
`#[allow(clippy::non_send_fields_in_send_ty)]` was added on
`unsafe impl<T: Driver> Send for Registration<T> {}`.

Switching from `allow` to `expect` produced the following result on my
end:

```
$ make LLVM=1 -j $(nproc)
CLIPPY=1 DESCEND objtool
  CALL    scripts/checksyscalls.sh
  INSTALL libsubcmd_headers
  CLIPPY L rust/kernel.o
error: this lint expectation is unfulfilled
   --> rust/kernel/cpufreq.rs:908:10
    |
908 | #[expect(clippy::non_send_fields_in_send_ty)]
    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D unfulfilled-lint-expectations` implied by `-D warnings`
    = help: to override `-D warnings` add
`#[allow(unfulfilled_lint_expectations)]`

error: aborting due to 1 previous error

make[2]: *** [rust/Makefile:534: rust/kernel.o] Error 1
make[1]: *** [/home/nimda/devspace/onur-ozkan/linux/Makefile:1283:
prepare] Error 2 make: *** [Makefile:248: __sub-make] Error 2

$ git log -1
commit c6af9a1191d042839e56abff69e8b0302d117988 (HEAD -> master)
Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Wed Jan 24 12:36:33 2024 +0530

    rust: cpufreq: Extend abstractions for driver registration

    Extend the cpufreq abstractions to support driver registration from
    Rust.

    Reviewed-by: Danilo Krummrich <dakr@kernel.org>
    Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

$ rustc --version --verbose
rustc 1.78.0 (9b00956e5 2024-04-29)
binary: rustc
commit-hash: 9b00956e56009bab2aa15d7bff10916599e3d6d6
commit-date: 2024-04-29
host: x86_64-unknown-linux-gnu
release: 1.78.0
LLVM version: 18.1.2
```

Regards,
Onur

On Sat, Jun 28, 2025 at 2:42 PM Onur <work@onurozkan.dev> wrote:
>
> Yes, I am sure. Just to clarify, I am not testing 5e7c9b84ad08. I am
> testing c6af9a1191d042839e56abff69e8b0302d117988 where
> `#[allow(clippy::non_send_fields_in_send_ty)]` was added on
> `unsafe impl<T: Driver> Send for Registration<T> {}`.
>
> Switching from `allow` to `expect` produced the following result on my
> end:

Yes, of course it does -- what I am telling you (and what 5e7c9b84ad08
says) is that the lint is disabled.

And since it is disabled, if you change the line to `expect`, then it
will obviously complain.

If you actually enabled the lint with e.g.

    #![warn(clippy::non_send_fields_in_send_ty)]

at the top of the file, and then used `expect`, it will build fine.

Cheers,
Miguel

On Sat, 28 Jun 2025 14:51:15 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Sat, Jun 28, 2025 at 2:42 PM Onur <work@onurozkan.dev> wrote:
> >
> > Yes, I am sure. Just to clarify, I am not testing 5e7c9b84ad08. I am
> > testing c6af9a1191d042839e56abff69e8b0302d117988 where
> > `#[allow(clippy::non_send_fields_in_send_ty)]` was added on
> > `unsafe impl<T: Driver> Send for Registration<T> {}`.
> >
> > Switching from `allow` to `expect` produced the following result on
> > my end:
> 
> Yes, of course it does -- what I am telling you (and what 5e7c9b84ad08
> says) is that the lint is disabled.
> 
> And since it is disabled, if you change the line to `expect`, then it
> will obviously complain.
> 
> If you actually enabled the lint with e.g.
> 
>     #![warn(clippy::non_send_fields_in_send_ty)]
> 
> at the top of the file, and then used `expect`, it will build fine.


Aha, I see. I missed that. I guess `allow` was added when the author
had this lint enabled on their checkout, but their work was merged when
lint removal was merged before that.

Do you want me to update the patch description by including
5e7c9b84ad08 ref and send v4?

Sorry for misunderstanding by the way!

Many thanks,
Onur

On Sat, Jun 28, 2025 at 3:11 PM Onur <work@onurozkan.dev> wrote:
>
> Aha, I see. I missed that. I guess `allow` was added when the author
> had this lint enabled on their checkout, but their work was merged when
> lint removal was merged before that.

Yeah, some of the code going around was written years ago, so
sometimes this sort of thing happens. :)

> Do you want me to update the patch description by including
> 5e7c9b84ad08 ref and send v4?

Sure -- maybe wait a few days, to see if anyone says anything else.
Then we will need to wait for Acked-bys from other maintainers.

Or, actually, if you are sending a new version and you are willing to
do it, then it would be easier to land if you split the first patch
also by subsystem -- that way each maintainer can take their patches
on their own time instead. Since each patch is independent, you can
send them in independent patch series, that makes it even easier for
maintainers to track.

For instance, you could do this grouping:

     rust/kernel/error.rs                | 2 +-
     rust/kernel/types.rs                | 2 +-
     rust/macros/helpers.rs              | 2 +-
     (+ this patch #2)

     rust/kernel/init.rs                 | 6 +++---

     rust/kernel/kunit.rs                | 2 +-

     drivers/gpu/nova-core/regs.rs       | 2 +-
     rust/kernel/drm/ioctl.rs            | 8 ++++----

     rust/kernel/devres.rs               | 2 +-
     rust/kernel/driver.rs               | 2 +-

     rust/kernel/alloc/allocator_test.rs | 2 +-

     rust/kernel/opp.rs                  | 4 ++--
     (+ this patch #3)

So e.g. the top one (Rust) would be a series of 2 patches, then the
next one (pin-init) a single independent patch, and so on.

> Sorry for misunderstanding by the way!

No worries at all! It happens :)

And thanks for doing these cleanups! They are appreciated.

Cheers,
Miguel

On Sat, 28 Jun 2025 15:28:29 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Sat, Jun 28, 2025 at 3:11 PM Onur <work@onurozkan.dev> wrote:
> >
> > Aha, I see. I missed that. I guess `allow` was added when the author
> > had this lint enabled on their checkout, but their work was merged
> > when lint removal was merged before that.
> 
> Yeah, some of the code going around was written years ago, so
> sometimes this sort of thing happens. :)
> 
> > Do you want me to update the patch description by including
> > 5e7c9b84ad08 ref and send v4?
> 
> Sure -- maybe wait a few days, to see if anyone says anything else.
> Then we will need to wait for Acked-bys from other maintainers.
> 
> Or, actually, if you are sending a new version and you are willing to
> do it, then it would be easier to land if you split the first patch
> also by subsystem -- that way each maintainer can take their patches
> on their own time instead. Since each patch is independent, you can
> send them in independent patch series, that makes it even easier for
> maintainers to track.

I don't have enough time to do it right now, but I would be happy
to do it in ~3 days during the week (assuming it's still not being
reviewed by the maintainers by then).

Regards,
Onur

On Sat, Jun 28, 2025 at 3:42 PM Onur <work@onurozkan.dev> wrote:
>
> I don't have enough time to do it right now, but I would be happy
> to do it in ~3 days during the week (assuming it's still not being
> reviewed by the maintainers by then).

No worries, there is no rush on these cleanups. Even if it gets
reviewed, you can still split, keeping their tag. Thanks!

Cheers,
Miguel