lkml.org 
[lkml]   [1998]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: typo in scsi_obsolete.c


On Sun, 29 Nov 1998, Andrea Arcangeli wrote:

> Reviewing my last patches that gone in the kernel, I noticed a bug I
> introduced by mistake :-(. Excuse me. This is the patch against 2.1.130
> that fix the bug:

> + /* FIXME: last_reset == 0 is allowed */

This comment does not make sense for a variable that wraps around.
As long as I will see such a comment, I will think that the way you fix
the jiffies wrap-around problem will lead to a similar problem to the Y2K
problem at the moment jiffies will wrap around as in earlier linux
versions.

If you use a variable that wraps around to measure a delay without any
flag to tell you if the delay exists or not, you must at least make some
assomptions on some max delay value somewhere. But the code will be
confused every 497.1 days, so this will make no real difference with the
old handling that did not try to be clever. Probably time_[before|after]
does some trick but I don't want to look how they do it since I think this
stuff is broken design.
In the case below:

1 - last_reset is assumed to be some timestamp in the past.
2 - you want to know if MIN_RESET_PERIOD later is in the future.

The formula time_after(jiffies, least_reset + MIN_RESET_PERIOD) looks
like very poor semantic to me. I donnot even want to ever burn a
single neuron to understand it.

I may want to understand something that looks like the following:
is_in_the_future(u_long time_stamp_in_the_past, u_long delay)

This has the advantage to hide the implementation of the timestamp and you
could also get rid of the jiffies thing by encapsulating it in something
like:

inline u_long now(void) {return jiffies};
In my opinion, the code below will believe that a reset is in progress
every 497.1 days even with your changes. If I am right your changes
haven't improved anything but slightly broken it for good SCSI systems
that will not experience any resets for 497 days.

By the way, the ncr53c8xx driver assumes that linux will be rebooted
before the 497 days uptime that wraps jiffies around.

Regards,
Gerard.

> Index: linux/drivers/scsi/scsi_obsolete.c
> diff -u linux/drivers/scsi/scsi_obsolete.c:1.1.1.1 linux/drivers/scsi/scsi_obsolete.c:1.1.1.1.2.1
> --- linux/drivers/scsi/scsi_obsolete.c:1.1.1.1 Fri Nov 20 00:02:54 1998
> +++ linux/drivers/scsi/scsi_obsolete.c Sun Nov 29 17:13:41 1998
> @@ -607,10 +607,9 @@
> if ((++SCpnt->retries) < SCpnt->allowed)
> {
> if ((SCpnt->retries >= (SCpnt->allowed >> 1))
> - /* FIXME: last_reset == 0 is allowed
> - * && !(SCpnt->host->last_reset > 0 */ &&
> - time_before(jiffies, SCpnt->host->last_reset
> - + MIN_RESET_PERIOD)
> + /* FIXME: last_reset == 0 is allowed */
> + && time_after(jiffies, SCpnt->host->last_reset
> + + MIN_RESET_PERIOD)
> && !(SCpnt->flags & WAS_RESET))
> {
> printk("scsi%d channel %d : resetting for second half of retries.\n",
>
>
> Right now I am browsing the scsi code waiting for understanding which is
> the best way to handle the dev_last_reset[] (aic7xxx) and ->last_reset
> cases...


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 13:45    [from the cache]
©2003-2011 Jasper Spaans