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

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

[Nicholas Mc Guire]

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.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Not completely sure if moving i2400m->reset_ctx == NULL is really necessary,
but I was not able to see what would prevent completion from being called
after it went out of context (struct completion is on the stack here)
This change needs a review by someone that knows the details of the reset
cases.

Patch was only compile tested with x86_64_defconfig + CONFIG_WIMAX=m
CONFIG_WIMAX_I2400M_USB=m (implies CONFIG_WIMAX_I2400M=m)

Patch is against 4.0-rc4 (localversion-next is -next-20150317)

 drivers/net/wimax/i2400m/driver.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wimax/i2400m/driver.c b/drivers/net/wimax/i2400m/driver.c
index 9c78090..bc98b64 100644
--- a/drivers/net/wimax/i2400m/driver.c
+++ b/drivers/net/wimax/i2400m/driver.c
@@ -178,6 +178,7 @@ static
 int i2400m_op_reset(struct wimax_dev *wimax_dev)
 {
 	int result;
+	unsigned long time_left;
 	struct i2400m *i2400m = wimax_dev_to_i2400m(wimax_dev);
 	struct device *dev = i2400m_dev(i2400m);
 	struct i2400m_reset_ctx ctx = {
@@ -192,17 +193,17 @@ int i2400m_op_reset(struct wimax_dev *wimax_dev)
 	result = i2400m_reset(i2400m, I2400M_RT_WARM);
 	if (result < 0)
 		goto out;
-	result = wait_for_completion_timeout(&ctx.completion, 4*HZ);
-	if (result == 0)
+	time_left = wait_for_completion_timeout(&ctx.completion, 4 * HZ);
+	if (!time_left)
 		result = -ETIMEDOUT;
-	else if (result > 0)
+	else
 		result = ctx.result;
-	/* if result < 0, pass it on */
+
+out:
+	d_fnend(4, dev, "(wimax_dev %p) = %d\n", wimax_dev, result);
 	mutex_lock(&i2400m->init_mutex);
 	i2400m->reset_ctx = NULL;
 	mutex_unlock(&i2400m->init_mutex);
-out:
-	d_fnend(4, dev, "(wimax_dev %p) = %d\n", wimax_dev, result);
 	return result;
 }
 
-- 
1.7.10.4

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

[Sergei Shtylyov]

Hello.

On 3/17/2015 12:49 PM, Nicholas Mc Guire wrote:

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

> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>

[...]

WBR, Sergei

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

[Nicholas Mc Guire]

On Wed, 18 Mar 2015, Sergei Shtylyov wrote:

> Hello.
>
> On 3/17/2015 12:49 PM, Nicholas Mc Guire wrote:
>
>> 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.

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 ?
Once that is clarified it will go out as 3 patchs.

thx!
hofrat

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

[Sergei Shtylyov]

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

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

[Nicholas Mc Guire]

On Fri, 27 Mar 2015, Sergei Shtylyov wrote:

> Hello.
>
> On 03/20/2015 10:47 AM, Nicholas Mc Guire wrote:
>
>    Sorry for late reply, I'm pretty busy these days.

no hurry on this - this is cleanup work only 

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

ok - in that case I will repost as you suggested - just thought it
is more readable to keep it in one patch for resolving the open
questions.

>> 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?
>

I had assumed that a RFC is not intended to be applied anywhere buyt only for review - will clean it up and put the relevant patched code snippet
in #0 then for review.

thx!
hofrat