LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
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
> 


  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 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 \
    --subject='Re: [greybus-dev] [PATCH v4] staging: greybus: Convert uart.c from IDR to XArray' \
    /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

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).