Re: [PATCH v2] rust: platform: add Io support
From: Alice Ryhl
Date: Fri Nov 15 2024 - 09:56:09 EST
On Wed, Nov 13, 2024 at 8:43 PM Daniel Almeida
<daniel.almeida@xxxxxxxxxxxxx> wrote:
>
> Hi all, hopefully I did not butcher anything using b4 this time.
>
> See one question from me below.
>
> > On 13 Nov 2024, at 16:30, Daniel Almeida <daniel.almeida@xxxxxxxxxxxxx> wrote:
> > + pub(crate) fn resource(&self, resource: u32) -> Result<*mut bindings::resource> {
> > + // SAFETY: By the type invariants, we know that `self.ptr` is non-null and valid.
> > + let resource = unsafe {
> > + bindings::platform_get_resource(self.as_raw(), bindings::IORESOURCE_MEM, resource)
> > + };
> > + if !resource.is_null() {
> > + Ok(resource)
> > + } else {
> > + Err(EINVAL)
> > + }
> > + }
> > }
>
> Should this return an immutable reference instead of a pointer?
>
> Once the pointer is known not to be null, I think it should. So far, this
> is the only place in the kernel crate calling `platform_get_resource`, so
> there would be no risk of breaking the reference rules and it would make
> the API more ergonomic to use.
You should never create references to bare C structs. If you want a
reference, you'll need to create a wrapper Rust type that uses Opaque
so that Rust's assumptions about references are not applied to the C
struct.
Since you're just passing it on to a C api that expects a raw pointer,
I would say keep the raw pointer. I wouldn't go for the wrapper struct
unless you expect this to be part of the API used by drivers.
Alice