Re: [patch] syncppp.c and skb->priority

Jan Kasprzak (kas@informatics.muni.cz)
Wed, 14 Jul 1999 17:23:01 +0200


[ Alexey Kuznetsov added to the Cc: list - see the end of this mail ]

Gergely Madarasz wrote:
: Hmm...
:
: case LCP_STATE_OPENED:
: #if 0
: /* Remote magic changed -- close session. */
[...]
: Any idea why this is #if 0 ? This should do exactly what you need.

Yes, that's it. I have tested it, and it works. I have added
a warning message for that case (the same way the IFF_RUNNING state changes
are reported):

--- syncppp.c.orig Wed Jul 14 16:55:51 1999
+++ syncppp.c Wed Jul 14 16:55:33 1999
@@ -512,15 +512,15 @@
sppp_ipcp_open (sp);
break;
case LCP_STATE_OPENED:
-#if 0
/* Remote magic changed -- close session. */
+ printk (KERN_WARNING "%s: Remote magic changed - restarting LCP.\n",
+ dev->name);
sp->lcp.state = LCP_STATE_CLOSED;
sp->ipcp.state = IPCP_STATE_CLOSED;
/* Initiate renegotiation. */
sppp_lcp_open (sp);
/* An ACK has already been sent. */
sp->lcp.state = LCP_STATE_ACK_SENT;
-#endif
break;
}
break;

I think we should apply this or a similar patch. Anyone thinks
otherwise? Alan?.

Another change may be to make sppp_close to send the TERM_REQ
packet before the device goes down (the same way as the async ppp works).

The resulting syncppp.c (with TC_PRIO_CONTROL changes and your
IFF_RUNNING patches) seems pretty stable and robust for me.

Maybe we should discuss the IFF_RUNNING stuff with Alexey,
and then submit it to the kernel sources.

Alexey Kuznetsov wrote:
: Changing dev->flags in any place different of dev.c is bug (pretty
: harmless). But if you change it at BH or IRQ, it is serious bug.
:
: I understand your enthusiasm, but try to keep administrative things
: and link state separately yet and to protect things changed by interrupts
: properly.

Alexey, we need the way of telling the kernel the link protocol
on the device is down. We thought the IFF_RUNNING is the proper way
of doing this. I think IFF_UP is the "administrative thing", while the
IFF_RUNNING is the "link state thing"). Unfortunately, they are both
stored in dev->flags so it can't be keeped separately.

When we do not get the the reply to the keepalive packet for
a given time, the syncppp.c should mark the device down (read: clear
the IFF_RUNNING flag). This is/can be done from the timer BH, I think.
The marking the device UP after the LCP/IPCP is finished is done from
the net BH (when the last conf-ack LCP packet is received).

Will the start_bh_atomic()/end_bh_atomic() be sufficient to protect
the dev->flags update? Should there be any special notification to the
upper layers when the line goes up or down? Maybe it will be better to
create some kind of mark_running(struct device *, int flag) function
in dev.c and make the syncppp.c (or other layers which can detect the
link protocol being up or down) using this function.

I think some ethernet chipset supports the carrier detection
on the media and can be modified to use these flags as well (Cis*o
can do that and can warn you when the ethernet is unplugged, I think
we should be able to do the same when the HW permits this).

-Yenya

-- 
\ Jan "Yenya" Kasprzak <kas at fi.muni.cz>       http://www.fi.muni.cz/~kas/
\\ PGP: finger kas at aisa.fi.muni.cz   0D99A7FB206605D7 8B35FCDE05B18A5E //
\\\             Czech Linux Homepage:  http://www.linux.cz/              ///
The new code base has not stabilized enough yet for benchmarking. Be patient,
we'll be sure to delay NT5's release a bit, trust me...              (DaveM)

- 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/