lkml.org 
[lkml]   [1999]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch] drivers/net/plip.c
    On Sat, 30 Jan 1999, Alexander Viro wrote:

    > On Sat, 30 Jan 1999, Ely Wilson wrote:
    > > the driver is *still* broken (as per 2.2.0-final release) and the initial
    > > bugfix (from 2.2.0-pre) did not fix the problem in 2.2.0, what it did was
    > > added a 1 in 4 chance that the code would not faulter.
    >
    > Eli, could you care to show me other 3 of 4? Scenario, please.
    > Could you explain your changes, BTW? I mean, show the finite automate and
    > show disable/enable points on it. I *do* care for plip and aside of It
    > Works For Me (tm) stuff I'ld like to see where had I screwed up in my
    > analisys ;-/

    You caught me! I recount my initial statement, your code is superb, perfect,
    the idiology of all good and true. My '1 in 4' lectural stupidity was
    brought on by my misunderstanding the state of the irq while in and
    returning from plip_send_packet(), and also not looking at how a failed
    return with a disabled irq would affect future operation of that routine.

    Let me explain my misunderstanding.

    You enter plip_send_packet from plip_bh, plip_bh then passes the return
    value on to plip_bh_timeout_error for processing. plip_bh_timeout_error
    then goes on to decide how to handle the error based on the return value and
    responses from the port.

    Ok, I bet that was fun putting together. Now if plip_send_packet disables
    the irq, and it does, and then exits with a return value of TIMEOUT before
    enabling irq we face the same problem as before your patch.

    Here's what I _thought_ I saw: (*my comments*)

    /* PLIP_SEND_PACKET --- send a packet */
    static int
    plip_send_packet(struct device *dev, struct net_local *nl,
    struct plip_local *snd, struct plip_local *rcv)
    {
    unsigned short data_addr = PAR_DATA(dev);
    unsigned short nibble_timeout = nl->nibble;
    unsigned char *lbuf;
    unsigned char c0;
    unsigned int cx;

    if (snd->skb == NULL || (lbuf = snd->skb->data) == NULL) {
    printk(KERN_DEBUG "%s: send skb lost\n", dev->name);
    snd->state = PLIP_PK_DONE;
    snd->skb = NULL;
    return ERROR;
    }

    switch (snd->state) {
    case PLIP_PK_TRIGGER:
    (*if all is normal the irq is enabled*)
    if ((inb(PAR_STATUS(dev)) & 0xf8) != 0x80)
    return HS_TIMEOUT;

    /* Trigger remote rx interrupt. */
    outb(0x08, data_addr);
    cx = nl->trigger;
    while (1) {
    udelay(PLIP_DELAY_UNIT);
    spin_lock_irq(&nl->lock);
    if (nl->connection == PLIP_CN_RECEIVE) {
    spin_unlock_irq(&nl->lock);
    /* Interrupted. */
    nl->enet_stats.collisions++;
    (*if all is normal irq is enabled*)
    return OK;
    }
    c0 = inb(PAR_STATUS(dev));
    if (c0 & 0x08) {
    spin_unlock_irq(&nl->lock);
    DISABLE(dev->irq);
    (*get ready to work, we just disabled the irq*)
    synchronize_irq();
    if (nl->connection == PLIP_CN_RECEIVE) {
    /* Interrupted.
    We don't need to enable irq,
    as it is soon disabled. */
    /* Yes, we do. New variant of
    {enable,disable}_irq *counts*
    them. -- AV */
    (*we know better now, so let's enable teh irq before we return*)
    ENABLE(dev->irq);
    nl->enet_stats.collisions++;
    return OK;
    }
    outb(PAR_INTR_OFF, PAR_CONTROL(dev));
    if (net_debug > 2)
    printk(KERN_DEBUG "%s: send
    start\n", dev->name);
    snd->state = PLIP_PK_LENGTH_LSB;
    snd->nibble = PLIP_NB_BEGIN;
    nl->timeout_count = 0;
    (*here we break from our while() block since that is what we do*)
    break;
    }
    spin_unlock_irq(&nl->lock);
    if (--cx == 0) {
    outb(0x00, data_addr);
    (*all is normal, irq should be enabled*)
    return HS_TIMEOUT;
    }
    }

    case PLIP_PK_LENGTH_LSB:
    (*all is great, woah! unless plip_send returns TIMEOUT*)
    if (plip_send(nibble_timeout, data_addr,
    &snd->nibble, snd->length.b.lsb))
    return TIMEOUT;
    snd->state = PLIP_PK_LENGTH_MSB;

    case PLIP_PK_LENGTH_MSB:
    (*all is great, woah! unless plip_send returns TIMEOUT*)
    if (plip_send(nibble_timeout, data_addr,
    &snd->nibble, snd->length.b.msb))
    return TIMEOUT;
    snd->state = PLIP_PK_DATA;
    snd->byte = 0;
    snd->checksum = 0;

    case PLIP_PK_DATA:
    do
    (*all is great, woah! unless plip_send returns TIMEOUT*)
    if (plip_send(nibble_timeout, data_addr,
    &snd->nibble, lbuf[snd->byte]))
    return TIMEOUT;
    while (++snd->byte < snd->length.h);
    do
    snd->checksum += lbuf[--snd->byte];
    while (snd->byte);
    snd->state = PLIP_PK_CHECKSUM;

    case PLIP_PK_CHECKSUM:
    (*all is great, woah! unless plip_send returns TIMEOUT*)
    if (plip_send(nibble_timeout, data_addr,
    &snd->nibble, snd->checksum))
    return TIMEOUT;

    nl->enet_stats.tx_bytes += snd->skb->len;
    dev_kfree_skb(snd->skb);
    nl->enet_stats.tx_packets++;
    snd->state = PLIP_PK_DONE;

    case PLIP_PK_DONE:
    /* Close the connection */
    outb (0x00, data_addr);
    snd->skb = NULL;
    if (net_debug > 2)
    printk(KERN_DEBUG "%s: send end\n", dev->name);
    nl->connection = PLIP_CN_CLOSING;
    nl->is_deferred = 1;
    queue_task(&nl->deferred, &tq_timer);
    outb(PAR_INTR_ON, PAR_CONTROL(dev));
    (*all went well, the sending operations in each stage went fine, so now we
    enable teh irq so we don't break ourselves*)
    ENABLE(dev->irq);
    return OK;
    }
    (*if we're here we probably don't care anyway, something like that.. :) *)
    return OK;
    }


    Okay, there's my braindamage (I thought it was 4, why not 6, I didn't
    count?). Maybe just me, but now when we return from this routine to plip_bh
    with a value of TIMEOUT it gets passed on to plip_bh_timeout_error where we
    wind up with this logic: (*my comments*)

    static int
    plip_bh_timeout_error(struct device *dev, struct net_local *nl,
    struct plip_local *snd, struct plip_local *rcv,
    int error)
    {
    unsigned char c0;
    /*
    * This is tricky. If we got here from the beginning of send (either
    * with ERROR or HS_TIMEOUT) we have IRQ enabled. Otherwise it's
    * already disabled. With the old variant of {enable,disable}_irq()
    * extra disable_irq() was a no-op. Now it became mortal - it's
    * unbalanced and thus we'll never re-enable IRQ (until rmmod plip,
    * that is). So we have to treat HS_TIMEOUT and ERROR from send
    * in a special way.
    */

    spin_lock_irq(&nl->lock);
    if (nl->connection == PLIP_CN_SEND) {

    if (error != ERROR) { /* Timeout */
    (*we have TIMEOUT so it's != ERROR, so let's enter this if() block and
    increment our nl->timeout_count field*)
    nl->timeout_count++;
    if ((error == HS_TIMEOUT
    && nl->timeout_count <= 10)
    || nl->timeout_count <= 3) {
    (*well, it's _not_ == HS_TIMEOUT, and let's assume nl->timeout_count == 4*)
    spin_unlock_irq(&nl->lock);
    /* Try again later */
    return TIMEOUT;
    }
    c0 = inb(PAR_STATUS(dev));
    (*here's that one message that floods my console now and then. Maybe
    becasue I have a p2 pumping bits into a 486 that chokes or whatever,
    I don't know, but it's so bad sometimes that I have been patching each new
    dev kernel I get to change timeout messages as is done in the patch I
    sent you*)
    printk(KERN_WARNING "%s: transmit
    timeout(%d,%02x)\n",
    dev->name, snd->state, c0);
    } else
    error = HS_TIMEOUT;
    nl->enet_stats.tx_errors++;
    nl->enet_stats.tx_aborted_errors++;
    } else if (nl->connection == PLIP_CN_RECEIVE) {
    if (rcv->state == PLIP_PK_TRIGGER) {
    /* Transmission was interrupted. */
    spin_unlock_irq(&nl->lock);
    return OK;
    }
    if (error != ERROR) { /* Timeout */
    if (++nl->timeout_count <= 3) {
    spin_unlock_irq(&nl->lock);
    /* Try again later */
    return TIMEOUT;
    }
    c0 = inb(PAR_STATUS(dev));
    printk(KERN_WARNING "%s: receive
    timeout(%d,%02x)\n",
    dev->name, rcv->state, c0);
    }
    nl->enet_stats.rx_dropped++;
    }
    (*we're going, and error == TIMEOUT still*)
    rcv->state = PLIP_PK_DONE;
    if (rcv->skb) {
    kfree_skb(rcv->skb);
    rcv->skb = NULL;
    }
    snd->state = PLIP_PK_DONE;
    if (snd->skb) {
    dev_kfree_skb(snd->skb);
    snd->skb = NULL;
    }
    (*we're going, and error == TIMEOUT still*)
    spin_unlock_irq(&nl->lock);
    if (error == HS_TIMEOUT) {
    (*uh oh, that might mean irq is still disabled*)
    DISABLE(dev->irq);
    synchronize_irq();
    }
    outb(PAR_INTR_OFF, PAR_CONTROL(dev));
    dev->tbusy = 1;
    nl->connection = PLIP_CN_ERROR;
    outb(0x00, PAR_DATA(dev));

    return TIMEOUT;
    }

    Unfortunately my machines don't agree with me. As much as I really really
    want this event to not happen, it must be, because I can't find any other
    explanation for the dead interface and endless transmit timeout messages
    that plague my machine with the current plip.c minus my patch.

    To explain my '1 in 4(or 5) chance' the chances would be that at any given
    point the state of irq could become unknown or 'out of sync' with the
    general operation of the driver.

    Something else I found while staring at the code last night was:

    plip_bh() passes ERROR as an error value to plip_bh_timeout_error(), in that
    function you will find this code:

    if (error != ERROR) { /* Timeout */
    nl->timeout_count++;
    if ((error == HS_TIMEOUT
    && nl->timeout_count <= 10)
    || nl->timeout_count <= 3) {
    spin_unlock_irq(&nl->lock);
    /* Try again later */
    return TIMEOUT;
    }
    c0 = inb(PAR_STATUS(dev));
    printk(KERN_WARNING "%s: transmit
    timeout(%d,%02x)\n",
    dev->name, snd->state, c0);
    } else
    error = HS_TIMEOUT;

    which effectively turns your ERROR into HS_TIMEOUT, which in turn makes a
    call to disable_irq()

    plip_receive_packet() (called from plip_bh)

    switch (rcv->state) {
    case PLIP_PK_TRIGGER:
    DISABLE(dev->irq);
    (...snip)
    printk(KERN_DEBUG "%s: checksum error\n",
    dev->name);
    return ERROR;
    }
    rcv->state = PLIP_PK_DONE;

    This is unlikely to happen, and my problem lies within the sending operation
    (or so I think). But this would produce the same result. I hadn't noticed
    this before, so.. I'll just fall back on the fact I'm not a kernel guru and
    I miss a lot of detail while trying to realize the concepts before me.

    That's it. I just created macros that would prevent this event from
    happening in all cases. And you want to hear some bold ass truth? It makes
    little difference. My solution? I leave the irq alone by making the macros
    Functional-NOPs (; statements). You want to know why? I don't see any
    reason to disable the irq. afaik the PLIP protocol itself relies on each
    end negotiating using PAR_CONTROL_INTER (parport.h) being written it's
    control line. In the event the two ends de-sync the connection attempts to
    resume by sending an ack and continuing (or so I like to believe, hence,
    the secondary-purpose behind the protocol). If this is the case then
    disabling irq yields little or no benefit.

    I've been using this 'technique' for a few days now and have had no
    problems, I would like to have someone else's opinion on this though. What
    advantage would be gained? What problem should it be causing that I'm not
    seeing? if you want me to point out how this driver recovers when de-sync "i
    don't know, grep for 'collision'."

    As a final note on not managing the irq, my xfer is about equal to
    40kbyte/s, the same as under 2.1.1xx

    I look forward to responses, I'm a kernel hacker wanna-be and any
    information sent my way is appreciated, even if you do get verbal, at least
    I'll know. I'm still struggling (between my job and other responsibilities)
    to understand much of how the kernel works. So most of what I'd thought is
    wrong is based on assumption. Things you could help me understand so I can
    shut my trap next time I think something works:

    The state of the irq when bh_plip() is called? I assumed irq is enabled.
    What happens if you disable irq from within a bh function? I assumed the
    state is left unchanged, so if you disable irq it remains disabled until re
    enabled. And will it be re-enabled before the next call to a bh function? I
    assumed no since it didn't appear to be working that way.

    Your code was perfect. It just doesn't seem to work on my machine even
    though it's been working since last August when I found out about the PLIP
    driver in Linux. So, I threw it out and began working from an older plip.c
    that worked not but six days ago. I'd *LOVE* to discuss this problem with
    someone (anyone) who knows something (anything) about the kernel, bh
    function handling internals and irq state surrounding this driver.

    Otherwise I'm just stabbing into darkness.

    I tried to avoid HS_TIMEOUT in hopes there was a better solution, possibly
    one that Works For Me As Well As You(tm) (can we trademark that?). I'm only
    basing this off the fact that it worked before, there was no HS_TIMEOUT
    before and that irq code was changed since then.

    synchronize_irq();
    }

    Just for the record, if irq needs a sync, it's handled within enable_irq(),
    i tried to point that out months ago but the only change made was the one
    line I made mention of it on. I could care less, though, I'm capable.

    Okay, now what did you want? Oh, of course, if you would like me to explain
    what you can read in the diff I sent you I can, but it's not terribly
    complicated. I had moved sections of code to more appropriate locations
    (such as #defines where you'd expect to find them, structs, functions et
    cetera), that might throw you off a bit.

    ----------------------------------------------------------
    ---- ely --- <plexus@ionet.net> ------
    "Persuasion consists of justifying the end by the means.."
    ----------------------------------------------------------


    -
    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:50    [W:0.046 / U:0.852 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site