Message-Id: <D9760KMM0PSB.HONQ7MUG8OTN@buenzli.dev>
Date: Tue, 15 Apr 2025 13:11:07 +0200
From: "Remo Senekowitsch" <remo@...nzli.dev>
To: "Danilo Krummrich" <dakr@...nel.org>
Cc: "Rob Herring" <robh@...nel.org>, "Saravana Kannan"
 <saravanak@...gle.com>, "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>, "Benno Lossin" <benno.lossin@...ton.me>,
 "Andreas Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>, "Greg
 Kroah-Hartman" <gregkh@...uxfoundation.org>, "Rafael J. Wysocki"
 <rafael@...nel.org>, <linux-kernel@...r.kernel.org>,
 <devicetree@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v2 2/5] rust: Add bindings for reading device properties

On Tue Apr 15, 2025 at 11:48 AM CEST, Danilo Krummrich wrote:
> On Tue, Apr 15, 2025 at 01:55:42AM +0200, Remo Senekowitsch wrote:
>> On Mon Apr 14, 2025 at 7:44 PM CEST, Danilo Krummrich wrote:
>> > On Mon, Apr 14, 2025 at 05:26:27PM +0200, Remo Senekowitsch wrote:
>> >> The device property API is a firmware agnostic API for reading
>> >> properties from firmware (DT/ACPI) devices nodes and swnodes.
>> >> 
>> >> While the C API takes a pointer to a caller allocated variable/buffer,
>> >> the rust API is designed to return a value and can be used in struct
>> >> initialization. Rust generics are also utilized to support different
>> >> types of properties where appropriate.
>> >> 
>> >> The PropertyGuard is a way to force users to specify whether a property
>> >> is supposed to be required or not. This allows us to move error
>> >> logging of missing required properties into core, preventing a lot of
>> >> boilerplate in drivers.
>> >
>> > The patch adds a lot of thing, i.e.
>> >   * implement PropertyInt
>> >   * implement PropertyGuard
>> >   * extend FwNode by a lot of functions
>> >   * extend Device by some property functions
>> >
>> > I see that from v1 a lot of things have been squashed, likely because there are
>> > a few circular dependencies. Is there really no reasonable way to break this
>> > down a bit?
>> 
>> I was explicitly asked to do this in the previous thread[1].
>
> I'm well aware that you were asked to do so and that one reason was that
> subsequent patches started deleting code that was added in previous ones
> (hence my suspicion of circular dependencies and that splitting up things might
> not be super trivial).
>
>> I'm happy
>> to invest time into organizing files and commits exactly the way people
>> want, but squashing and splitting the same commits back and forth
>> between subsequent patch series is a waste of my time.
>
> I don't think you were asked to go back and forth, but whether you see a
> reasonable way to break things down a bit, where "reasonable" means without
> deleting code that was just added.

I was asked to squash two specific commits. The first was making the
read method generic. That was the one that deleted much code. Totally
reasonable, and the generic stuff might be discarded anyway, so I won't
be moving stuff back and forth.

However, the second commit was the one introducing PropertyGuard. That
was a beautifully atomic commit, no circular dependencies in sight. If
this commit is to be split up, one of the smaller ones would without
doubt look exactly the same as the one before. I provided a link[1]
to the exact email telling me to squash that exact patch to avoid any
confusion.

>> Do reviewers not typically read the review comments of others as well?
>
> I think mostly they do, but maintainers and reviewers are rather busy people.
> So, I don't think you can expect everyone to follow every thread, especially
> when they get lengthy.
>
>> What can I do to avoid this situation and make progress instead of
>> running in circles?
>
> I suggest to investigate whether it can be split it up in a reasonable way and
> subsequently answer the question.

The point is that I agree with you that the PropertyGuard patch can be
split out. And possibly more: I think a reasonable person could make a
separate commit for every `property_read_<type>` method. And I'm happy
to do that if it's the way to go.  But if I do that, send a v3 and then
someone else asks me to squash it again (because they didn't read this
exchange between you and me...) then I would've wasted my time.

> With your contribution you attempt to add a rather large portion of pretty core
> code. This isn't an easy task and quite some discussion is totally expected;
> please don't get frustrated, the series goes pretty well. :)

I'm trying, but I can't say I have a good first impression of doing
code review over a mailing list. Doing open-heart surgery with a
pickfork and writing documents in Microsoft Word both seem like more
productive and enjoyable activities to me.
 
Link: https://lore.kernel.org/rust-for-linux/20250326171411.590681-1-remo@buenzli.dev/T/#m68b99b283a2e62726ee039bb2394d0741b31e330 [1]

Powered by blists - more mailing lists