Re: [PATCH v2] rust: platform: add Io support

[Alice Ryhl]

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