Re: [greybus-dev] [PATCH v4] staging: greybus: Convert uart.c from IDR to XArray

From: Alex Elder
Date: Tue Aug 31 2021 - 06:42:27 EST


On 8/31/21 3:07 AM, Johan Hovold wrote:
On Mon, Aug 30, 2021 at 08:20:25AM -0500, Alex Elder wrote:

I have been offering review feedback on this patch for three reasons:

- First, because I think the intended change does no real harm to the
Greybus code, and in a small way actually simplifies it.

You leave out that we've already seen three versions of the patch that
broke things in various ways and that there was still work to be done
with respect to the commit message and verifying the locking. That's all
real costs that someone needs to bear.

This is true. But it's separate from my reason for doing it,
and unrelated to the suggested change.

- Because I wanted to encourage Fabio's efforts to be part of the
Linux contributor community.

Helping new contributers that for example have run into a bug or need
some assistance adding a new feature that they themselves have use for
is one thing.

I'm not so sure we're helping either newcomers or Linux long term by
inventing work for an already strained community however.

[ This is more of a general comment and of course in no way a critique
against Fabio or a claim that the XArray conversions are pointless. ]

Yes, yours is a general comment. But I would characterize
this as Fabio "scratching an itch" rather than "inventing
new work." The strained community needs more helpers, and
they don't suddenly appear fully-formed; they need to be
cultivated. There's a balance to strike between "I see
you need a little guidance here" and "go away and come
back when you know how to do it right."

In any case, I don't disagree with your general point, but
we seem to view this particular instance differently.

- Because I suspected that Matthew's long-term intention was to
replace IDR/IDA use with XArray, so this would represent an early
conversion.

That could be a valid motivation for the change indeed (since the
efficiency arguments are irrelevant in this case), but I could not find
any indications that there has been an agreement to deprecate the
current interfaces.

Last time around I think there was even push-back to convert IDR/IDA to
use XArray internally instead (but I may misremember).

The Greybus code is generally good, but that doesn't mean it can't
evolve. It gets very little patch traffic, so I don't consider small
changes like this "useless churn." The Greybus code is held up as an
example of Linux kernel code that can be safely modified, and I think
it's actively promoted as a possible source of new developer tasks
(e.g. for Outreachy).

Since most people can't really test their changes to Greybus perhaps it
isn't the best example of code that can be safely modified. But yeah,
parts of it are still in staging and, yes, staging has been promoted as
place were some churn is accepted.

Testing Greybus code is a problem. *That* would be something useful
for people to help fix.

-Alex

Johan