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