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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YS4eFP3xXdAugyYL@hovoldconsulting.com>
Date:   Tue, 31 Aug 2021 14:18:28 +0200
From:   Johan Hovold <johan@...nel.org>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     Alex Elder <elder@...aro.org>,
        Matthew Wilcox <willy@...radead.org>,
        Alex Elder <elder@...nel.org>, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org, greybus-dev@...ts.linaro.org
Subject: Re: [greybus-dev] [PATCH v4] staging: greybus: Convert uart.c from
 IDR to XArray

On Tue, Aug 31, 2021 at 01:50:05PM +0200, Fabio M. De Francesco wrote:
> On Tuesday, August 31, 2021 10:07:38 AM CEST Johan Hovold wrote:

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

> I cannot test my changes to Greybus, but I think that trivial changes are 
> just required to build. To stay on the safe side I had left those 
> mutex_lock() around xa_load(). I wasn't sure about it, but since the original 
> code had the Mutexes I thought it wouldn't hurt to leave them there.

But if you weren't sure that your patch was correct, you can't also
claim that it was trivial. Patches dealing with locking and concurrency
usually are not.

I too had go look up the XArray interface and review the Greybus uart
code (which is broken and that doesn't make it easier for any of us).

So no, this wasn't trivial, it carries a cost and should therefore in
the end also have some gain. And what that was wasn't clear from the
commit message (since any small efficiency gain is irrelevant in this
case).

Sorry you got stuck in the middle. Perhaps you can see it as a
first-hand experience of some of the trade offs involved when doing
kernel development.

And remember that a good commit message describing the motivation for
the change (avoiding boiler-plate marketing terms) is a good start if
you want to get your patches accepted.

Johan