Message-ID: <8df20a3a-3068-1fb7-0421-e6c417550125@embeddedor.com>
Date:   Tue, 16 Apr 2019 15:49:20 -0500
From:   "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To:     Miquel Raynal <miquel.raynal@...tlin.com>
Cc:     David Woodhouse <dwmw2@...radead.org>,
        Brian Norris <computersforpeace@...il.com>,
        Boris Brezillon <bbrezillon@...nel.org>,
        Marek Vasut <marek.vasut@...il.com>,
        Richard Weinberger <richard@....at>,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH] mtd: cfi_util: mark expected switch fall-throughs

Hi Miquel,

On 4/16/19 12:24 PM, Miquel Raynal wrote:
> Hi Gustavo,
> 
> "Gustavo A. R. Silva" <gustavo@...eddedor.com> wrote on Mon, 15 Apr
> 2019 07:57:11 -0500:
> 
>> Hi Miquel,
>>
>> On 4/15/19 3:44 AM, Miquel Raynal wrote:
>>> Hi Gustavo,
>>>
>>> "Gustavo A. R. Silva" <gustavo@...eddedor.com> wrote on Wed, 10 Apr
>>> 2019 16:16:51 -0500:
>>>   
>>>> Hi all,
>>>>
>>>> If no one cares I'll add this to my tree for 5.2.  
>>>
>>> Which tree are you talking about?
>>>   
>>
>> This one:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/log/?h=for-next/kspp
>>
>>> Please let the MTD maintainers take patches through their tree. We
>>> might be late but this is definitely not a good reason to bypass us.
>>>   
>> It's a bit confusing when patches are being ignored for more than two
>> months:
>>
>> https://lore.kernel.org/patchwork/patch/1040099/
>> https://lore.kernel.org/patchwork/patch/1040100/
>> https://lore.kernel.org/patchwork/patch/1040098/
>>
> 
> Patches posted at -rc6 right before the last release? Come on! Gustavo,
> we always spend more time for you than for other contributors because we
> do not trust your changes. We could apply them blindly but we don't do
> that for other (worthy) contributions, so why shall we do it for you?
> 

Oh, I didn't know about that. You don't have to blindly trust me.
I sincerely think people should always double check any changes,
regardless of their level of trust in a particular person/entity.

Anyway, I really appreciate your sincerity because now I think we can
come up with a good strategy to collaborate with each other smoothly.

It seems to me that the root cause for this lack of trust, and, maybe
even despite towards these type of patches, is basically misunderstanding
of what I'm trying to accomplish and, more importantly, how I do it.

> I think you could at least flag these changes as "automatic and
> unverified" in the commit log so that when git blaming, people could
> know that the additional explicit /* fallthrough */ comment might be
> wrong and was just added in order to limit the number of warnings when
> enabling the extra GCC warning.
> 

I don't do that because that's not how I'm tackling this task.

I'm not sending these patches with the intention of merely accumulate
contributions --and I'm not saying you say so, this is just for
clarification-- or because of a lack of more technically interesting
things to do in the kernel --this is certainly not the only thing I'm
working on.  What I'm trying to accomplish is to be able to add
-Wimplicit-fallthrough to the build so that the kernel will stay
entirely free of this class of bug going forward; is that simple. Now,
why is that? because sometimes people forget to place a break/return
and a bug is introduced, and it could take up to 7 years to fix it [1].

Now, I really try to determine if I'm dealing with a false positive or
an actual bug every time. I read the code and try to understand the
context around which each warning is reported. You can tell it's not
the most sexy and glamorous thing. And a static analyzer is clearly not
sophisticated enough to spot actual bugs in this situation, not even
the Coccinelle tool.

I had a similar conversation with a wireless maintainer a while ago. He
claimed I was not even looking at the code and that I was blindly using
a transformation tool [2]. Please, take a look at it, so you can better
understand my workflow.

I have gone through this process of reading code all over the tree and
trying to understand it hundreds of times; there were more than 2000 of
these warnings at the time I started working on this, and there are are
around 50 left in linux-next.  Of course, the vast majority of cases have
resulted to be obvious false positives, but it's me who have determined
that, by auditing each case, so I haven't blindly placed any fall-through
comment.

Now, have I made any mistake? Of course! but I have also amended it
immediately [3][4]. And the number of bugs I have fixed while working
on this task is much bigger.  A clear example of how hard this can be is
documented in this thread, in which you, being an MTD maintainer, cannot
clearly determine if this is a false positive or an actual bug [5]. It
can be troublesome for you for a number of reasons --I'm not judging that.
I'm trying to illustrate the magnitude of the task as a whole.

So, this patches together with the related bugfixes are part of that
whole. And, although sometimes painful for everyone, that whole is
what's important, and worth it.

>> Certainly, Richard Weinberger replied to this one. But I couldn't
>> find a tree to which this patch was applied, in case it actually
>> was.
>>
>> It's a common practice for maintainers to reply saying that a patch
>> has been finally applied, and in most cases they also explicitly
>> mention the tree and branch to which it was applied. All this info
>> is really helpful for people working all over the tree.
> 
> It is common practice for contributors to understand what they
> are doing before submitting a change and this is something that you
> clearly don't try to do.
> 

This is too much to say, and sadly, it's not uncommon for even the most
senior people to assume others don't even make an effort to think through
their work, before at least asking. But I have already explained myself
above.

Regarding this:

> Patches posted at -rc6 right before the last release? Come on! Gustavo,

I don't expect people to send an urgent pull-request to merge this
patches into mainline as soon as they arrive, and I have never requested
such thing.

Lastly, what I really want we *all* get out of this conversation is a
better way to collaborate with each other.  For me, and I guess for most
contributors, it's good enough to have a confirmation that the accepted
patch has been applied to a certain branch in a certain tree. I understand
this may sound like an special request, in particular because, currently,
the number of people working all over the tree is not that big, so it
is not that critical for maintainers to adopt certain practices that
benefits this small group of contributors, but thanks to recent initiatives
as The Linux Kernel Mentorship project I think this is going to change and
it will force us all to evolve in the right direction.

By the way, notice that these are the last patches for MTD. :)

Thank you
--
Gustavo

References:

[1] https://lore.kernel.org/patchwork/patch/1042976/
[2] https://lore.kernel.org/patchwork/patch/1002568/
[3] https://lore.kernel.org/patchwork/patch/970617/
[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.1-rc5&id=ad0eaee6195db1db1749dd46b9e6f4466793d178
[5] https://lore.kernel.org/patchwork/patch/1036251/

Powered by blists - more mailing lists