Re: [PATCH RFC] wimax/i2400m: fixup completion handling for resetting a device

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

Hello.

On 03/20/2015 10:47 AM, Nicholas Mc Guire wrote:

    Sorry for late reply, I'm pretty busy these days.

>>> wait_for_completion_timeout return 0 (timeout) or >=1 (completion) so the check
>>> for > 0 in the else branch is always true and can be dropped. The comment seems
>>> misleading as it is always going to pass the result up.

>>> The sync of the completion access with __i2400m_dev_reset_handle (which checks
>>> for   if (i2400m->reset_ctx)   could race if i2400m_reset() returns negative so
>>> the resetting of i2400m->reset_ctx == NULL is moved to the out: path.

>>> As wait_for_completion_timeout returns unsigned long not int, an appropriately
>>> named variable of type unsigned long is added and assignments fixed up.

>>     Don't try to do several things in one patch.

> normaly yes - this was marked as RFC and if I had split it up into
> 3 patches it would be hard to see how it fits together without
> actually applying them.

    You could summarize your intent in the cover letter (PATCH #0).

> The intent was to get feedback notably on moving i2400m->reset_ctx == NULL
> and if dropping the (I think missleading) comment about negative return is ok

> Should this be in seperate patches even as RFC ?

    I think the RFC patches should still conform to all the usual patch rules. 
How would we understand whether you intent to split the patch up later, if you 
didn't even write about it anywhere?

[...]

WBR, Sergei