lkml.org 
[lkml]   [1998]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    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    [W:0.035 / U:0.844 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site