From: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
To: Danilo Krummrich <dakr@redhat.com>
Cc: Wedson Almeida Filho <wedsonaf@gmail.com>,
gregkh@linuxfoundation.org, rafael@kernel.org, ojeda@kernel.org,
alex.gaynor@gmail.com, boqun.feng@gmail.com, gary@garyguo.net,
bjorn3_gh@protonmail.com, benno.lossin@proton.me,
a.hindborg@samsung.com, aliceryhl@google.com,
tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
dave.hansen@linux.intel.com, rust-for-linux@vger.kernel.org,
x86@kernel.org, lyude@redhat.com, pstanner@redhat.com,
ajanulgu@redhat.com, airlied@redhat.com
Subject: Re: [PATCH v7 0/8] [RFC] Rust device / driver abstractions
Date: Wed, 27 Mar 2024 17:30:19 +0100 [thread overview]
Message-ID: <CANiq72kXDAiYdCARMbK5Z3QWQNm=MQa4Va7PTc5zJm=9+cH_5Q@mail.gmail.com> (raw)
In-Reply-To: <ZgQx_rjWrelMGm04@pollux>
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
prev parent reply other threads:[~2024-03-27 16:30 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-25 17:47 [PATCH v7 0/8] [RFC] Rust device / driver abstractions Danilo Krummrich
2024-03-25 17:47 ` [PATCH v7 1/8] arch: x86: tools: increase symbol name size Danilo Krummrich
2024-03-25 17:52 ` Miguel Ojeda
2024-03-25 17:52 ` [PATCH v7 0/8] [RFC] Rust device / driver abstractions Danilo Krummrich
2024-03-27 0:48 ` Wedson Almeida Filho
2024-03-27 11:25 ` Danilo Krummrich
2024-03-27 13:31 ` Miguel Ojeda
2024-03-27 14:49 ` Danilo Krummrich
2024-03-27 16:30 ` Miguel Ojeda [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CANiq72kXDAiYdCARMbK5Z3QWQNm=MQa4Va7PTc5zJm=9+cH_5Q@mail.gmail.com' \
--to=miguel.ojeda.sandonis@gmail.com \
--cc=a.hindborg@samsung.com \
--cc=airlied@redhat.com \
--cc=ajanulgu@redhat.com \
--cc=alex.gaynor@gmail.com \
--cc=aliceryhl@google.com \
--cc=benno.lossin@proton.me \
--cc=bjorn3_gh@protonmail.com \
--cc=boqun.feng@gmail.com \
--cc=bp@alien8.de \
--cc=dakr@redhat.com \
--cc=dave.hansen@linux.intel.com \
--cc=gary@garyguo.net \
--cc=gregkh@linuxfoundation.org \
--cc=lyude@redhat.com \
--cc=mingo@redhat.com \
--cc=ojeda@kernel.org \
--cc=pstanner@redhat.com \
--cc=rafael@kernel.org \
--cc=rust-for-linux@vger.kernel.org \
--cc=tglx@linutronix.de \
--cc=wedsonaf@gmail.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).