Nouveau - Jul 2025 - [PATCH v13 2/5] rust: support formatting of foreign types

On Thu, Jul 3, 2025 at 5:38?PM Miguel Ojeda
<miguel.ojeda.sandonis at gmail.com> wrote:
>
> On Thu, Jul 3, 2025 at 11:28?PM Andrew Lunn <andrew at lunn.ch>
wrote:
> >
> > A small patch tends to be more obviously correct than a big patch. The
> > commit message is more focused and helpful because it refers to a
> > small chunk of code. Because the commit message is more focused, it
> > can answer questions reviewers might ask, before they ask them. If i
>
> Yeah, also better for smaller reverts, as well as typically easier to
> backport if needed, etc.
I appreciate that this advice is well-intentioned, thank you. I agree
that all things being equal, small changes are better. In this
particular case, there are specific downsides to splitting for its own
sake which I tried to explain in previous replies: splitting the proc
macro from the rest of the machinery risks forcing a reviewer to
assess a chunk of code without seeing how it is used; in my experience
this limits the scope of the review. Splitting by subsystem has other
downsides, which I attempted to enumerate in my reply to Benno in the
other fork of this discussion (let's discuss those there, please).

There's also a tactical question about splitting by subsystem: are
there any tools that would assist in doing this, or is it a matter of
manually consulting MAINTAINERS to figure out file groupings?

> There's also a tactical question about splitting by subsystem: are
> there any tools that would assist in doing this, or is it a matter of
> manually consulting MAINTAINERS to figure out file groupings?
You can run ./scripts/get_maintainer -f path/to/file.c

and it will give you the Maintainers for that file. From that you can
imply the subsystem.

It might be possible to do one tree wide patchset, since Rust is still
small at the moment. But you will need to get Reviewed-by: or
Acked-by: from each subsystem Maintainer for the patches. That is not
always easy, since some subsystems have CI systems, and will want the
patch to pass their CI tests before giving an Reviewed-by.

	Andrew

On Fri, Jul 4, 2025 at 12:46?AM Tamir Duberstein <tamird at gmail.com>
wrote:
>
> There's also a tactical question about splitting by subsystem: are
> there any tools that would assist in doing this, or is it a matter of
> manually consulting MAINTAINERS to figure out file groupings?
As Andrew mentioned, you can use that script, though I recommend not
fully/blindly trusting it.

Sometimes you will want to adjust things, e.g. sometimes things may be
related even if in a couple different `MAINTAINERS` entries, or you
may want to adjust the flags the script provides to filter, or you may
want to check `git log --no-merges` to see who is recently applying
patches related to that area, etc.

It is essentially the same process as when you send patches.

For instance, taking the diffstat above, and ignoring contents (i.e.
assuming all lines could just be freely split and without considering
other splits discussed to make the patches smaller first and reducing
the flag day changes), I could have done something like this:

    drivers/block/rnull.rs       |  2 +-
    rust/kernel/block/mq.rs      |  2 +-

    drivers/gpu/nova-core/gpu.rs |  4 +-

    rust/kernel/device.rs        |  2 +-

    rust/kernel/kunit.rs         |  6 +--

    rust/kernel/seq_file.rs      |  2 +-

    rust/kernel/fmt.rs           | 89 +++++++++++++++++++++++++++++++++++++++
    rust/kernel/lib.rs           |  1 +
    rust/kernel/prelude.rs       |  3 +-
    rust/kernel/print.rs         |  4 +-
    rust/kernel/str.rs           | 22 ++++------
    rust/macros/fmt.rs           | 99
++++++++++++++++++++++++++++++++++++++++++++
    rust/macros/lib.rs           | 19 +++++++++
    rust/macros/quote.rs         |  7 ++++
    scripts/rustdoc_test_gen.rs  |  2 +-

And then those long lines may hint that it may make sense to split the
smaller tweaks in the last group into their own patch, so that it
mirrors what is done for the other smaller groups. Thus possibly
leaving the feature being added into its own patch, which would be the
biggest and the one that would take some discussion. And the others
would be the small ones that are easy to Acked-by or Reviewed-by or
simply take (if independently possible) by other maintainers.

And so on -- again, this is speaking generally, and it is just a dummy
example, not intended to say anything about the current patch. And
sometimes things may not make sense to split too far, and it can be
more annoying than it is worth for everyone involved, e.g. when we are
talking about trivial conversions that could take 50+ patches that
could be automated instead and then applied by a single maintainer.

So it is a balance.

Cheers,
Miguel