LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH RFC] wimax/i2400m: fixup completion handling for resetting a device
@ 2015-03-17  9:49 Nicholas Mc Guire
  2015-03-18 12:29 ` Sergei Shtylyov
  0 siblings, 1 reply; 5+ messages in thread
From: Nicholas Mc Guire @ 2015-03-17  9:49 UTC (permalink / raw)
  To: Inaky Perez-Gonzalez; +Cc: linux-wimax, netdev, linux-kernel, 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH RFC] wimax/i2400m: fixup completion handling for resetting a device
  2015-03-17  9:49 [PATCH RFC] wimax/i2400m: fixup completion handling for resetting a device Nicholas Mc Guire
@ 2015-03-18 12:29 ` Sergei Shtylyov
  2015-03-20  7:47   ` Nicholas Mc Guire
  0 siblings, 1 reply; 5+ messages in thread
From: Sergei Shtylyov @ 2015-03-18 12:29 UTC (permalink / raw)
  To: Nicholas Mc Guire, Inaky Perez-Gonzalez; +Cc: linux-wimax, netdev, linux-kernel

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH RFC] wimax/i2400m: fixup completion handling for resetting a device
  2015-03-18 12:29 ` Sergei Shtylyov
@ 2015-03-20  7:47   ` Nicholas Mc Guire
  2015-03-27 17:19     ` Sergei Shtylyov
  0 siblings, 1 reply; 5+ messages in thread
From: Nicholas Mc Guire @ 2015-03-20  7:47 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: Nicholas Mc Guire, Inaky Perez-Gonzalez, linux-wimax, netdev,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH RFC] wimax/i2400m: fixup completion handling for resetting a device
  2015-03-20  7:47   ` Nicholas Mc Guire
@ 2015-03-27 17:19     ` Sergei Shtylyov
  2015-03-31  0:44       ` Nicholas Mc Guire
  0 siblings, 1 reply; 5+ messages in thread
From: Sergei Shtylyov @ 2015-03-27 17:19 UTC (permalink / raw)
  To: Nicholas Mc Guire
  Cc: Nicholas Mc Guire, Inaky Perez-Gonzalez, linux-wimax, netdev,
	linux-kernel

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH RFC] wimax/i2400m: fixup completion handling for resetting a device
  2015-03-27 17:19     ` Sergei Shtylyov
@ 2015-03-31  0:44       ` Nicholas Mc Guire
  0 siblings, 0 replies; 5+ messages in thread
From: Nicholas Mc Guire @ 2015-03-31  0:44 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: Nicholas Mc Guire, Inaky Perez-Gonzalez, linux-wimax, netdev,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-03-31  0:44 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-17  9:49 [PATCH RFC] wimax/i2400m: fixup completion handling for resetting a device Nicholas Mc Guire
2015-03-18 12:29 ` Sergei Shtylyov
2015-03-20  7:47   ` Nicholas Mc Guire
2015-03-27 17:19     ` Sergei Shtylyov
2015-03-31  0:44       ` Nicholas Mc Guire

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