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



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