Re: [PATCH V8 12/14] rust: Extend cpufreq bindings for driver registration

Viresh Kumar
2025-02-07T09:15:37Z

On 06-02-25, 13:04, Danilo Krummrich wrote:
> > + unsafe { drop(KBox::from_raw(drv.attr)) };
>
> This could just be
>
> let _ = unsafe { KBox::from_raw(drv.attr) };
>
> At least drop() should not be within the unsafe block.
>
> > + }
> > +
> > + // Free data
> > + drop(self.clear_data());
>
> No need for drop(), but I also don't mind to be explicit.

For both of these I kept the explicit drop() to avoid any potential
confusion. I do prefer them.

--
viresh

-------------------------8<-------------------------

diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index ecf7c6e2cb89..d2e7913e170b 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -645,7 +645,7 @@ pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Resul

// Pass the ownership of the memory block to the C code. This will be freed when
// the [`Registration`] object goes out of scope.
- drv_ref.attr = KBox::leak(attr) as *mut _;
+ drv_ref.attr = KBox::into_raw(attr) as *mut _;

// Initialize mandatory callbacks.
drv_ref.init = Some(Self::init_callback);
@@ -813,7 +813,7 @@ fn clear_data(&mut self) -> Option<T::Data> {
// cpufreq driver callbacks.
impl<T: Driver> Registration<T> {
// Policy's init callback.
- extern "C" fn init_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+ extern "C" fn init_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
from_result(|| {
// SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
// the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -838,7 +838,7 @@ extern "C" fn exit_callback(ptr: *mut bindings::cpufreq_policy) {
}

// Policy's online callback.
- extern "C" fn online_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+ extern "C" fn online_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
from_result(|| {
// SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
// the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -849,7 +849,7 @@ extern "C" fn online_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::
}

// Policy's offline callback.
- extern "C" fn offline_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+ extern "C" fn offline_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
from_result(|| {
// SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
// the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -860,7 +860,7 @@ extern "C" fn offline_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi:
}

// Policy's suspend callback.
- extern "C" fn suspend_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+ extern "C" fn suspend_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
from_result(|| {
// SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
// the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -871,7 +871,7 @@ extern "C" fn suspend_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi:
}

// Policy's resume callback.
- extern "C" fn resume_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+ extern "C" fn resume_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
from_result(|| {
// SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
// the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -890,7 +890,7 @@ extern "C" fn ready_callback(ptr: *mut bindings::cpufreq_policy) {
}

// Policy's verify callback.
- extern "C" fn verify_callback(ptr: *mut bindings::cpufreq_policy_data) -> core::ffi::c_int {
+ extern "C" fn verify_callback(ptr: *mut bindings::cpufreq_policy_data) -> kernel::ffi::c_int {
from_result(|| {
// SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
// the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -901,7 +901,7 @@ extern "C" fn verify_callback(ptr: *mut bindings::cpufreq_policy_data) -> core::
}

// Policy's setpolicy callback.
- extern "C" fn setpolicy_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+ extern "C" fn setpolicy_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
from_result(|| {
// SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
// the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -916,7 +916,7 @@ extern "C" fn target_callback(
ptr: *mut bindings::cpufreq_policy,
target_freq: u32,
relation: u32,
- ) -> core::ffi::c_int {
+ ) -> kernel::ffi::c_int {
from_result(|| {
// SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
// the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -930,7 +930,7 @@ extern "C" fn target_callback(
extern "C" fn target_index_callback(
ptr: *mut bindings::cpufreq_policy,
index: u32,
- ) -> core::ffi::c_int {
+ ) -> kernel::ffi::c_int {
from_result(|| {
// SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
// the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -944,7 +944,7 @@ extern "C" fn target_index_callback(
extern "C" fn fast_switch_callback(
ptr: *mut bindings::cpufreq_policy,
target_freq: u32,
- ) -> core::ffi::c_uint {
+ ) -> kernel::ffi::c_uint {
// SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for the
// duration of this call, so it is guaranteed to remain alive for the lifetime of `ptr`.
let mut policy = unsafe { Policy::from_raw_policy(ptr) };
@@ -967,7 +967,7 @@ extern "C" fn adjust_perf_callback(
extern "C" fn get_intermediate_callback(
ptr: *mut bindings::cpufreq_policy,
index: u32,
- ) -> core::ffi::c_uint {
+ ) -> kernel::ffi::c_uint {
// SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for the
// duration of this call, so it is guaranteed to remain alive for the lifetime of `ptr`.
let mut policy = unsafe { Policy::from_raw_policy(ptr) };
@@ -978,7 +978,7 @@ extern "C" fn get_intermediate_callback(
extern "C" fn target_intermediate_callback(
ptr: *mut bindings::cpufreq_policy,
index: u32,
- ) -> core::ffi::c_int {
+ ) -> kernel::ffi::c_int {
from_result(|| {
// SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
// the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -989,7 +989,7 @@ extern "C" fn target_intermediate_callback(
}

// Policy's get callback.
- extern "C" fn get_callback(cpu: u32) -> core::ffi::c_uint {
+ extern "C" fn get_callback(cpu: u32) -> kernel::ffi::c_uint {
Policy::from_cpu(cpu).map_or(0, |mut policy| T::get(&mut policy).map_or(0, |f| f))
}

@@ -1001,7 +1001,7 @@ extern "C" fn update_limits_callback(cpu: u32) {
}

// Policy's bios_limit callback.
- extern "C" fn bios_limit_callback(cpu: i32, limit: *mut u32) -> core::ffi::c_int {
+ extern "C" fn bios_limit_callback(cpu: i32, limit: *mut u32) -> kernel::ffi::c_int {
from_result(|| {
let mut policy = Policy::from_cpu(cpu as u32)?;

@@ -1014,7 +1014,7 @@ extern "C" fn bios_limit_callback(cpu: i32, limit: *mut u32) -> core::ffi::c_int
extern "C" fn set_boost_callback(
ptr: *mut bindings::cpufreq_policy,
state: i32,
- ) -> core::ffi::c_int {
+ ) -> kernel::ffi::c_int {
from_result(|| {
// SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
// the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -1036,7 +1036,6 @@ extern "C" fn register_em_callback(ptr: *mut bindings::cpufreq_policy) {
impl<T: Driver> Drop for Registration<T> {
// Removes the registration from the kernel if it has completed successfully before.
fn drop(&mut self) {
- pr_info!("Registration dropped\n");
let drv = self.drv.get_mut();

// SAFETY: The driver was earlier registered from `new()`.
@@ -1044,8 +1043,8 @@ fn drop(&mut self) {

// Free the previously leaked memory to the C code.
if !drv.attr.is_null() {
- // SAFETY: The pointer was earlier initialized from the result of `KBox::leak`.
- unsafe { drop(KBox::from_raw(drv.attr)) };
+ // SAFETY: The pointer was earlier initialized from the result of `KBox::into_raw()`.
+ drop(unsafe { KBox::from_raw(drv.attr) });
}

// Free data
diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
index 4030953c2001..b83bd97a4f37 100644
--- a/rust/kernel/opp.rs
+++ b/rust/kernel/opp.rs
@@ -300,9 +300,9 @@ extern "C" fn config_clks(
dev: *mut bindings::device,
opp_table: *mut bindings::opp_table,
opp: *mut bindings::dev_pm_opp,
- _data: *mut core::ffi::c_void,
+ _data: *mut kernel::ffi::c_void,
scaling_down: bool,
- ) -> core::ffi::c_int {
+ ) -> kernel::ffi::c_int {
from_result(|| {
// SAFETY: 'dev' is guaranteed by the C code to be valid.
let dev = unsafe { Device::get_device(dev) };
@@ -324,8 +324,8 @@ extern "C" fn config_regulators(
old_opp: *mut bindings::dev_pm_opp,
new_opp: *mut bindings::dev_pm_opp,
regulators: *mut *mut bindings::regulator,
- count: core::ffi::c_uint,
- ) -> core::ffi::c_int {
+ count: kernel::ffi::c_uint,
+ ) -> kernel::ffi::c_int {
from_result(|| {
// SAFETY: 'dev' is guaranteed by the C code to be valid.
let dev = unsafe { Device::get_device(dev) };