Re: [PATCH] DVB: dvb_frontend: fix delayed thread exit

[Mauro Carvalho Chehab <mchehab@redhat.com>]

On 10-12-2011 11:43, Devin Heitmueller wrote:
> Hello Mauro,
>
> On Sat, Dec 10, 2011 at 5:28 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com>  wrote:
>> Devin,
>>
>> You're over-reacting. This patch were a reply from Andreas to a thread,
>> and not a separate patch submission.
>>
>> Patches like are generally handled as RFC, especially since it doesn't
>> contain a description.
>
> Any email that starts with "WTF, Devin, you again?" will probably not
> get a very polite response.
>
> I agree there's been some overreaction, but it hasn't been on my part.
>   He took the time to split it onto a new thread, add the subject line
> "PATCH", as well as adding an SOB.  Even if his intent was only to get
> it reviewed, why should I waste half an hour of my time analyzing his
> patch to try to figure out his intent if he isn't willing to simply
> document it?

Both overacted, but this doesn't bring anything good.

> You have a history of blindly accepting such patches without review.

No. I always review all patches I receive. Yeah, I have to confess:
I'm not a robot, I'm not infallible ;) (well, even a robot would
hardly be infallible, anyway).

> My only intent was to flag this patch to ensure that this didn't
> happen here.  I've spent way more time than I should have to fixing
> obscure race conditions in dvb core.  If the author of a patch cannot
> take the time to document his findings to provide context then the
> patch should be rejected without review until he does so.
>
> And why isn't this broken into a patch series?  Even after you
> analyzed the patch you still don't understand what the changes do and
> why there are being made.  Your explanation for why he added the
> "mb()" call was because "Probably Andreas added it because he noticed
> some race condition".  What is the race condition?  Did he find
> multiple race conditions?  Is this patch multiple fixes for a race
> condition and some other crap at the same time?
>
> If a developer wants a patch reviewed (as Andreas suggested was the
> case here after-the-fact), then here's my feedback:  break this into a
> series of small incremental patches which *in detail* describe the
> problem that was found and how each patch addresses the issue.  Once
> we have that, the maintainer can do a more in-depth analysis of
> whether the patch should be accepted.  Code whose function cannot be
> explicitly justified but simply 'looks better' should not be mixed in
> with real functional changes.

I understand that you want patches better documented, so do I, and it
would be great if this patch had a better description since the beginning.

Yet, I don't agree that this patch should be split. It does just one
thing: it fixes the timeout handling for the dvb core frontend thread.

Regards,
Mauro