[PATCH v7 0/8] [RFC] Rust device / driver abstractions

[PATCH v7 0/8] [RFC] Rust device / driver abstractions

[Danilo Krummrich]

Hi,

This patch series provides some initial Rust abstractions around the device /
driver model, including an abstraction for device private data.

This patch series is sent in the context of [1] and is also available at [2].

- Danilo

[1] https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u
[2] https://github.com/Rust-for-Linux/linux/tree/staging/rust-device

Danilo Krummrich (1):
  arch: x86: tools: increase symbol name size

Wedson Almeida Filho (7):
  rust: device: Add a minimal RawDevice trait
  rust: device: Add a stub abstraction for devices
  rust: add driver abstraction
  rust: add rcu abstraction
  rust: add revocable mutex
  rust: add revocable objects
  rust: add device::Data

 arch/x86/tools/insn_decoder_test.c |   2 +-
 rust/bindings/bindings_helper.h    |   1 +
 rust/helpers.c                     |  28 ++
 rust/kernel/device.rs              | 215 +++++++++++++
 rust/kernel/driver.rs              | 493 +++++++++++++++++++++++++++++
 rust/kernel/lib.rs                 |   6 +-
 rust/kernel/revocable.rs           | 438 +++++++++++++++++++++++++
 rust/kernel/sync.rs                |   3 +
 rust/kernel/sync/rcu.rs            |  52 +++
 rust/kernel/sync/revocable.rs      |  98 ++++++
 rust/macros/module.rs              |   2 +-
 samples/rust/rust_minimal.rs       |   2 +-
 samples/rust/rust_print.rs         |   2 +-
 13 files changed, 1337 insertions(+), 5 deletions(-)
 create mode 100644 rust/kernel/device.rs
 create mode 100644 rust/kernel/driver.rs
 create mode 100644 rust/kernel/revocable.rs
 create mode 100644 rust/kernel/sync/rcu.rs
 create mode 100644 rust/kernel/sync/revocable.rs


base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
-- 
2.44.0

[PATCH v7 1/8] arch: x86: tools: increase symbol name size

[Danilo Krummrich]

Increase the symbol name size as the Rust compiler seems to generate
symbol names exceeding the previous buffer size of 256.

arch/x86/tools/insn_decoder_test: error: malformed line 2486512:
77620
make[2]: *** [arch/x86/tools/Makefile:26: posttest] Error 3

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 arch/x86/tools/insn_decoder_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/tools/insn_decoder_test.c b/arch/x86/tools/insn_decoder_test.c
index 472540aeabc2..18601b3c5037 100644
--- a/arch/x86/tools/insn_decoder_test.c
+++ b/arch/x86/tools/insn_decoder_test.c
@@ -106,7 +106,7 @@ static void parse_args(int argc, char **argv)
 	}
 }
 
-#define BUFSIZE 256
+#define BUFSIZE 1024
 
 int main(int argc, char **argv)
 {
-- 
2.44.0

Re: [PATCH v7 1/8] arch: x86: tools: increase symbol name size

[Miguel Ojeda]

On Mon, Mar 25, 2024 at 6:47 PM Danilo Krummrich <dakr@redhat.com> wrote:
>
> Increase the symbol name size as the Rust compiler seems to generate
> symbol names exceeding the previous buffer size of 256.
>
> arch/x86/tools/insn_decoder_test: error: malformed line 2486512:
> 77620
> make[2]: *** [arch/x86/tools/Makefile:26: posttest] Error 3
>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>

For reference, this is
https://lore.kernel.org/rust-for-linux/320c4dba-9919-404b-8a26-a8af16be1845@app.fastmail.com/

Cheers,
Miguel

Re: [PATCH v7 0/8] [RFC] Rust device / driver abstractions

[Danilo Krummrich]

Please ignore this one, I fat-fingered the version of the series.

- Danilo

On 3/25/24 18:47, Danilo Krummrich wrote:
> Hi,
> 
> This patch series provides some initial Rust abstractions around the device /
> driver model, including an abstraction for device private data.
> 
> This patch series is sent in the context of [1] and is also available at [2].
> 
> - Danilo
> 
> [1] https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u
> [2] https://github.com/Rust-for-Linux/linux/tree/staging/rust-device
> 
> Danilo Krummrich (1):
>    arch: x86: tools: increase symbol name size
> 
> Wedson Almeida Filho (7):
>    rust: device: Add a minimal RawDevice trait
>    rust: device: Add a stub abstraction for devices
>    rust: add driver abstraction
>    rust: add rcu abstraction
>    rust: add revocable mutex
>    rust: add revocable objects
>    rust: add device::Data
> 
>   arch/x86/tools/insn_decoder_test.c |   2 +-
>   rust/bindings/bindings_helper.h    |   1 +
>   rust/helpers.c                     |  28 ++
>   rust/kernel/device.rs              | 215 +++++++++++++
>   rust/kernel/driver.rs              | 493 +++++++++++++++++++++++++++++
>   rust/kernel/lib.rs                 |   6 +-
>   rust/kernel/revocable.rs           | 438 +++++++++++++++++++++++++
>   rust/kernel/sync.rs                |   3 +
>   rust/kernel/sync/rcu.rs            |  52 +++
>   rust/kernel/sync/revocable.rs      |  98 ++++++
>   rust/macros/module.rs              |   2 +-
>   samples/rust/rust_minimal.rs       |   2 +-
>   samples/rust/rust_print.rs         |   2 +-
>   13 files changed, 1337 insertions(+), 5 deletions(-)
>   create mode 100644 rust/kernel/device.rs
>   create mode 100644 rust/kernel/driver.rs
>   create mode 100644 rust/kernel/revocable.rs
>   create mode 100644 rust/kernel/sync/rcu.rs
>   create mode 100644 rust/kernel/sync/revocable.rs
> 
> 
> base-commit: e8f897f4afef0031fe618a8e94127a0934896aba

Re: [PATCH v7 0/8] [RFC] Rust device / driver abstractions

[Wedson Almeida Filho]

On Mon, 25 Mar 2024 at 14:47, Danilo Krummrich <dakr@redhat.com> wrote:
>
> Hi,
>
> This patch series provides some initial Rust abstractions around the device /
> driver model, including an abstraction for device private data.
>
> This patch series is sent in the context of [1] and is also available at [2].
>
> - Danilo
>
> [1] https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u
> [2] https://github.com/Rust-for-Linux/linux/tree/staging/rust-device
>
> Danilo Krummrich (1):
>   arch: x86: tools: increase symbol name size
>
> Wedson Almeida Filho (7):
>   rust: device: Add a minimal RawDevice trait
>   rust: device: Add a stub abstraction for devices
>   rust: add driver abstraction
>   rust: add rcu abstraction
>   rust: add revocable mutex
>   rust: add revocable objects
>   rust: add device::Data

Danilo,

It seems like I'm the original author of the vast majority of the code
in this RFC series, yet I wasn't contacted by you for coordination
before you sent this.

A bunch (all?) of these patches were already submitted upstream with a
lot of discussion and decisions made to modify things. Why are you
resubmitting them basically ignoring all previous discussions? Take
patches 2 & 3 as examples (I don't bother to look for others now):

https://lore.kernel.org/lkml/20230224-rust-iopt-rtkit-v1-2-49ced3391295@asahilina.net/
https://lore.kernel.org/lkml/20230224-rust-iopt-rtkit-v1-5-49ced3391295@asahilina.net/

Also, these patches were written in the rust branch. Before we
upstream them, we have to revisit them to check if changes are needed
given the changes/improvements we have made; for example, pin init now
allows us to initialise pinned objects safely -- we need to follow the
new way now and I see that you don't in `RevocableMutex`. PinInit also
 enables us to have pinned modules, which simplifies how we do
registrations (so they also need to be updated), locks have been
redone with a common `Lock` type, etc.

In summary, we can't just copy code, we need to revisit some of it and
at least check suitability before submitting them.

>  arch/x86/tools/insn_decoder_test.c |   2 +-
>  rust/bindings/bindings_helper.h    |   1 +
>  rust/helpers.c                     |  28 ++
>  rust/kernel/device.rs              | 215 +++++++++++++
>  rust/kernel/driver.rs              | 493 +++++++++++++++++++++++++++++
>  rust/kernel/lib.rs                 |   6 +-
>  rust/kernel/revocable.rs           | 438 +++++++++++++++++++++++++
>  rust/kernel/sync.rs                |   3 +
>  rust/kernel/sync/rcu.rs            |  52 +++
>  rust/kernel/sync/revocable.rs      |  98 ++++++
>  rust/macros/module.rs              |   2 +-
>  samples/rust/rust_minimal.rs       |   2 +-
>  samples/rust/rust_print.rs         |   2 +-
>  13 files changed, 1337 insertions(+), 5 deletions(-)
>  create mode 100644 rust/kernel/device.rs
>  create mode 100644 rust/kernel/driver.rs
>  create mode 100644 rust/kernel/revocable.rs
>  create mode 100644 rust/kernel/sync/rcu.rs
>  create mode 100644 rust/kernel/sync/revocable.rs
>
>
> base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
> --
> 2.44.0
>

Re: [PATCH v7 0/8] [RFC] Rust device / driver abstractions

[Danilo Krummrich]

On Tue, Mar 26, 2024 at 09:48:07PM -0300, Wedson Almeida Filho wrote:
> On Mon, 25 Mar 2024 at 14:47, Danilo Krummrich <dakr@redhat.com> wrote:
> >
> > Hi,
> >
> > This patch series provides some initial Rust abstractions around the device /
> > driver model, including an abstraction for device private data.
> >
> > This patch series is sent in the context of [1] and is also available at [2].
> >
> > - Danilo
> >
> > [1] https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u
> > [2] https://github.com/Rust-for-Linux/linux/tree/staging/rust-device
> >
> > Danilo Krummrich (1):
> >   arch: x86: tools: increase symbol name size
> >
> > Wedson Almeida Filho (7):
> >   rust: device: Add a minimal RawDevice trait
> >   rust: device: Add a stub abstraction for devices
> >   rust: add driver abstraction
> >   rust: add rcu abstraction
> >   rust: add revocable mutex
> >   rust: add revocable objects
> >   rust: add device::Data
> 
> Danilo,
> 
> It seems like I'm the original author of the vast majority of the code
> in this RFC series, yet I wasn't contacted by you for coordination
> before you sent this.

Agree, it would have been better to directly contact you again.

Except for patches 2 & 3 the other ones are created from patches that
"extracted" code from the HEAD of R4L/rust. I just fixed authorship before
sending them, hence I think I didn't really notice that you are the original
author of all device / driver abstractions.

Please also note that I announced these efforts a couple of times, e.g. in [1]
and [2].

> 
> A bunch (all?) of these patches were already submitted upstream with a
> lot of discussion and decisions made to modify things. Why are you
> resubmitting them basically ignoring all previous discussions? Take
> patches 2 & 3 as examples (I don't bother to look for others now):
> 
> https://lore.kernel.org/lkml/20230224-rust-iopt-rtkit-v1-2-49ced3391295@asahilina.net/
> https://lore.kernel.org/lkml/20230224-rust-iopt-rtkit-v1-5-49ced3391295@asahilina.net/o

For the patches 2 & 3 I'm aware of the previous discussions. However, I have to
admit it's been a while since I read them and hence I forgot to mention that
this series is, besides others, also a follow up of that one.

For everything else I'm not aware of previous discussions, if there are any,
I'm sorry I missed them. Also please let me know if there are any, such that I
can follow up.

> 
> Also, these patches were written in the rust branch. Before we
> upstream them, we have to revisit them to check if changes are needed
> given the changes/improvements we have made; for example, pin init now
> allows us to initialise pinned objects safely -- we need to follow the
> new way now and I see that you don't in `RevocableMutex`. PinInit also
>  enables us to have pinned modules, which simplifies how we do
> registrations (so they also need to be updated), locks have been
> redone with a common `Lock` type, etc.

In [2] I was asking about the preferred way to get some immediate discussions
going. When I proposed to send links to the corresponding branches to the
mailing list or send a patch series (for rust-device I ended up doing both) I
did not hear any contradiction.

To me the mailing list seems to be a good place to revisit, review and improve
these patches.

I'd propose to just continue with this series and collaborate on improving it.
Are you fine with that?

- Danilo

[1] https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u
[2] https://rust-for-linux.zulipchat.com/#narrow/stream/415254-DRM/topic/Topic.20branches.20for.20staging.20Rust.20abstractions/near/426580539

> 
> In summary, we can't just copy code, we need to revisit some of it and
> at least check suitability before submitting them.
> 
> >  arch/x86/tools/insn_decoder_test.c |   2 +-
> >  rust/bindings/bindings_helper.h    |   1 +
> >  rust/helpers.c                     |  28 ++
> >  rust/kernel/device.rs              | 215 +++++++++++++
> >  rust/kernel/driver.rs              | 493 +++++++++++++++++++++++++++++
> >  rust/kernel/lib.rs                 |   6 +-
> >  rust/kernel/revocable.rs           | 438 +++++++++++++++++++++++++
> >  rust/kernel/sync.rs                |   3 +
> >  rust/kernel/sync/rcu.rs            |  52 +++
> >  rust/kernel/sync/revocable.rs      |  98 ++++++
> >  rust/macros/module.rs              |   2 +-
> >  samples/rust/rust_minimal.rs       |   2 +-
> >  samples/rust/rust_print.rs         |   2 +-
> >  13 files changed, 1337 insertions(+), 5 deletions(-)
> >  create mode 100644 rust/kernel/device.rs
> >  create mode 100644 rust/kernel/driver.rs
> >  create mode 100644 rust/kernel/revocable.rs
> >  create mode 100644 rust/kernel/sync/rcu.rs
> >  create mode 100644 rust/kernel/sync/revocable.rs
> >
> >
> > base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
> > --
> > 2.44.0
> >
> 

Re: [PATCH v7 0/8] [RFC] Rust device / driver abstractions

[Miguel Ojeda]

On Wed, Mar 27, 2024 at 12:25 PM Danilo Krummrich <dakr@redhat.com> wrote:
>
> In [2] I was asking about the preferred way to get some immediate discussions
> going. When I proposed to send links to the corresponding branches to the
> mailing list or send a patch series (for rust-device I ended up doing both) I
> did not hear any contradiction.

Sending patch series directly is something you/Philipp proposed. And
that is fine -- nobody can tell you to not send patches. But, from
experience, we know sometimes it is best to get in the same page
and/or room.

I specifically suggested reporting your progress in Zulip and/or the
mailing lists for that reason, and because lifting code from other
places like `rust` generally requires revisiting it first like Wedson
mentioned.

In addition, we always suggest pinging directly the original authors
too before submitting patches from them, to avoid this kind of thing.

Moreover, I would recommend tagging more prominently the patches as
staging/RFC/... (i.e. each of them). I know you have "RFC" in the
cover letter one, but I am aware of at least one person that did not
realize initially the patches were actually RFC. In addition, I would
suggest that the cover letter explains where and how the patches where
lifted from, so that people are aware of their state etc. I would also
recommend, for clarity and even if out of deference only, to mention
whether the authors of the patches you are carrying were aware of this
submission (i.e. if you explicitly heard from them).

Finally, it is not always possible "to get some immediate discussions
going" (i.e. emphasis on "immediate"). It all depends on who / which
subsystem / etc. you are dealing with.

Cheers,
Miguel

Re: [PATCH v7 0/8] [RFC] Rust device / driver abstractions

[Danilo Krummrich]

On Wed, Mar 27, 2024 at 02:31:16PM +0100, Miguel Ojeda wrote:
> On Wed, Mar 27, 2024 at 12:25 PM Danilo Krummrich <dakr@redhat.com> wrote:
> >
> > In [2] I was asking about the preferred way to get some immediate discussions
> > going. When I proposed to send links to the corresponding branches to the
> > mailing list or send a patch series (for rust-device I ended up doing both) I
> > did not hear any contradiction.
> 
> Sending patch series directly is something you/Philipp proposed. And
> that is fine -- nobody can tell you to not send patches. But, from
> experience, we know sometimes it is best to get in the same page
> and/or room.
> 
> I specifically suggested reporting your progress in Zulip and/or the
> mailing lists for that reason, and because lifting code from other
> places like `rust` generally requires revisiting it first like Wedson
> mentioned.

Sorry, I wasn't aware that you prefer to have some extra process / stage of
revisiting / reviewing of existing patches / code that is picked up from
R4L/rust.

Maybe it would have been good to give me this pointer when I asked: "how (and
where) to get specific code discussed." [1]

Maybe I'm also misunderstanding what you mean by "revisiting".

Anyway, how can we proceed? Can we just continue with this series and improve
things by further review? Do you prefer to approach it differently?

> 
> In addition, we always suggest pinging directly the original authors
> too before submitting patches from them, to avoid this kind of thing.

As already mentioned, fully agree.

> 
> Moreover, I would recommend tagging more prominently the patches as
> staging/RFC/... (i.e. each of them). I know you have "RFC" in the
> cover letter one, but I am aware of at least one person that did not
> realize initially the patches were actually RFC. In addition, I would

Sure, I can try to make it more obvious by adding --subject-prefix="RFC: PATCH"
to git-format-patch.

> suggest that the cover letter explains where and how the patches where
> lifted from, so that people are aware of their state etc. I would also

I think that's actually explained in [2], which I referenced in the cover
letter. If you think there should be additional information, please let me know,
I'm happy to add it.

> recommend, for clarity and even if out of deference only, to mention
> whether the authors of the patches you are carrying were aware of this
> submission (i.e. if you explicitly heard from them).

Noted. But just to clarify (and I'm clearly not saying you implied something
else), not doing so is not meant to be understood as doing it without proper
deference. I was very careful in setting up proper authorship and co-authorship
(e.g. for fixes that I squashed into some commits).

> 
> Finally, it is not always possible "to get some immediate discussions
> going" (i.e. emphasis on "immediate"). It all depends on who / which
> subsystem / etc. you are dealing with.
> 
> Cheers,
> Miguel
> 

[1] https://rust-for-linux.zulipchat.com/#narrow/stream/415254-DRM/topic/Topic.20branches.20for.20staging.20Rust.20abstractions/near/426750399
[2] https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u

Re: [PATCH v7 0/8] [RFC] Rust device / driver abstractions

[Miguel Ojeda]

On Wed, Mar 27, 2024 at 3:49 PM Danilo Krummrich <dakr@redhat.com> wrote:
>
> Sorry, I wasn't aware that you prefer to have some extra process / stage of
> revisiting / reviewing of existing patches / code that is picked up from
> R4L/rust.

No worries. It is up to the submitters in the end (i.e. you in this
case) -- we just try to help by suggesting what we think may have the
best chances of succeeding.

In other words, it is not that we want to have "extra process". In
fact, we are not the ones that should/will decide when to merge code
in many/most cases (it is the relevant subsystems where applicable).
But we are trying to help you get there.

In particular, the old `rust` branch contained a lot of work from
different people, in different stages. Some was mainlined. Some was
marked as unfinished. Some is now old enough that requires adjustments
to new stuff like pinned-init, or maybe the C side has changed since
then, etc. even if it was upstreamable. Some was definitely not
intended/ready for upstreaming.

> Maybe it would have been good to give me this pointer when I asked: "how (and
> where) to get specific code discussed." [1]

I think we talked about the usual process we have been
following/suggesting in our virtual call. I don't discount having been
unclear or not mentioning everything to you, but it was a fairly long
call (~3 hours I think! :)

We also have some of our recommendations at
https://rust-for-linux.com/contributing#submitting-new-abstractions-and-modules

i.e. the main point for work like this is to get people involved.
Sending the patches like you did is one way of doing it, but sometimes
it is best to approach it a bit differently, and thus our suggestions.

> Maybe I'm also misunderstanding what you mean by "revisiting".
>
> Anyway, how can we proceed? Can we just continue with this series and improve
> things by further review? Do you prefer to approach it differently?

Up to you, but I would suggest taking a look at what Greg & Wedson
have mentioned so far (last time this was submitted and now), fixing
whatever is needed there (like the pinned-init stuff) and improving
the documentation (API documentation, code comments, even `Doc/` if
needed) as much as possible where needed (so that maintainers can very
clearly see what you are doing, and it will also help other people
later on too), perhaps replying to all the previous feedback in a
summary in the cover letter...

Then I would double-check afterwards with Wedson and others if it
looks better, and then resubmitting to the list.

Or maybe Wedson wants to revisit and submit these himself -- I would
check with him, perhaps you can save yourself some work :)

> I think that's actually explained in [2], which I referenced in the cover
> letter. If you think there should be additional information, please let me know,
> I'm happy to add it.

I did read your Nova message before my other reply, and I pointed it
out because it is not (even assuming your readers do read that other
message).

That message just says "something" was taking from "somewhere",
essentially. You want to be way more concrete. For instance:

  - From where/when it was taken: i.e. it could have been the `rust`
branch, or another tree from one of the main users, or a personal tree
(e.g. Wedson's in this case), or even a random one somewhere else (and
somebody there could have had the code modified without telling
anybody anything nor adding a SoB etc.).

    It could also have been not the latest version from one of those
trees, either.

    You could provide the URL/hash if you think it is useful.

  - Why it was taken: i.e. what will require this code? Yes, "Nova",
but ideally you want to be more concrete. You can even get to the
point where, if you are e.g. adding a function, and you already have
the code somewhere that calls the function, then if you can include a
link (or even an example of the caller inline if that makes sense,
assuming there is not an example in the API documentation itself), it
will help a lot readers.

    After all, the main blocker for some of these things is the
"chicken-and-egg" issue, thus clarifying helps.

    For instance, take a look at e.g. commit e7b9b1ff1d49 ("rust:
sync: add `CondVar::wait_timeout`"), where Alice most recently wrote:

        This is used by Rust Binder for process freezing. There, we want to
        sleep until the freeze operation completes, but we want to be able to
        abort the process freezing if it doesn't complete within some timeout.

And this typically you should do per series at least, or in some cases
per patch may be best.

> Noted. But just to clarify (and I'm clearly not saying you implied something
> else), not doing so is not meant to be understood as doing it without proper
> deference. I was very careful in setting up proper authorship and co-authorship
> (e.g. for fixes that I squashed into some commits).

Yeah, the authorship looks fine -- I meant to be clear when you didn't
hear from the authors.

i.e. I would recommend pinging, waiting a fair amount of time, and if
you don't hear from them, saying so in the cover letter (so that
everybody understands that they may not have realized this was sent,
or they may not have had the chance to comment, or they may not be
ready to reply in the mailing list, etc.).

I hope that helps, and thanks for working on this!

Cheers,
Miguel