Message-ID: <CANeycqqBVyDa=e+heDM-Wa9Bz30PESdMJ8Eu0OCPLh4Kqv+xwQ@mail.gmail.com>
Date:   Thu, 9 Mar 2023 16:06:06 -0300
From:   Wedson Almeida Filho <wedsonaf@...il.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Asahi Lina <lina@...hilina.net>, Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Joerg Roedel <joro@...tes.org>,
        Hector Martin <marcan@...can.st>,
        Sven Peter <sven@...npeter.dev>, Arnd Bergmann <arnd@...db.de>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>,
        Neal Gompa <neal@...pa.dev>, rust-for-linux@...r.kernel.org,
        linux-kernel@...r.kernel.org, asahi@...ts.linux.dev
Subject: Re: [PATCH 5/5] rust: device: Add a stub abstraction for devices

On Thu, 9 Mar 2023 at 14:11, Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> On Thu, Mar 09, 2023 at 01:46:39PM -0300, Wedson Almeida Filho wrote:
> > On Thu, 9 Mar 2023 at 08:24, Greg Kroah-Hartman
> > <gregkh@...uxfoundation.org> wrote:
> > > > > > +        // owns a reference. This is satisfied by the call to `get_device` above.
> > > > > > +        Self { ptr }
> > > > > > +    }
> > > > > > +
> > > > > > +    /// Creates a new device instance from an existing [`RawDevice`] instance.
> > > > > > +    pub fn from_dev(dev: &dyn RawDevice) -> Self {
> > > > >
> > > > > I am a rust newbie, but I don't understand this "RawDevice" here at all.
> > > >
> > > > Different buses will have their own Rust "Device" type, for example,
> > > > pci::Device, amba::Device, platform::Device that wrap their C
> > > > counterparts pci_dev, amba_device, platform_device.
> > > >
> > > > "RawDevice" is a trait for functionality that is common to all
> > > > devices. It exposes the "struct device" of each bus/subsystem so that
> > > > functions that work on any "struct device", for example, `clk_get`,
> > > > `pr_info`. will automatically work on all subsystems.
> > >
> > > Why is this being called "Raw" then?  Why not just "Device" to follow
> > > along with the naming scheme that the kernel already uses?
> >
> > Because it gives us access to underlying raw `struct device` pointer,
> > in Rust raw pointers are those unsafe `*mut T` or `*const T`. I'm not
> > married to the name though, we should probably look for a better one
> > if this one is confusing.
> >
> > Just "Device" is already taken. It's a ref-counted `struct device` (it
> > calls get_device/put_device in the right places automatically,
> > guarantees no dandling pointers); it is meant to be used by code that
> > needs to hold on to devices when they don't care about the bus. (It in
> > fact implements `RawDevice`.)
>
> I don't understand, why do you need both of these?  Why can't one just
> do?  Why would you need one without the other?  I would think that
> "Device" and "RawDevice" here would be the same thing, that is a way to
> refer to a "larger" underlying struct device memory chunk in a way that
> can be passed around without knowing, or caring, what the "real" device
> type is.

`Device` is a struct, it is the Rust abstraction for C's `struct device`.

Let's use the platform bus as our running example: we have
`platform::Device` as the Rust abstraction for C's `struct
platform_device`.

Let's use `clk_get`as our running example of a function that takes a
`struct device` as argument.

If we have a platform device, we can't just call `clk_get` because the
types don't match. In C, we access the `dev` field of `struct
platform_device` before we call `clk_get` (i.e., we call
clk_get(&pdev->dev, ...)), but in Rust we don't want to make the
fields of `platform::Device` public, especially because they're fields
of a C struct. So as part of `platform::Device` we'd have to implement
something like:

impl platform::Device {
    fn get_device(&self) -> &Device {
    ...
    }
}

Then calling `clk_get` would be something like:

pdev.get_device().clk_get(...)

The problem is that `clk_get` doesn't know that `platform::Device` is
a device, that's why we need this `get_device()` call on each bus
abstraction of a device, plus on each call to bus-agnostic device
functions.

Since we're implementing this "adapter" function anyway, we may as
well put in a _trait_ and improve how people use it. We say: every
struct that is a device should implement the `RawDevice` (or
`IsDevice`, or whatever we decide to call it) trait. Then
`platform::Device` would still have to implement something like:

impl RawDevice for platform::Device {
    fn get_device(&self) -> &Device {
    ...
    }
}

(Note that we went from `impl X` to `impl RawDevice for X`.)

With this trait, users can call `clk_get` on a platform device as follows:

pdev.clk_get(...)

So we improve the experience of driver developers by having bus
abstraction developers add "RawDevice for" to their impl block of the
`get_device` function for their device.

> >  How about `IsDevice`?
>
> That sounds like a question, and would return a boolean, not a structure :)

`RawDevice` is not a struct, it's a trait that is implemented by
bus-specific structs.

> > Then, for example, the platform bus would implement `IsDevice` for
> > `plaform::Device`.
>
> I don't really understand that, sorry.

I tried to explain it above. I think the source of the problem is the
distinction between `struct` and `trait`, the latter of course doesn't
exist in C.

Cheers,
-Wedson

Powered by blists - more mailing lists