LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Johan Hovold <johan@kernel.org>
To: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Cc: Alex Elder <elder@linaro.org>,
	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
Subject: Re: [greybus-dev] [PATCH v4] staging: greybus: Convert uart.c from IDR to XArray
Date: Tue, 31 Aug 2021 14:18:28 +0200	[thread overview]
Message-ID: <YS4eFP3xXdAugyYL@hovoldconsulting.com> (raw)
In-Reply-To: <6155058.TBsaUTXu4T@localhost.localdomain>

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

  reply	other threads:[~2021-08-31 12:19 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
2021-08-31 11:51                 ` Johan Hovold
2021-08-31 11:50               ` Fabio M. De Francesco
2021-08-31 12:18                 ` Johan Hovold [this message]
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=YS4eFP3xXdAugyYL@hovoldconsulting.com \
    --to=johan@kernel.org \
    --cc=elder@kernel.org \
    --cc=elder@linaro.org \
    --cc=fmdefrancesco@gmail.com \
    --cc=greybus-dev@lists.linaro.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).