Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [greybus-dev] [PATCH v4] staging: greybus: Convert uart.c from
 IDR to XArray
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>
References: <20210829092250.25379-1-fmdefrancesco@gmail.com>
 <YSyg/Db1So0LDGR+@hovoldconsulting.com>
 <2879439.WEJLM9RBEh@localhost.localdomain>
 <YSzGkNfG6HOayiXi@hovoldconsulting.com>
 <YSzMB80NOkNvdjy1@casper.infradead.org>
 <YSzQAd0Rg5CF/eLe@hovoldconsulting.com>
 <f7a25eb1-20f4-5031-a156-9e5dc019ad28@linaro.org>
 <YS3jSsGSs0yAw/Ba@hovoldconsulting.com>
From:   Alex Elder <elder@linaro.org>
Message-ID: <51c57831-71bf-92f8-2b20-3e542160a8bf@linaro.org>
Date:   Tue, 31 Aug 2021 05:42:20 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <YS3jSsGSs0yAw/Ba@hovoldconsulting.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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
>