From: Alex Elder <elder@linaro.org>
To: Johan Hovold <johan@kernel.org>
Cc: Matthew Wilcox <willy@infradead.org>,
Alex Elder <elder@kernel.org>,
linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
greybus-dev@lists.linaro.org,
"Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Subject: Re: [greybus-dev] [PATCH v4] staging: greybus: Convert uart.c from IDR to XArray
Date: Tue, 31 Aug 2021 05:42:20 -0500 [thread overview]
Message-ID: <51c57831-71bf-92f8-2b20-3e542160a8bf@linaro.org> (raw)
In-Reply-To: <YS3jSsGSs0yAw/Ba@hovoldconsulting.com>
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
>
next prev parent reply other threads:[~2021-08-31 10:42 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-29 9:22 [PATCH v4] staging: greybus: Convert uart.c from IDR to XArray Fabio M. De Francesco
2021-08-30 9:12 ` Johan Hovold
2021-08-30 11:10 ` Fabio M. De Francesco
2021-08-30 11:52 ` Johan Hovold
2021-08-30 12:16 ` Matthew Wilcox
2021-08-30 12:33 ` Johan Hovold
2021-08-30 13:16 ` Fabio M. De Francesco
2021-08-30 13:20 ` [greybus-dev] " Alex Elder
2021-08-31 8:07 ` Johan Hovold
2021-08-31 10:42 ` Alex Elder [this message]
2021-08-31 11:51 ` Johan Hovold
2021-08-31 11:50 ` Fabio M. De Francesco
2021-08-31 12:18 ` Johan Hovold
2021-09-01 12:09 ` Alex Elder
2021-09-01 13:56 ` Fabio M. De Francesco
2021-09-01 14:29 ` Matthew Wilcox
2021-09-01 15:39 ` Fabio M. De Francesco
2021-08-30 13:31 ` Matthew Wilcox
2021-08-31 8:16 ` Johan Hovold
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=51c57831-71bf-92f8-2b20-3e542160a8bf@linaro.org \
--to=elder@linaro.org \
--cc=elder@kernel.org \
--cc=fmdefrancesco@gmail.com \
--cc=greybus-dev@lists.linaro.org \
--cc=johan@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=willy@infradead.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 NNTP newsgroup(s).