All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] scsi: NCR5380: use msecs_to_jiffies for conversions
@ 2015-01-31  8:13 Nicholas Mc Guire
  2015-01-31 23:20 ` Finn Thain
  0 siblings, 1 reply; 7+ messages in thread
From: Nicholas Mc Guire @ 2015-01-31  8:13 UTC (permalink / raw)
  To: Finn Thain
  Cc: Michael Schmitz, James E.J. Bottomley, linux-scsi, Nicholas Mc Guire

This is only an API consolidation to make things more readable.
Instances of  var * HZ / 1000  are replaced by  msecs_to_jiffies(var).

Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>

v2: the original patch was not taking care of all the dependencies
    as reported by Finn Thain <fthain@telegraphics.com.au> - this
    version now uses the suggested config to check the patch.

Converting milliseconds to jiffies by "val * HZ / 1000" is technically
ok but msecs_to_jiffies(val) is the cleaner solution and handles all
corner cases correctly. This is a minor API cleanup only.

This patch was only compile tested with i386_defconfig + CONFIG_ISA=y
as well as all dependent drivers enabled:
CONFIG_SCSI_LOWLEVEL=y, CONFIG_SCSI_GENERIC_NCR5380=m
CONFIG_SCSI_DMX3191D=m, CONFIG_SCSI_DTC3280=m
CONFIG_SCSI_GENERIC_NCR5380=m, CONFIG_SCSI_GENERIC_NCR5380_MMIO=m
CONFIG_SCSI_PAS16=m, CONFIG_SCSI_T128=m

Patch is against 3.19.0-rc6 -next-20150130

---
 drivers/scsi/NCR5380.c   |   10 +++++-----
 drivers/scsi/g_NCR5380.c |    6 +++---
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 36244d6..35bb93b 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -474,11 +474,11 @@ static void NCR5380_print_phase(struct Scsi_Host *instance)
  */
 #ifndef USLEEP_SLEEP
 /* 20 ms (reasonable hard disk speed) */
-#define USLEEP_SLEEP (20*HZ/1000)
+#define USLEEP_SLEEP msecs_to_jiffies(20)
 #endif
 /* 300 RPM (floppy speed) */
 #ifndef USLEEP_POLL
-#define USLEEP_POLL (200*HZ/1000)
+#define USLEEP_POLL msecs_to_jiffies(200)
 #endif
 #ifndef USLEEP_WAITLONG
 /* RvC: (reasonable time to wait on select error) */
@@ -576,7 +576,7 @@ static int __init __maybe_unused NCR5380_probe_irq(struct Scsi_Host *instance,
 		if ((mask & possible) && (request_irq(i, &probe_intr, 0, "NCR-probe", NULL) == 0))
 			trying_irqs |= mask;
 
-	timeout = jiffies + (250 * HZ / 1000);
+	timeout = jiffies + msecs_to_jiffies(250);
 	probe_irq = NO_IRQ;
 
 	/*
@@ -634,7 +634,7 @@ static void prepare_info(struct Scsi_Host *instance)
 	         "sg_tablesize %d, this_id %d, "
 	         "flags { %s%s%s}, "
 #if defined(USLEEP_POLL) && defined(USLEEP_WAITLONG)
-	         "USLEEP_POLL %d, USLEEP_WAITLONG %d, "
+		 "USLEEP_POLL %lu, USLEEP_WAITLONG %lu, "
 #endif
 	         "options { %s} ",
 	         instance->hostt->name, instance->io_port, instance->n_io_port,
@@ -1348,7 +1348,7 @@ static int NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
 	 * selection.
 	 */
 
-	timeout = jiffies + (250 * HZ / 1000);
+	timeout = jiffies + msecs_to_jiffies(250);
 
 	/* 
 	 * XXX very interesting - we're seeing a bounce where the BSY we 
diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index f35792f..9f978ad 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -57,9 +57,9 @@
  */
 
 /* settings for DTC3181E card with only Mustek scanner attached */
-#define USLEEP_POLL	1
-#define USLEEP_SLEEP	20
-#define USLEEP_WAITLONG	500
+#define USLEEP_POLL	msecs_to_jiffies(1)
+#define USLEEP_SLEEP	msecs_to_jiffies(20)
+#define USLEEP_WAITLONG	msecs_to_jiffies(500)
 
 #define AUTOPROBE_IRQ
 
-- 
1.7.10.4


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

* Re: [PATCH v2] scsi: NCR5380: use msecs_to_jiffies for conversions
  2015-01-31  8:13 [PATCH v2] scsi: NCR5380: use msecs_to_jiffies for conversions Nicholas Mc Guire
@ 2015-01-31 23:20 ` Finn Thain
  2015-02-01  4:20   ` Michael Schmitz
  0 siblings, 1 reply; 7+ messages in thread
From: Finn Thain @ 2015-01-31 23:20 UTC (permalink / raw)
  To: Nicholas Mc Guire; +Cc: Michael Schmitz, James E.J. Bottomley, linux-scsi


On Sat, 31 Jan 2015, Nicholas Mc Guire wrote:

> This is only an API consolidation to make things more readable.
> 
> Instances of  var * HZ / 1000  are replaced by  msecs_to_jiffies(var).

... and some instances of "value" are replaced by "msecs_to_jiffies(value)"
which seems to be completely wrong.

> 
> Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
> 
> v2: the original patch was not taking care of all the dependencies
>     as reported by Finn Thain <fthain@telegraphics.com.au> - this
>     version now uses the suggested config to check the patch.

No. The original patch introduced compiler warnings. v2 changed the format 
specifiers as advised by me. And it also changed g_NCR5380.c (for some 
unstated reason).

The patch revision history should go after a "---" cut line, as is 
described in Documentation/SubmittingPatches.

> 
> Converting milliseconds to jiffies by "val * HZ / 1000" is technically
> ok but msecs_to_jiffies(val) is the cleaner solution and handles all
> corner cases correctly. This is a minor API cleanup only.

Do these corner cases affect constants like 1, 20, 200, 250 or 500 ms?

> 
> This patch was only compile tested with i386_defconfig + CONFIG_ISA=y
> as well as all dependent drivers enabled:
> CONFIG_SCSI_LOWLEVEL=y, CONFIG_SCSI_GENERIC_NCR5380=m
> CONFIG_SCSI_DMX3191D=m, CONFIG_SCSI_DTC3280=m
> CONFIG_SCSI_GENERIC_NCR5380=m, CONFIG_SCSI_GENERIC_NCR5380_MMIO=m
> CONFIG_SCSI_PAS16=m, CONFIG_SCSI_T128=m
> 
> Patch is against 3.19.0-rc6 -next-20150130
> 
> ---
>  drivers/scsi/NCR5380.c   |   10 +++++-----
>  drivers/scsi/g_NCR5380.c |    6 +++---
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
> index 36244d6..35bb93b 100644
> --- a/drivers/scsi/NCR5380.c
> +++ b/drivers/scsi/NCR5380.c
> @@ -474,11 +474,11 @@ static void NCR5380_print_phase(struct Scsi_Host *instance)
>   */
>  #ifndef USLEEP_SLEEP
>  /* 20 ms (reasonable hard disk speed) */
> -#define USLEEP_SLEEP (20*HZ/1000)
> +#define USLEEP_SLEEP msecs_to_jiffies(20)
>  #endif
>  /* 300 RPM (floppy speed) */
>  #ifndef USLEEP_POLL
> -#define USLEEP_POLL (200*HZ/1000)
> +#define USLEEP_POLL msecs_to_jiffies(200)
>  #endif
>  #ifndef USLEEP_WAITLONG
>  /* RvC: (reasonable time to wait on select error) */
> @@ -576,7 +576,7 @@ static int __init __maybe_unused NCR5380_probe_irq(struct Scsi_Host *instance,
>  		if ((mask & possible) && (request_irq(i, &probe_intr, 0, "NCR-probe", NULL) == 0))
>  			trying_irqs |= mask;
>  
> -	timeout = jiffies + (250 * HZ / 1000);
> +	timeout = jiffies + msecs_to_jiffies(250);
>  	probe_irq = NO_IRQ;
>  
>  	/*
> @@ -634,7 +634,7 @@ static void prepare_info(struct Scsi_Host *instance)
>  	         "sg_tablesize %d, this_id %d, "
>  	         "flags { %s%s%s}, "
>  #if defined(USLEEP_POLL) && defined(USLEEP_WAITLONG)
> -	         "USLEEP_POLL %d, USLEEP_WAITLONG %d, "
> +		 "USLEEP_POLL %lu, USLEEP_WAITLONG %lu, "
>  #endif
>  	         "options { %s} ",
>  	         instance->hostt->name, instance->io_port, instance->n_io_port,
> @@ -1348,7 +1348,7 @@ static int NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
>  	 * selection.
>  	 */
>  
> -	timeout = jiffies + (250 * HZ / 1000);
> +	timeout = jiffies + msecs_to_jiffies(250);
>  
>  	/* 
>  	 * XXX very interesting - we're seeing a bounce where the BSY we 
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index f35792f..9f978ad 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -57,9 +57,9 @@
>   */
>  
>  /* settings for DTC3181E card with only Mustek scanner attached */
> -#define USLEEP_POLL	1
> -#define USLEEP_SLEEP	20
> -#define USLEEP_WAITLONG	500
> +#define USLEEP_POLL	msecs_to_jiffies(1)
> +#define USLEEP_SLEEP	msecs_to_jiffies(20)
> +#define USLEEP_WAITLONG	msecs_to_jiffies(500)
>  
>  #define AUTOPROBE_IRQ
>  
> 

-- 

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

* Re: [PATCH v2] scsi: NCR5380: use msecs_to_jiffies for conversions
  2015-01-31 23:20 ` Finn Thain
@ 2015-02-01  4:20   ` Michael Schmitz
  2015-02-01  6:23     ` Finn Thain
  2015-02-01  7:14     ` Nicholas Mc Guire
  0 siblings, 2 replies; 7+ messages in thread
From: Michael Schmitz @ 2015-02-01  4:20 UTC (permalink / raw)
  To: Finn Thain; +Cc: Nicholas Mc Guire, James E.J. Bottomley, linux-scsi

Finn, Nicholas,

> On Sat, 31 Jan 2015, Nicholas Mc Guire wrote:
>
>> This is only an API consolidation to make things more readable.
>>
>> Instances of  var * HZ / 1000  are replaced by  msecs_to_jiffies(var).
>
> ... and some instances of "value" are replaced by 
> "msecs_to_jiffies(value)"
> which seems to be completely wrong.

The values for USLEEP_* are taken to be in units jiffies, according to 
comments in NCR5380.c. Replacing them by the msecs_to_jiffies 
conversion is in fact wrong.

Please drop the changes to g_NCR5380.c for that reason.

Cheers,

   Michael


>
>>
>> Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
>>
>> v2: the original patch was not taking care of all the dependencies
>>     as reported by Finn Thain <fthain@telegraphics.com.au> - this
>>     version now uses the suggested config to check the patch.
>
> No. The original patch introduced compiler warnings. v2 changed the 
> format
> specifiers as advised by me. And it also changed g_NCR5380.c (for some
> unstated reason).
>
> The patch revision history should go after a "---" cut line, as is
> described in Documentation/SubmittingPatches.
>
>>
>> Converting milliseconds to jiffies by "val * HZ / 1000" is technically
>> ok but msecs_to_jiffies(val) is the cleaner solution and handles all
>> corner cases correctly. This is a minor API cleanup only.
>
> Do these corner cases affect constants like 1, 20, 200, 250 or 500 ms?
>
>>
>> This patch was only compile tested with i386_defconfig + CONFIG_ISA=y
>> as well as all dependent drivers enabled:
>> CONFIG_SCSI_LOWLEVEL=y, CONFIG_SCSI_GENERIC_NCR5380=m
>> CONFIG_SCSI_DMX3191D=m, CONFIG_SCSI_DTC3280=m
>> CONFIG_SCSI_GENERIC_NCR5380=m, CONFIG_SCSI_GENERIC_NCR5380_MMIO=m
>> CONFIG_SCSI_PAS16=m, CONFIG_SCSI_T128=m
>>
>> Patch is against 3.19.0-rc6 -next-20150130
>>
>> ---
>>  drivers/scsi/NCR5380.c   |   10 +++++-----
>>  drivers/scsi/g_NCR5380.c |    6 +++---
>>  2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
>> index 36244d6..35bb93b 100644
>> --- a/drivers/scsi/NCR5380.c
>> +++ b/drivers/scsi/NCR5380.c
>> @@ -474,11 +474,11 @@ static void NCR5380_print_phase(struct 
>> Scsi_Host *instance)
>>   */
>>  #ifndef USLEEP_SLEEP
>>  /* 20 ms (reasonable hard disk speed) */
>> -#define USLEEP_SLEEP (20*HZ/1000)
>> +#define USLEEP_SLEEP msecs_to_jiffies(20)
>>  #endif
>>  /* 300 RPM (floppy speed) */
>>  #ifndef USLEEP_POLL
>> -#define USLEEP_POLL (200*HZ/1000)
>> +#define USLEEP_POLL msecs_to_jiffies(200)
>>  #endif
>>  #ifndef USLEEP_WAITLONG
>>  /* RvC: (reasonable time to wait on select error) */
>> @@ -576,7 +576,7 @@ static int __init __maybe_unused 
>> NCR5380_probe_irq(struct Scsi_Host *instance,
>>  		if ((mask & possible) && (request_irq(i, &probe_intr, 0, 
>> "NCR-probe", NULL) == 0))
>>  			trying_irqs |= mask;
>>
>> -	timeout = jiffies + (250 * HZ / 1000);
>> +	timeout = jiffies + msecs_to_jiffies(250);
>>  	probe_irq = NO_IRQ;
>>
>>  	/*
>> @@ -634,7 +634,7 @@ static void prepare_info(struct Scsi_Host 
>> *instance)
>>  	         "sg_tablesize %d, this_id %d, "
>>  	         "flags { %s%s%s}, "
>>  #if defined(USLEEP_POLL) && defined(USLEEP_WAITLONG)
>> -	         "USLEEP_POLL %d, USLEEP_WAITLONG %d, "
>> +		 "USLEEP_POLL %lu, USLEEP_WAITLONG %lu, "
>>  #endif
>>  	         "options { %s} ",
>>  	         instance->hostt->name, instance->io_port, 
>> instance->n_io_port,
>> @@ -1348,7 +1348,7 @@ static int NCR5380_select(struct Scsi_Host 
>> *instance, struct scsi_cmnd *cmd)
>>  	 * selection.
>>  	 */
>>
>> -	timeout = jiffies + (250 * HZ / 1000);
>> +	timeout = jiffies + msecs_to_jiffies(250);
>>
>>  	/*
>>  	 * XXX very interesting - we're seeing a bounce where the BSY we
>> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
>> index f35792f..9f978ad 100644
>> --- a/drivers/scsi/g_NCR5380.c
>> +++ b/drivers/scsi/g_NCR5380.c
>> @@ -57,9 +57,9 @@
>>   */
>>
>>  /* settings for DTC3181E card with only Mustek scanner attached */
>> -#define USLEEP_POLL	1
>> -#define USLEEP_SLEEP	20
>> -#define USLEEP_WAITLONG	500
>> +#define USLEEP_POLL	msecs_to_jiffies(1)
>> +#define USLEEP_SLEEP	msecs_to_jiffies(20)
>> +#define USLEEP_WAITLONG	msecs_to_jiffies(500)
>>
>>  #define AUTOPROBE_IRQ
>>
>>
>
> -- 


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

* Re: [PATCH v2] scsi: NCR5380: use msecs_to_jiffies for conversions
  2015-02-01  4:20   ` Michael Schmitz
@ 2015-02-01  6:23     ` Finn Thain
  2015-02-01  7:14     ` Nicholas Mc Guire
  1 sibling, 0 replies; 7+ messages in thread
From: Finn Thain @ 2015-02-01  6:23 UTC (permalink / raw)
  To: Michael Schmitz; +Cc: Nicholas Mc Guire, James E.J. Bottomley, linux-scsi


On Sun, 1 Feb 2015, Michael Schmitz wrote:

> Finn, Nicholas,
> 
> > On Sat, 31 Jan 2015, Nicholas Mc Guire wrote:
> >
> > > This is only an API consolidation to make things more readable.
> > >
> > > Instances of var * HZ / 1000 are replaced by msecs_to_jiffies(var).
> >
> > ... and some instances of "value" are replaced by 
> > "msecs_to_jiffies(value)" which seems to be completely wrong.
> 
> The values for USLEEP_* are taken to be in units jiffies, according to 
> comments in NCR5380.c. Replacing them by the msecs_to_jiffies conversion 
> is in fact wrong.
> 
> Please drop the changes to g_NCR5380.c for that reason.
> 
> Cheers,
> 
>   Michael
> 

Yes, that's a better way to put it than my regrettably short-tempered 
message.

-- 

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

* Re: [PATCH v2] scsi: NCR5380: use msecs_to_jiffies for conversions
  2015-02-01  4:20   ` Michael Schmitz
  2015-02-01  6:23     ` Finn Thain
@ 2015-02-01  7:14     ` Nicholas Mc Guire
  2015-02-02  1:04       ` Michael Schmitz
  1 sibling, 1 reply; 7+ messages in thread
From: Nicholas Mc Guire @ 2015-02-01  7:14 UTC (permalink / raw)
  To: Michael Schmitz; +Cc: Finn Thain, James E.J. Bottomley, linux-scsi

On Sun, 01 Feb 2015, Michael Schmitz wrote:

> Finn, Nicholas,
>
>> On Sat, 31 Jan 2015, Nicholas Mc Guire wrote:
>>
>>> This is only an API consolidation to make things more readable.
>>>
>>> Instances of  var * HZ / 1000  are replaced by  msecs_to_jiffies(var).
>>
>> ... and some instances of "value" are replaced by  
>> "msecs_to_jiffies(value)"
>> which seems to be completely wrong.
>
> The values for USLEEP_* are taken to be in units jiffies, according to  
> comments in NCR5380.c. Replacing them by the msecs_to_jiffies conversion 
> is in fact wrong.
>
> Please drop the changes to g_NCR5380.c for that reason.
>

right the comment indicates it should be jiffies - my interpretation
of that was that in NCR5380.c 
were  jiffies + (250 * HZ / 1000); constructs would be correctly
converted to e.g. jiffies + msecs_to_jiffies(250)

And defines like USLEEP_POLL are noted to be in jiffies:
* USLEEP_SLEEP - amount of time, in jiffies, to sleep

and then defined correctly as HZ indepenedent values:
#define USLEEP_SLEEP (20*HZ/1000)

and thus should be the same as msecs_to_jiffies(20)

now g_NCR5380.c defines
#define USLEEP_POLL     1
#define USLEEP_SLEEP    20
#define USLEEP_WAITLONG 500

for the DTC3181E card - but without the conversion to ms
from the use in the code though (e.g NCR5380_set_timer) it 
seemed to me that it actually should be jiffeis equivalent ms and
the intent was only to change the USLEEP_POLL and USLEEP_WAITLONG 
settings for the specific device but not the unit and thus it 
shuld have been converted by msecs_to_jiffies(1) resp. 
msecs_to_jiffies(500). The problem with this if it is left in its 
current form is that the timeouts would actually depend on the HZ
setting of the system which is probably not the intent.

Pleas do give this one more look if the argument above is
not sound I appologize for the noise.

thx!
hofrat

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

* Re: [PATCH v2] scsi: NCR5380: use msecs_to_jiffies for conversions
  2015-02-01  7:14     ` Nicholas Mc Guire
@ 2015-02-02  1:04       ` Michael Schmitz
  2015-02-02  7:47         ` Nicholas Mc Guire
  0 siblings, 1 reply; 7+ messages in thread
From: Michael Schmitz @ 2015-02-02  1:04 UTC (permalink / raw)
  To: Nicholas Mc Guire
  Cc: Finn Thain, James E.J. Bottomley, scsi, ronald.van.cuijlenborg

Hi Nicholas,

>> The values for USLEEP_* are taken to be in units jiffies, according to
>> comments in NCR5380.c. Replacing them by the msecs_to_jiffies conversion
>> is in fact wrong.
>>
>> Please drop the changes to g_NCR5380.c for that reason.
>>
>
> right the comment indicates it should be jiffies - my interpretation
> of that was that in NCR5380.c
> were  jiffies + (250 * HZ / 1000); constructs would be correctly
> converted to e.g. jiffies + msecs_to_jiffies(250)

No objection there.

> And defines like USLEEP_POLL are noted to be in jiffies:
> * USLEEP_SLEEP - amount of time, in jiffies, to sleep
>
> and then defined correctly as HZ indepenedent values:
> #define USLEEP_SLEEP (20*HZ/1000
>
> and thus should be the same as msecs_to_jiffies(20)

None here either.

>
> now g_NCR5380.c defines
> #define USLEEP_POLL     1
> #define USLEEP_SLEEP    20
> #define USLEEP_WAITLONG 500
>
> for the DTC3181E card - but without the conversion to ms
> from the use in the code though (e.g NCR5380_set_timer) it
> seemed to me that it actually should be jiffeis equivalent ms and

We can't know that - it's a fair guess, but the way it is currently
defined will see these constants used as jiffies, not ms.

'git blame' is of little help here ...

^1da177e (Linus Torvalds     2005-04-16 15:20:36 -0700  59) /*
settings for DTC3181E card with only Mustek scanner attached */
^1da177e (Linus Torvalds     2005-04-16 15:20:36 -0700  60) #define
USLEEP_POLL 1
^1da177e (Linus Torvalds     2005-04-16 15:20:36 -0700  61) #define
USLEEP_SLEEP        20
^1da177e (Linus Torvalds     2005-04-16 15:20:36 -0700  62) #define
USLEEP_WAITLONG     500

The DTC3181E part dates back to '97. Let's see whether Ronald stilll remembers.

HZ used to be set to 100 in those days, so USLEEP_POLL=1 equates to
10ms, not 1ms

> the intent was only to change the USLEEP_POLL and USLEEP_WAITLONG
> settings for the specific device but not the unit and thus it
> shuld have been converted by msecs_to_jiffies(1) resp.
> msecs_to_jiffies(500). The problem with this if it is left in its
> current form is that the timeouts would actually depend on the HZ
> setting of the system which is probably not the intent.

I think it's safe to assume the code in question predates the option
to configure the scheduling tick frequency, so yes, probably not
intended.

The problem with your replacing jiffies by ms is that this will change
the timing for this particular combination of hardware by an order of
magnitude. That large a change in timing would need to be backed up by
testing on the actual hardware.

Much safer to use

#define USLEEP_POLL    msecs_to_jiffies(10)
#define USLEEP_SLEEP   msecs_to_jiffies(200)
#define USLEEP_WAITLONG        msecs_to_jiffies(5000)

and make sure no change in timing is incurred at all.

Cheers,

  Michael


>
> Pleas do give this one more look if the argument above is
> not sound I appologize for the noise.
>
> thx!
> hofrat

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

* Re: [PATCH v2] scsi: NCR5380: use msecs_to_jiffies for conversions
  2015-02-02  1:04       ` Michael Schmitz
@ 2015-02-02  7:47         ` Nicholas Mc Guire
  0 siblings, 0 replies; 7+ messages in thread
From: Nicholas Mc Guire @ 2015-02-02  7:47 UTC (permalink / raw)
  To: Michael Schmitz
  Cc: Finn Thain, James E.J. Bottomley, scsi, ronald.van.cuijlenborg

On Mon, 02 Feb 2015, Michael Schmitz wrote:

> Hi Nicholas,
> 
> >> The values for USLEEP_* are taken to be in units jiffies, according to
> >> comments in NCR5380.c. Replacing them by the msecs_to_jiffies conversion
> >> is in fact wrong.
> >>
> >> Please drop the changes to g_NCR5380.c for that reason.
> >>
> >
> > right the comment indicates it should be jiffies - my interpretation
> > of that was that in NCR5380.c
> > were  jiffies + (250 * HZ / 1000); constructs would be correctly
> > converted to e.g. jiffies + msecs_to_jiffies(250)
> 
> No objection there.
> 
> > And defines like USLEEP_POLL are noted to be in jiffies:
> > * USLEEP_SLEEP - amount of time, in jiffies, to sleep
> >
> > and then defined correctly as HZ indepenedent values:
> > #define USLEEP_SLEEP (20*HZ/1000
> >
> > and thus should be the same as msecs_to_jiffies(20)
> 
> None here either.
> 
> >
> > now g_NCR5380.c defines
> > #define USLEEP_POLL     1
> > #define USLEEP_SLEEP    20
> > #define USLEEP_WAITLONG 500
> >
> > for the DTC3181E card - but without the conversion to ms
> > from the use in the code though (e.g NCR5380_set_timer) it
> > seemed to me that it actually should be jiffeis equivalent ms and
> 
> We can't know that - it's a fair guess, but the way it is currently
> defined will see these constants used as jiffies, not ms.
> 
> 'git blame' is of little help here ...
> 
> ^1da177e (Linus Torvalds     2005-04-16 15:20:36 -0700  59) /*
> settings for DTC3181E card with only Mustek scanner attached */
> ^1da177e (Linus Torvalds     2005-04-16 15:20:36 -0700  60) #define
> USLEEP_POLL 1
> ^1da177e (Linus Torvalds     2005-04-16 15:20:36 -0700  61) #define
> USLEEP_SLEEP        20
> ^1da177e (Linus Torvalds     2005-04-16 15:20:36 -0700  62) #define
> USLEEP_WAITLONG     500
> 
> The DTC3181E part dates back to '97. Let's see whether Ronald stilll remembers.
> 
> HZ used to be set to 100 in those days, so USLEEP_POLL=1 equates to
> 10ms, not 1ms
>

ok - thats my ignorance - did not think that far - but it makes sense
to me now why the values would be correct without conversion. looking
at linux-2.0.31 (1998) your assumption looks correct

 #ifndef USLEEP_SLEEP
 /* 20 ms (reasonable hard disk speed) */
 #define USLEEP_SLEEP 2
 #endif
 /* 300 RPM (floppy speed) */
 #ifndef USLEEP_POLL
 #define USLEEP_POLL 20
 #endif                                                                          

in linux-2.2.16 this becomes

 #ifndef USLEEP_SLEEP
 /* 20 ms (reasonable hard disk speed) */
 #define USLEEP_SLEEP (20*HZ/1000)
 #endif
 /* 300 RPM (floppy speed) */
 #ifndef USLEEP_POLL
 #define USLEEP_POLL (200*HZ/1000)
 #endif

but no update for the DTC case that stays

 /* settings for DTC3181E card with only Mustek scanner attached */
 #define USLEEP 
 #define USLEEP_POLL     1
 #define USLEEP_SLEEP    20
 #define USLEEP_WAITLONG 500

so the original timing unit does seem to be 100HZ based and your 
conversion below seems to be the right one.

> > the intent was only to change the USLEEP_POLL and USLEEP_WAITLONG
> > settings for the specific device but not the unit and thus it
> > shuld have been converted by msecs_to_jiffies(1) resp.
> > msecs_to_jiffies(500). The problem with this if it is left in its
> > current form is that the timeouts would actually depend on the HZ
> > setting of the system which is probably not the intent.
> 
> I think it's safe to assume the code in question predates the option
> to configure the scheduling tick frequency, so yes, probably not
> intended.
> 
> The problem with your replacing jiffies by ms is that this will change
> the timing for this particular combination of hardware by an order of
> magnitude. That large a change in timing would need to be backed up by
> testing on the actual hardware.
> 
> Much safer to use
> 
> #define USLEEP_POLL    msecs_to_jiffies(10)
> #define USLEEP_SLEEP   msecs_to_jiffies(200)
> #define USLEEP_WAITLONG        msecs_to_jiffies(5000)
> 
> and make sure no change in timing is incurred at all.
>

well your solution definitely is the safer and from looking at
2.0.31 -> 2.2.16 likely the right one and it achieves the goal 
of HZ independent timing.

thanks - will clean it up accordingly

thx!
hofrat 

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

end of thread, other threads:[~2015-02-02  7:47 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-31  8:13 [PATCH v2] scsi: NCR5380: use msecs_to_jiffies for conversions Nicholas Mc Guire
2015-01-31 23:20 ` Finn Thain
2015-02-01  4:20   ` Michael Schmitz
2015-02-01  6:23     ` Finn Thain
2015-02-01  7:14     ` Nicholas Mc Guire
2015-02-02  1:04       ` Michael Schmitz
2015-02-02  7:47         ` Nicholas Mc Guire

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.