From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Schmitz Subject: Re: [PATCH v2] scsi: NCR5380: use msecs_to_jiffies for conversions Date: Sun, 1 Feb 2015 17:20:52 +1300 Message-ID: References: <1422692020-6607-1-git-send-email-der.herr@hofr.at> Mime-Version: 1.0 (Apple Message framework v624) Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mail-pa0-f46.google.com ([209.85.220.46]:42207 "EHLO mail-pa0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752170AbbBAEVT (ORCPT ); Sat, 31 Jan 2015 23:21:19 -0500 Received: by mail-pa0-f46.google.com with SMTP id lj1so68381873pab.5 for ; Sat, 31 Jan 2015 20:21:19 -0800 (PST) In-Reply-To: Sender: linux-scsi-owner@vger.kernel.org List-Id: linux-scsi@vger.kernel.org To: Finn Thain Cc: Nicholas Mc Guire , "James E.J. Bottomley" , linux-scsi@vger.kernel.org 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 >> >> v2: the original patch was not taking care of all the dependencies >> as reported by Finn Thain - 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 >> >> > > --