Add a method to get a pointer to the data contained in an `Arc`.
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
rust/kernel/sync/arc.rs | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 3673496c2363..a57ea3e2b44c 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -258,6 +258,14 @@ pub fn into_raw(self) -> *const T {
unsafe { core::ptr::addr_of!((*ptr).data) }
}
+ /// Return a raw pointer to the data in this arc.
+ pub fn as_ptr(&self) -> *const T {
+ let ptr = self.ptr.as_ptr();
+ // SAFETY: As we derive the pointer from a reference above, the pointer
+ // must be valid.
+ unsafe { core::ptr::addr_of!((*ptr).data) }
+ }
+
/// Recreates an [`Arc`] instance previously deconstructed via [`Arc::into_raw`].
///
/// # Safety
--
2.46.0On 18.09.24 00:27, Andreas Hindborg wrote:
> Add a method to get a pointer to the data contained in an `Arc`.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/sync/arc.rs | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 3673496c2363..a57ea3e2b44c 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -258,6 +258,14 @@ pub fn into_raw(self) -> *const T {
> unsafe { core::ptr::addr_of!((*ptr).data) }
> }
>
> + /// Return a raw pointer to the data in this arc.
> + pub fn as_ptr(&self) -> *const T {
I don't know if we have a convention for this, but shouldn't this be an
associated function? Because if `T` also has an `as_ptr` function, it
will be shadowed by this one.
---
Cheers,
Benno
> + let ptr = self.ptr.as_ptr();
> + // SAFETY: As we derive the pointer from a reference above, the pointer
> + // must be valid.
> + unsafe { core::ptr::addr_of!((*ptr).data) }
> + }
> +
> /// Recreates an [`Arc`] instance previously deconstructed via [`Arc::into_raw`].
> ///
> /// # Safety
> --
> 2.46.0
>
>
On 19.09.2024 16:03, Benno Lossin wrote:
> On 18.09.24 00:27, Andreas Hindborg wrote:
>> Add a method to get a pointer to the data contained in an `Arc`.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>> rust/kernel/sync/arc.rs | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
>> index 3673496c2363..a57ea3e2b44c 100644
>> --- a/rust/kernel/sync/arc.rs
>> +++ b/rust/kernel/sync/arc.rs
>> @@ -258,6 +258,14 @@ pub fn into_raw(self) -> *const T {
>> unsafe { core::ptr::addr_of!((*ptr).data) }
>> }
>>
>> + /// Return a raw pointer to the data in this arc.
>> + pub fn as_ptr(&self) -> *const T {
>
> I don't know if we have a convention for this, but shouldn't this be an
> associated function? Because if `T` also has an `as_ptr` function, it
> will be shadowed by this one.
Yes. In Fabien's out of tree regmap we have an as_ptr() for Regmap [1]
which operates on &Arc<Regmap> [2]. Once this patch is applied to arc.rs
the compilation fails as then Arc.as_ptr() is used, not the
Regmap.as_ptr() any more [3]. Switching this to something like [4] makes
the compiler happy.
Thanks,
Dirk
P.S.: Just to learn something: For the unmodified, failing case: Is
there a rule when which as_ptr() will be used? Is there an order rule
for the shadowing? Any documentation link?
[1]
https://github.com/Fabo/linux/blob/fparent/rust-ncv6336/rust/kernel/regmap.rs#L71
[2]
https://github.com/Fabo/linux/blob/fparent/rust-ncv6336/rust/kernel/regulator/driver.rs#L418
[3]
error[E0308]: mismatched types
--> rust/kernel/regulator/driver.rs:420:33
|
420 | config.cfg.regmap = regmap.as_ptr();
| ^^^^^^^^^^^^^^^ types differ in
mutability
|
= note: expected raw pointer `*mut bindings::regmap`
found raw pointer `*const Regmap`
error: aborting due to 1 previous error
[4]
diff --git a/rust/kernel/hrtimer/arc.rs b/rust/kernel/hrtimer/arc.rs
index ff04b0b75bb39..7c39ab440e1c6 100644
--- a/rust/kernel/hrtimer/arc.rs
+++ b/rust/kernel/hrtimer/arc.rs
@@ -25,7 +25,7 @@ unsafe impl<U> TimerHandle for ArcTimerHandle<U>
U: HasTimer<U>,
{
fn cancel(&mut self) -> bool {
- let self_ptr = self.inner.as_ptr();
+ let self_ptr = Arc::as_ptr(&self.inner);
// SAFETY: As we obtained `self_ptr` from a valid reference
above, it
// must point to a valid `U`.
@@ -57,7 +57,7 @@ impl<U> TimerPointer for Arc<U>
fn schedule(self, expires: Ktime) -> ArcTimerHandle<U> {
// SAFETY: Since we generate the pointer passed to `schedule`
from a
// valid reference, it is a valid pointer.
- unsafe { U::schedule(self.as_ptr(), expires) };
+ unsafe { U::schedule(Arc::as_ptr(&self), expires) };
ArcTimerHandle { inner: self }
}
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 1466d9cd41652..0a314c2f4c5ea 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -259,8 +259,8 @@ pub fn into_raw(self) -> *const T {
}
/// Return a raw pointer to the data in this arc.
- pub fn as_ptr(&self) -> *const T {
- let ptr = self.ptr.as_ptr();
+ pub fn as_ptr(arc: &Self) -> *const T {
+ let ptr = arc.ptr.as_ptr();
// SAFETY: As we derive the pointer from a reference above,
the pointer
// must be valid.
unsafe { core::ptr::addr_of!((*ptr).data) }
On 01.10.24 06:56, Dirk Behme wrote:
> On 19.09.2024 16:03, Benno Lossin wrote:
>> On 18.09.24 00:27, Andreas Hindborg wrote:
>>> Add a method to get a pointer to the data contained in an `Arc`.
>>>
>>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>> ---
>>> rust/kernel/sync/arc.rs | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
>>> index 3673496c2363..a57ea3e2b44c 100644
>>> --- a/rust/kernel/sync/arc.rs
>>> +++ b/rust/kernel/sync/arc.rs
>>> @@ -258,6 +258,14 @@ pub fn into_raw(self) -> *const T {
>>> unsafe { core::ptr::addr_of!((*ptr).data) }
>>> }
>>>
>>> + /// Return a raw pointer to the data in this arc.
>>> + pub fn as_ptr(&self) -> *const T {
>>
>> I don't know if we have a convention for this, but shouldn't this be an
>> associated function? Because if `T` also has an `as_ptr` function, it
>> will be shadowed by this one.
>
> Yes. In Fabien's out of tree regmap we have an as_ptr() for Regmap [1]
> which operates on &Arc<Regmap> [2]. Once this patch is applied to arc.rs
> the compilation fails as then Arc.as_ptr() is used, not the
> Regmap.as_ptr() any more [3]. Switching this to something like [4] makes
> the compiler happy.
Yeah then we should switch to that.
> Thanks,
>
> Dirk
>
> P.S.: Just to learn something: For the unmodified, failing case: Is
> there a rule when which as_ptr() will be used? Is there an order rule
> for the shadowing? Any documentation link?
Yes, there is a page about the method call expression in the reference:
https://doc.rust-lang.org/reference/expressions/method-call-expr.html
---
Cheers,
Benno
> [1]
> https://github.com/Fabo/linux/blob/fparent/rust-ncv6336/rust/kernel/regmap.rs#L71
>
> [2]
> https://github.com/Fabo/linux/blob/fparent/rust-ncv6336/rust/kernel/regulator/driver.rs#L418
>
> [3]
>
> error[E0308]: mismatched types
> --> rust/kernel/regulator/driver.rs:420:33
> |
> 420 | config.cfg.regmap = regmap.as_ptr();
> | ^^^^^^^^^^^^^^^ types differ in
> mutability
> |
> = note: expected raw pointer `*mut bindings::regmap`
> found raw pointer `*const Regmap`
>
> error: aborting due to 1 previous error
>
> [4]
>
> diff --git a/rust/kernel/hrtimer/arc.rs b/rust/kernel/hrtimer/arc.rs
> index ff04b0b75bb39..7c39ab440e1c6 100644
> --- a/rust/kernel/hrtimer/arc.rs
> +++ b/rust/kernel/hrtimer/arc.rs
> @@ -25,7 +25,7 @@ unsafe impl<U> TimerHandle for ArcTimerHandle<U>
> U: HasTimer<U>,
> {
> fn cancel(&mut self) -> bool {
> - let self_ptr = self.inner.as_ptr();
> + let self_ptr = Arc::as_ptr(&self.inner);
>
> // SAFETY: As we obtained `self_ptr` from a valid reference
> above, it
> // must point to a valid `U`.
> @@ -57,7 +57,7 @@ impl<U> TimerPointer for Arc<U>
> fn schedule(self, expires: Ktime) -> ArcTimerHandle<U> {
> // SAFETY: Since we generate the pointer passed to `schedule`
> from a
> // valid reference, it is a valid pointer.
> - unsafe { U::schedule(self.as_ptr(), expires) };
> + unsafe { U::schedule(Arc::as_ptr(&self), expires) };
>
> ArcTimerHandle { inner: self }
> }
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 1466d9cd41652..0a314c2f4c5ea 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -259,8 +259,8 @@ pub fn into_raw(self) -> *const T {
> }
>
> /// Return a raw pointer to the data in this arc.
> - pub fn as_ptr(&self) -> *const T {
> - let ptr = self.ptr.as_ptr();
> + pub fn as_ptr(arc: &Self) -> *const T {
> + let ptr = arc.ptr.as_ptr();
> // SAFETY: As we derive the pointer from a reference above,
> the pointer
> // must be valid.
> unsafe { core::ptr::addr_of!((*ptr).data) }
>
>
On Thu, 19 Sep 2024 14:03:50 +0000
Benno Lossin <benno.lossin@proton.me> wrote:
> On 18.09.24 00:27, Andreas Hindborg wrote:
> > Add a method to get a pointer to the data contained in an `Arc`.
> >
> > Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> > ---
> > rust/kernel/sync/arc.rs | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > index 3673496c2363..a57ea3e2b44c 100644
> > --- a/rust/kernel/sync/arc.rs
> > +++ b/rust/kernel/sync/arc.rs
> > @@ -258,6 +258,14 @@ pub fn into_raw(self) -> *const T {
> > unsafe { core::ptr::addr_of!((*ptr).data) }
> > }
> >
> > + /// Return a raw pointer to the data in this arc.
> > + pub fn as_ptr(&self) -> *const T {
>
> I don't know if we have a convention for this, but shouldn't this be an
> associated function? Because if `T` also has an `as_ptr` function, it
> will be shadowed by this one.
The usual Rust convention is usually that if `Deref` is implemented,
then unless there's a good reason to do otherwise, associated function
should be used.
Best,
Gary
>
> ---
> Cheers,
> Benno
>
> > + let ptr = self.ptr.as_ptr();
> > + // SAFETY: As we derive the pointer from a reference above, the pointer
> > + // must be valid.
> > + unsafe { core::ptr::addr_of!((*ptr).data) }
> > + }
> > +
> > /// Recreates an [`Arc`] instance previously deconstructed via [`Arc::into_raw`].
> > ///
> > /// # Safety
> > --
> > 2.46.0
> >
> >
>
On Sat, Sep 21, 2024 at 5:58 PM Gary Guo <gary@garyguo.net> wrote:
>
> On Thu, 19 Sep 2024 14:03:50 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
>
> > On 18.09.24 00:27, Andreas Hindborg wrote:
> > > Add a method to get a pointer to the data contained in an `Arc`.
> > >
> > > Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> > > ---
> > > rust/kernel/sync/arc.rs | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > > index 3673496c2363..a57ea3e2b44c 100644
> > > --- a/rust/kernel/sync/arc.rs
> > > +++ b/rust/kernel/sync/arc.rs
> > > @@ -258,6 +258,14 @@ pub fn into_raw(self) -> *const T {
> > > unsafe { core::ptr::addr_of!((*ptr).data) }
> > > }
> > >
> > > + /// Return a raw pointer to the data in this arc.
> > > + pub fn as_ptr(&self) -> *const T {
> >
> > I don't know if we have a convention for this, but shouldn't this be an
> > associated function? Because if `T` also has an `as_ptr` function, it
> > will be shadowed by this one.
>
> The usual Rust convention is usually that if `Deref` is implemented,
> then unless there's a good reason to do otherwise, associated function
> should be used.
The reason for this convention is that adding new &self methods on a
smart pointer is a breaking change, but we don't care about breaking
changes, so it doesn't fully apply to us.
Not that we shouldn't follow the convention, but it's not completely
clear to me either.
Alice
On 21.09.24 17:58, Gary Guo wrote:
> On Thu, 19 Sep 2024 14:03:50 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
>
>> On 18.09.24 00:27, Andreas Hindborg wrote:
>>> Add a method to get a pointer to the data contained in an `Arc`.
>>>
>>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>> ---
>>> rust/kernel/sync/arc.rs | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
>>> index 3673496c2363..a57ea3e2b44c 100644
>>> --- a/rust/kernel/sync/arc.rs
>>> +++ b/rust/kernel/sync/arc.rs
>>> @@ -258,6 +258,14 @@ pub fn into_raw(self) -> *const T {
>>> unsafe { core::ptr::addr_of!((*ptr).data) }
>>> }
>>>
>>> + /// Return a raw pointer to the data in this arc.
>>> + pub fn as_ptr(&self) -> *const T {
>>
>> I don't know if we have a convention for this, but shouldn't this be an
>> associated function? Because if `T` also has an `as_ptr` function, it
>> will be shadowed by this one.
>
> The usual Rust convention is usually that if `Deref` is implemented,
> then unless there's a good reason to do otherwise, associated function
> should be used.
We don't do this for `into_raw` (ie it takes `self`), I will open a
good-first-issue to fix that then.
---
Cheers,
Benno
© 2016 - 2024 Red Hat, Inc.