From: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To: Johan Hovold <johan@kernel.org>, Alex Elder <elder@linaro.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
Subject: Re: [greybus-dev] [PATCH v4] staging: greybus: Convert uart.c from IDR to XArray
Date: Wed, 01 Sep 2021 15:56:20 +0200 [thread overview]
Message-ID: <8914101.vIO1HAjRha@localhost.localdomain> (raw)
In-Reply-To: <794b3ff8-0240-ff14-8721-cdf510f52be3@linaro.org>
On Wednesday, September 1, 2021 2:09:16 PM CEST Alex Elder wrote:
> On 8/31/21 6:50 AM, Fabio M. De Francesco wrote:
> > I was wrong in assuming that trivial patches to Greybus are welcome as
they
> > are for other drivers.
>
> This is not a correct statement.
Yes, I agree: it's not a correct statement. Please let me explain what I was
trying to convey with that consideration...
The Mutexes were there around idr_find() and I decided to leave the code as
it was. Who am I to say that they are not necessary? I must stay on the safe
side. First because I don't know how the drivers work (can that critical
section really be entered by different threads that could possibly share the
gb_tty that is retrieved by xa_load()? Even if xa_load() always give you back
the right gb_tty, how do I know if in the while other threads change its
fields or destroy the object? I guess I should stay on the safe side and
leave the Mutexes there, exactly were they were.
These are the reason why v1 was indeed a trivial patch. But v2 *was not*
because you wrote that you were pretty sure they were unneeded and you asked
me to leave them or remove them and in either case I had to provide a reason
why.
I guess that in v1 I should not provide a reason why they are still there, as
well as I don't have to provide any reason on why the greybus code (line by
line) is as it is: it is out of the scope of my patch. Am I wrong?
Your note about the possibility that the mutexes could be removed pushed me
beyond what I need to know to accomplish the intended task.
Anyway I tried to reason about it. I perfectly know what is required to
protect critical sections of code, but I don't know how drivers work; I mean
I don't know whether or not different threads that run concurrently could
really interfere in that specific section. This is because I simply reason in
terms of general rules of protection of critical section but I really don't
know how Greybus works or (more in general) how drivers work.
I still think that if I stayed within the bounds of my original purpose I
didn't have to reason about this topic and that the v1 patch was trivial.
v2 was not!
I'm sorry because I'm still not sure if I was able to conveyed what I thought
and still think.
> But as Johan pointed out, even for a trivial patch if you
> must understand the consequences of what the change does.
> If testing is not possible, you must work extra hard to
> ensure your patch is correct.
Again, I don't see any possible harm with the mutexes in place :)
> In the first (or an early) version of your patch I pointed
> out a bug. Later, I suggested
> the lock might not be necessary
> and asked you to either confirm
> it was or explain why it was
> not, but you didn't do that.
This was beyond my knowledge and perhaps unnecessary (sorry if I insist on
that :)).
> I agree that the change appeared trivial, and even sensible,
> but even trivial patches must result in correct code. And
> all patches should have good and complete explanations.
>
> - Alex
Is v2 correct with the mutexes restored where they were? I guess it is.
Thanks for you kind review and the time you spent for me. I appreciated it,
seriously.
Fabio
next prev parent reply other threads:[~2021-09-01 13:56 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
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 [this message]
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=8914101.vIO1HAjRha@localhost.localdomain \
--to=fmdefrancesco@gmail.com \
--cc=elder@kernel.org \
--cc=elder@linaro.org \
--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).