Message ID | 1420016643-19707-1-git-send-email-der.herr@hofr.at |
---|---|
State | Awaiting Upstream, archived |
Delegated to: | David Miller |
Headers | show |
On Wed, Dec 31, 2014 at 04:04:03AM -0500, Nicholas Mc Guire wrote: > This is only removing the comment which is misleading as > wait_for_completion_timeout does not return < 0 thus there > never is anything to be passed on. a small doubt - i am seeing that: unsigned long wait_for_completion_timeout() is calling long wait_for_common() which is again calling long __wait_for_common which is again calling long do_wait_for_common() now the return value from do_wait_for_common can be -ERESTARTSYS, so then what happens when wait_for_completion_timeout return this -ERESTARTSYS as an unsigned value ? it becomes a positive value, and ultimately ctx.result (which is 0) is returned. so then are we just ignoring the error value from do_wait_for_common() ? sudip > > patch is against linux-next 3.19.0-rc1 -next-20141226 > > Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at> > --- > drivers/net/wimax/i2400m/driver.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/net/wimax/i2400m/driver.c b/drivers/net/wimax/i2400m/driver.c > index 9c78090..0a6384e 100644 > --- a/drivers/net/wimax/i2400m/driver.c > +++ b/drivers/net/wimax/i2400m/driver.c > @@ -197,7 +197,6 @@ int i2400m_op_reset(struct wimax_dev *wimax_dev) > result = -ETIMEDOUT; > else if (result > 0) > result = ctx.result; > - /* if result < 0, pass it on */ > mutex_lock(&i2400m->init_mutex); > i2400m->reset_ctx = NULL; > mutex_unlock(&i2400m->init_mutex); > -- > 1.7.10.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 31 Dec 2014, Sudip Mukherjee wrote: > On Wed, Dec 31, 2014 at 04:04:03AM -0500, Nicholas Mc Guire wrote: > > This is only removing the comment which is misleading as > > wait_for_completion_timeout does not return < 0 thus there > > never is anything to be passed on. > > a small doubt - > i am seeing that: > unsigned long wait_for_completion_timeout() is calling > long wait_for_common() which is again calling > long __wait_for_common which is again calling > long do_wait_for_common() > > now the return value from do_wait_for_common can be -ERESTARTSYS, > so then what happens when wait_for_completion_timeout return this -ERESTARTSYS as an unsigned value ? > it becomes a positive value, and ultimately ctx.result (which is 0) is returned. > so then are we just ignoring the error value from do_wait_for_common() ? > ESTARTSYS only can be returned if state matches in do_wait_for_common but wait_for_completion_timemout passes TASK_UNINTERRUPTIBLE so signal_pending_state will return 0 and thus it will never return -ERESTARTSYS. my understanding of the callchain is: wait_for_completion_timemout which is uninterruptible -> wait_for_common(...TASK_UNINTERRUPTIBLE) -> __wait_for_common(...TASK_UNINTERRUPTIBLE) -> do_wait_for_common(...TASK_UNINTERRUPTIBLE) -> signal_pending_state(TASK_UNINTERRUPTIBLE...) static inline int signal_pending_state(long state, struct task_struct *p) { if (!(state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL))) return 0; so wait_for_completion_timemout should return >=0 only thx! hofrat > > > > patch is against linux-next 3.19.0-rc1 -next-20141226 > > > > Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at> > > --- > > drivers/net/wimax/i2400m/driver.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/net/wimax/i2400m/driver.c b/drivers/net/wimax/i2400m/driver.c > > index 9c78090..0a6384e 100644 > > --- a/drivers/net/wimax/i2400m/driver.c > > +++ b/drivers/net/wimax/i2400m/driver.c > > @@ -197,7 +197,6 @@ int i2400m_op_reset(struct wimax_dev *wimax_dev) > > result = -ETIMEDOUT; > > else if (result > 0) > > result = ctx.result; > > - /* if result < 0, pass it on */ > > mutex_lock(&i2400m->init_mutex); > > i2400m->reset_ctx = NULL; > > mutex_unlock(&i2400m->init_mutex); > > -- > > 1.7.10.4 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 31, 2014 at 11:37:11AM +0100, Nicholas Mc Guire wrote: > On Wed, 31 Dec 2014, Sudip Mukherjee wrote: > > > On Wed, Dec 31, 2014 at 04:04:03AM -0500, Nicholas Mc Guire wrote: > > > This is only removing the comment which is misleading as > > > wait_for_completion_timeout does not return < 0 thus there > > > never is anything to be passed on. > > > > a small doubt - > > i am seeing that: > > unsigned long wait_for_completion_timeout() is calling > > long wait_for_common() which is again calling > > long __wait_for_common which is again calling > > long do_wait_for_common() > > > > now the return value from do_wait_for_common can be -ERESTARTSYS, > > so then what happens when wait_for_completion_timeout return this -ERESTARTSYS as an unsigned value ? > > it becomes a positive value, and ultimately ctx.result (which is 0) is returned. > > so then are we just ignoring the error value from do_wait_for_common() ? > > > > ESTARTSYS only can be returned if state matches in do_wait_for_common > but wait_for_completion_timemout passes TASK_UNINTERRUPTIBLE > so signal_pending_state will return 0 and thus it will never > return -ERESTARTSYS. > > my understanding of the callchain is: > wait_for_completion_timemout which is uninterruptible > -> wait_for_common(...TASK_UNINTERRUPTIBLE) > -> __wait_for_common(...TASK_UNINTERRUPTIBLE) > -> do_wait_for_common(...TASK_UNINTERRUPTIBLE) > -> signal_pending_state(TASK_UNINTERRUPTIBLE...) > > static inline int signal_pending_state(long state, struct task_struct *p) > { > if (!(state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL))) > return 0; > > so wait_for_completion_timemout should return >=0 only doubt cleared. thanks sudip > > thx! > hofrat > > > > > > > patch is against linux-next 3.19.0-rc1 -next-20141226 > > > > > > Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at> > > > --- > > > drivers/net/wimax/i2400m/driver.c | 1 - > > > 1 file changed, 1 deletion(-) > > > > > > diff --git a/drivers/net/wimax/i2400m/driver.c b/drivers/net/wimax/i2400m/driver.c > > > index 9c78090..0a6384e 100644 > > > --- a/drivers/net/wimax/i2400m/driver.c > > > +++ b/drivers/net/wimax/i2400m/driver.c > > > @@ -197,7 +197,6 @@ int i2400m_op_reset(struct wimax_dev *wimax_dev) > > > result = -ETIMEDOUT; > > > else if (result > 0) > > > result = ctx.result; > > > - /* if result < 0, pass it on */ > > > mutex_lock(&i2400m->init_mutex); > > > i2400m->reset_ctx = NULL; > > > mutex_unlock(&i2400m->init_mutex); > > > -- > > > 1.7.10.4 > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wimax/i2400m/driver.c b/drivers/net/wimax/i2400m/driver.c index 9c78090..0a6384e 100644 --- a/drivers/net/wimax/i2400m/driver.c +++ b/drivers/net/wimax/i2400m/driver.c @@ -197,7 +197,6 @@ int i2400m_op_reset(struct wimax_dev *wimax_dev) result = -ETIMEDOUT; else if (result > 0) result = ctx.result; - /* if result < 0, pass it on */ mutex_lock(&i2400m->init_mutex); i2400m->reset_ctx = NULL; mutex_unlock(&i2400m->init_mutex);
This is only removing the comment which is misleading as wait_for_completion_timeout does not return < 0 thus there never is anything to be passed on. patch is against linux-next 3.19.0-rc1 -next-20141226 Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at> --- drivers/net/wimax/i2400m/driver.c | 1 - 1 file changed, 1 deletion(-)