[PATCH 1/2] staging: dgnc: use schedule_timeout_interruptible()

[PATCH 1/2] staging: dgnc: use schedule_timeout_interruptible()

[Nicholas Mc Guire]

API consolidation with coccinelle found:
./drivers/staging/dgnc/dgnc_utils.c:16:1-17:
        consolidation with schedule_timeout_*() recommended

This is a 1:1 conversion with respect to schedule_timeout() to the
schedule_timeout_interruptible() helper only - so only an API
consolidation to improve readability. The timeout was being passed
as (ms * HZ) / 1000 but that is not reliable as it allows the timeout
to become 0 for small values of ms. As this cut-off is HZ dependent
this is most likely not intended, so the timeout is converted with 
msecs_to_jiffies which handles all corener-cases correctly.

Patch was compile tested with x86_64_defconfig + CONFIG_STAGING=y,
CONFIG_DGNC=m

Patch is against 4.1-rc5 (localversion-next is -next-20150529)

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---
 drivers/staging/dgnc/dgnc_utils.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_utils.c b/drivers/staging/dgnc/dgnc_utils.c
index f76de82..0cbb8a1 100644
--- a/drivers/staging/dgnc/dgnc_utils.c
+++ b/drivers/staging/dgnc/dgnc_utils.c
@@ -12,7 +12,6 @@
  */
 int dgnc_ms_sleep(ulong ms)
 {
-	__set_current_state(TASK_INTERRUPTIBLE);
-	schedule_timeout((ms * HZ) / 1000);
+	schedule_timeout_interruptible(msecs_to_jiffies(ms));
 	return signal_pending(current);
 }
-- 
1.7.10.4

[PATCH 2/2] staging: dgnc: switch timeout to signed type

[Nicholas Mc Guire]

The schedule_timeout*() helpers take the timeout as signed long, as
ch_close_delay in struct channel_t was not used for other purposes its
type was switched to signed long and the declarations fixed up.

Patch was compile tested with x86_64_defconfig + CONFIG_STAGING=y,
CONFIG_DGNC=m

Patch is against 4.1-rc5 (localversion-next is -next-20150529)

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---
Note that there is a "over 80 char" warning here that was not fixed as
there are quite a few in dgnc_driver.h.

 drivers/staging/dgnc/dgnc_driver.h |    2 +-
 drivers/staging/dgnc/dgnc_utils.c  |    2 +-
 drivers/staging/dgnc/dgnc_utils.h  |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_driver.h b/drivers/staging/dgnc/dgnc_driver.h
index f77fed5..5cbeb4d 100644
--- a/drivers/staging/dgnc/dgnc_driver.h
+++ b/drivers/staging/dgnc/dgnc_driver.h
@@ -320,7 +320,7 @@ struct channel_t {
 	uint		ch_open_count;	/* open count			*/
 	uint		ch_flags;	/* Channel flags		*/
 
-	ulong		ch_close_delay;	/* How long we should drop RTS/DTR for */
+	long		ch_close_delay;	/* How long we should drop RTS/DTR for */
 
 	ulong		ch_cpstime;	/* Time for CPS calculations    */
 
diff --git a/drivers/staging/dgnc/dgnc_utils.c b/drivers/staging/dgnc/dgnc_utils.c
index 0cbb8a1..4f7f86b 100644
--- a/drivers/staging/dgnc/dgnc_utils.c
+++ b/drivers/staging/dgnc/dgnc_utils.c
@@ -10,7 +10,7 @@
  *
  * Returns 0 if timed out, !0 (showing signal) if interrupted by a signal.
  */
-int dgnc_ms_sleep(ulong ms)
+int dgnc_ms_sleep(signed long ms)
 {
 	schedule_timeout_interruptible(msecs_to_jiffies(ms));
 	return signal_pending(current);
diff --git a/drivers/staging/dgnc/dgnc_utils.h b/drivers/staging/dgnc/dgnc_utils.h
index 1164c3a..44cb479 100644
--- a/drivers/staging/dgnc/dgnc_utils.h
+++ b/drivers/staging/dgnc/dgnc_utils.h
@@ -1,6 +1,6 @@
 #ifndef __DGNC_UTILS_H
 #define __DGNC_UTILS_H
 
-int dgnc_ms_sleep(ulong ms);
+int dgnc_ms_sleep(signed long ms);
 
 #endif
-- 
1.7.10.4

Re: [PATCH 2/2] staging: dgnc: switch timeout to signed type

[Dan Carpenter]

On Fri, May 29, 2015 at 06:41:28PM +0200, Nicholas Mc Guire wrote:
> The schedule_timeout*() helpers take the timeout as signed long, as
> ch_close_delay in struct channel_t was not used for other purposes its
> type was switched to signed long and the declarations fixed up.

Uh, we never pass it to schedule_timeout etc and even if we did how
would that matter?  It's either 250 or 0.

What is the bug you are trying to fix and we can help you?

regards,
dan carpenter

Re: [PATCH 2/2] staging: dgnc: switch timeout to signed type

[Nicholas Mc Guire]

On Fri, 29 May 2015, Dan Carpenter wrote:

> On Fri, May 29, 2015 at 06:41:28PM +0200, Nicholas Mc Guire wrote:
> > The schedule_timeout*() helpers take the timeout as signed long, as
> > ch_close_delay in struct channel_t was not used for other purposes its
> > type was switched to signed long and the declarations fixed up.
> 
> Uh, we never pass it to schedule_timeout etc and even if we did how
> would that matter?  It's either 250 or 0.
> 
> What is the bug you are trying to fix and we can help you?
>
static code checkers being unhappy with type mismatch
automatic type conversion is ok if necessary but in this
case it simply is not as the ch_close_delay is only being
used in this one place so why not do it type clean ?
I'll turn the question around - what reason would there be to
go through type conversion if it is not needed ?

thx!
hofrat 

Re: [PATCH 2/2] staging: dgnc: switch timeout to signed type

[Dan Carpenter]

On Fri, May 29, 2015 at 07:21:26PM +0200, Nicholas Mc Guire wrote:
> On Fri, 29 May 2015, Dan Carpenter wrote:
> 
> > On Fri, May 29, 2015 at 06:41:28PM +0200, Nicholas Mc Guire wrote:
> > > The schedule_timeout*() helpers take the timeout as signed long, as
> > > ch_close_delay in struct channel_t was not used for other purposes its
> > > type was switched to signed long and the declarations fixed up.
> > 
> > Uh, we never pass it to schedule_timeout etc and even if we did how
> > would that matter?  It's either 250 or 0.
> > 
> > What is the bug you are trying to fix and we can help you?
> >
> static code checkers being unhappy with type mismatch
> automatic type conversion is ok if necessary but in this
> case it simply is not as the ch_close_delay is only being
> used in this one place so why not do it type clean ?

This seems like a pointless warning.  What does the warning look like?
We pass ms to msecs_to_jiffies() and not to schedule_timeout() so it
seems like somewhere something is confused.

> I'll turn the question around - what reason would there be to
> go through type conversion if it is not needed ?

You can go crazy if you do ever pointless change which a static analysis
tool suggests...

Btw, Smatch says that "ms" is always 250 here, actually.  I was guessing
earlier when I said it could be zero.  Get a smarter static checker
which can read code.  :P

regards,
dan carpenter

Re: [PATCH 2/2] staging: dgnc: switch timeout to signed type

[Nicholas Mc Guire]

On Fri, 29 May 2015, Dan Carpenter wrote:

> On Fri, May 29, 2015 at 07:21:26PM +0200, Nicholas Mc Guire wrote:
> > On Fri, 29 May 2015, Dan Carpenter wrote:
> > 
> > > On Fri, May 29, 2015 at 06:41:28PM +0200, Nicholas Mc Guire wrote:
> > > > The schedule_timeout*() helpers take the timeout as signed long, as
> > > > ch_close_delay in struct channel_t was not used for other purposes its
> > > > type was switched to signed long and the declarations fixed up.
> > > 
> > > Uh, we never pass it to schedule_timeout etc and even if we did how
> > > would that matter?  It's either 250 or 0.
> > > 
> > > What is the bug you are trying to fix and we can help you?
> > >
> > static code checkers being unhappy with type mismatch
> > automatic type conversion is ok if necessary but in this
> > case it simply is not as the ch_close_delay is only being
> > used in this one place so why not do it type clean ?
> 
> This seems like a pointless warning.  What does the warning look like?
> We pass ms to msecs_to_jiffies() and not to schedule_timeout() so it
> seems like somewhere something is confused.

Not really - just my carelessness - the msecs_to_jiffies was not in there
and I fixed up the types first - then put the msecs_to_jiffies in there
to fix up the time conversion ...oh well took the type conversion out
just to put it back in my self...sorry thats a bit braindead.
thanks for catching that.

> 
> > I'll turn the question around - what reason would there be to
> > go through type conversion if it is not needed ?
> 
> You can go crazy if you do ever pointless change which a static analysis
> tool suggests...
> 
> Btw, Smatch says that "ms" is always 250 here, actually.  I was guessing
> earlier when I said it could be zero.  Get a smarter static checker
> which can read code. 

wont blame it on coccinelle - its my scripts that are to blame - but in 
this case it was the cleanup after the fix for the warning that broke
it.

so 2/2 is pointless - sorry for that - pleas just toss it.

thx!
hofrat

Re: [PATCH 1/2] staging: dgnc: use schedule_timeout_interruptible()

[Greg Kroah-Hartman]

On Fri, May 29, 2015 at 06:41:27PM +0200, Nicholas Mc Guire wrote:
> API consolidation with coccinelle found:
> ./drivers/staging/dgnc/dgnc_utils.c:16:1-17:
>         consolidation with schedule_timeout_*() recommended
> 
> This is a 1:1 conversion with respect to schedule_timeout() to the
> schedule_timeout_interruptible() helper only - so only an API
> consolidation to improve readability. The timeout was being passed
> as (ms * HZ) / 1000 but that is not reliable as it allows the timeout
> to become 0 for small values of ms. As this cut-off is HZ dependent
> this is most likely not intended, so the timeout is converted with 
> msecs_to_jiffies which handles all corener-cases correctly.
> 
> Patch was compile tested with x86_64_defconfig + CONFIG_STAGING=y,
> CONFIG_DGNC=m
> 
> Patch is against 4.1-rc5 (localversion-next is -next-20150529)

Can you resend this without these two sentances?  They are not needed
and are just "implied" as you should have done this for every patch
submitted.

thanks,

greg k-h

Re: [PATCH 1/2] staging: dgnc: use schedule_timeout_interruptible()

[Nicholas Mc Guire]

On Sun, 31 May 2015, Greg Kroah-Hartman wrote:

> On Fri, May 29, 2015 at 06:41:27PM +0200, Nicholas Mc Guire wrote:
> > API consolidation with coccinelle found:
> > ./drivers/staging/dgnc/dgnc_utils.c:16:1-17:
> >         consolidation with schedule_timeout_*() recommended
> > 
> > This is a 1:1 conversion with respect to schedule_timeout() to the
> > schedule_timeout_interruptible() helper only - so only an API
> > consolidation to improve readability. The timeout was being passed
> > as (ms * HZ) / 1000 but that is not reliable as it allows the timeout
> > to become 0 for small values of ms. As this cut-off is HZ dependent
> > this is most likely not intended, so the timeout is converted with 
> > msecs_to_jiffies which handles all corener-cases correctly.
> > 
> > Patch was compile tested with x86_64_defconfig + CONFIG_STAGING=y,
> > CONFIG_DGNC=m
> > 
> > Patch is against 4.1-rc5 (localversion-next is -next-20150529)
> 
> Can you resend this without these two sentances?  They are not needed
> and are just "implied" as you should have done this for every patch
> submitted.

The config does allow for some level of variantion (e.g. what hardware
it was compile tested for) and also if it was a module or built-in.

I originally put this below the "---" until I was explicitly ast to put 
it above http://lkml.org/lkml/2015/5/11/552

will fix it up and resend.

thx!
hofrat 

Re: [PATCH 1/2] staging: dgnc: use schedule_timeout_interruptible()

[Greg Kroah-Hartman]

On Sun, May 31, 2015 at 07:54:34AM +0200, Nicholas Mc Guire wrote:
> On Sun, 31 May 2015, Greg Kroah-Hartman wrote:
> 
> > On Fri, May 29, 2015 at 06:41:27PM +0200, Nicholas Mc Guire wrote:
> > > API consolidation with coccinelle found:
> > > ./drivers/staging/dgnc/dgnc_utils.c:16:1-17:
> > >         consolidation with schedule_timeout_*() recommended
> > > 
> > > This is a 1:1 conversion with respect to schedule_timeout() to the
> > > schedule_timeout_interruptible() helper only - so only an API
> > > consolidation to improve readability. The timeout was being passed
> > > as (ms * HZ) / 1000 but that is not reliable as it allows the timeout
> > > to become 0 for small values of ms. As this cut-off is HZ dependent
> > > this is most likely not intended, so the timeout is converted with 
> > > msecs_to_jiffies which handles all corener-cases correctly.
> > > 
> > > Patch was compile tested with x86_64_defconfig + CONFIG_STAGING=y,
> > > CONFIG_DGNC=m
> > > 
> > > Patch is against 4.1-rc5 (localversion-next is -next-20150529)
> > 
> > Can you resend this without these two sentances?  They are not needed
> > and are just "implied" as you should have done this for every patch
> > submitted.
> 
> The config does allow for some level of variantion (e.g. what hardware
> it was compile tested for) and also if it was a module or built-in.
> 
> I originally put this below the "---" until I was explicitly ast to put 
> it above http://lkml.org/lkml/2015/5/11/552

Josh is a maintainer of a subsystem that gets a handful of patches a
release, at the most, he can carefully hand edit them all with no hit to
his productivity.  Me, I deal with a few thousand a release :)

And he's right about the tools, just not the fact of what branch you
made it against, or that you actually test built the thing, those are
not useful things in the changelog to preserve for forever as they don't
help anyone out.

thanks,

greg k-h