WAN: new PPP code for generic HDLC

WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

Hi,

I'll follow up with a patch for generic HDLC PPP code.

Current status of generic HDLC is:
- up to 2.6.22 - working.
- 2.6.23.* and 2.6.24.* - Frame Relay and PPP protocols panic instantly,
  breakage caused by a change to netdev code that I overlooked.
- 2.6.25-git - Frame Relay is already fixed, PPP still panics.

The fix to FR was simple (983e23041b28abb113862b2935a85cfb9aab4f5a).

PPP has been using syncppp.c implementation for years, meaning working
as a mid-layer between hardware drivers and syncppp. This situation is
increasingly hard to maintain, and I've decided to write a dedicated
PPP implementation for generic HDLC.

Rationale:
- syncppp assumes no mid-layer between itself and hw drivers - dirty
  hacks must be used to work around this
- syncppp doesn't even try to implement PPP correctly, it looks like
  it was written to work with another specific implementation and then
  left in this state for *teen years
- every protocol (LCP and IPCP) uses different state machine code.
- lack of IPv6 support and adding it is non-trivial.
- I've considered using the generic PPP stack, however it's oriented
  towards async tty and requires the pppd daemon - I guess code to
  interface to it would be more complicated than the new PPP code.

The PPP state machine is distributed over many functions and this is
extremely hard to maintain or even understand. No wonder it's not very
standard-compliant.

Instead my new implementation has:
- a single state machine code for all control protocols (LCP, IPCP,
  IPV6CP, and whatever is needed)
- it's modelled after STD-51
- even more minimalistic than syncppp (no "magic number"), though
  adding optional support (if needed) is simple.
- can actually be maintained.

I have positively tested it on i686 and big-endian ARM, against itself
and against syncppp.c. I can't test it against any other device due to
-ENOHW, so I expect some bugs/problems in/with this code, though I hope
it's better than the current brokenness.

I guess it should go into 2.6.25, not sure about "stable" series.
I will appreciate any feedback, review and/or test results.
-- 
Krzysztof Halasa

[PATCH] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

New synchronous PPP implementation for generic HDLC.

Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl>

diff --git a/drivers/net/wan/Makefile b/drivers/net/wan/Makefile
index d61fef3..3081683 100644
--- a/drivers/net/wan/Makefile
+++ b/drivers/net/wan/Makefile
@@ -14,7 +14,7 @@ obj-$(CONFIG_HDLC_RAW)		+= hdlc_raw.o
 obj-$(CONFIG_HDLC_RAW_ETH)	+= hdlc_raw_eth.o
 obj-$(CONFIG_HDLC_CISCO)	+= hdlc_cisco.o
 obj-$(CONFIG_HDLC_FR)		+= hdlc_fr.o
-obj-$(CONFIG_HDLC_PPP)		+= hdlc_ppp.o	syncppp.o
+obj-$(CONFIG_HDLC_PPP)		+= hdlc_ppp.o
 obj-$(CONFIG_HDLC_X25)		+= hdlc_x25.o
 
 pc300-y				:= pc300_drv.o
diff --git a/drivers/net/wan/hdlc_ppp.c b/drivers/net/wan/hdlc_ppp.c
index 10396d9..b626aae 100644
--- a/drivers/net/wan/hdlc_ppp.c
+++ b/drivers/net/wan/hdlc_ppp.c
@@ -2,7 +2,7 @@
  * Generic HDLC support routines for Linux
  * Point-to-point protocol support
  *
- * Copyright (C) 1999 - 2006 Krzysztof Halasa <khc@pm.waw.pl>
+ * Copyright (C) 1999 - 2008 Krzysztof Halasa <khc@pm.waw.pl>
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of version 2 of the GNU General Public License
@@ -19,87 +19,629 @@
 #include <linux/skbuff.h>
 #include <linux/pkt_sched.h>
 #include <linux/inetdevice.h>
-#include <linux/lapb.h>
-#include <linux/rtnetlink.h>
 #include <linux/hdlc.h>
-#include <net/syncppp.h>
+#include <linux/spinlock.h>
+
+#define DEBUG_CP		0 /* also bytes# to dump */
+#define DEBUG_STATE		0
+#define DEBUG_HARD_HEADER	0
+
+#define HDLC_ADDR_ALLSTATIONS	0xFF
+#define HDLC_CTRL_UI		0x03
+
+#define PID_LCP			0xC021
+#define PID_IP			0x0021
+#define PID_IPCP		0x8021
+#define PID_IPV6		0x0057
+#define PID_IPV6CP		0x8057
+
+enum {IDX_LCP = 0, IDX_IPCP, IDX_IPV6CP, IDX_COUNT};
+enum {CP_CONF_REQ = 1, CP_CONF_ACK, CP_CONF_NAK, CP_CONF_REJ, CP_TERM_REQ,
+      CP_TERM_ACK, CP_CODE_REJ, LCP_PROTO_REJ, LCP_ECHO_REQ, LCP_ECHO_REPLY,
+      LCP_DISC_REQ, CP_CODES};
+#if DEBUG_CP
+static const char *code_names[CP_CODES] = {"0", "ConfReq", "ConfAck",
+					   "ConfNak", "ConfRej", "TermReq",
+					   "TermAck", "CodeRej", "ProtoRej",
+					   "EchoReq", "EchoReply", "Discard"};
+static char debug_buffer[64 + 3 * DEBUG_CP];
+#endif
+
+enum {LCP_OPTION_MRU = 1, LCP_OPTION_ACCM, LCP_OPTION_MAGIC = 5};
+
+struct hdlc_header {
+	u8 address;
+	u8 control;
+	__be16 protocol;
+} __attribute__ ((packed));
+
+struct cp_header {
+	u8 code;
+	u8 id;
+	__be16 len;
+} __attribute__ ((packed));
+
+
+struct proto {
+	struct net_device *dev;
+	struct timer_list timer;
+	unsigned long timeout;
+	u16 pid;		/* protocol ID */
+	u8 state;
+	u8 cr_id;		/* ID of last Configuration-Request */
+	u8 restart_counter;
+};
 
-struct ppp_state {
-	struct ppp_device pppdev;
-	struct ppp_device *syncppp_ptr;
-	int (*old_change_mtu)(struct net_device *dev, int new_mtu);
+struct ppp {
+	struct proto protos[IDX_COUNT];
+	spinlock_t lock;
+	unsigned long last_pong;
+	unsigned int req_timeout, cr_retries, term_retries;
+	unsigned int keepalive_interval, keepalive_timeout;
+	u8 seq;			/* local sequence number for requests */
+	u8 echo_id;		/* ID of last Echo-Request (LCP) */
 };
 
+enum {CLOSED = 0, STOPPED, STOPPING, REQ_SENT, ACK_RECV, ACK_SENT, OPENED,
+      STATES, STATE_MASK = 0xF};
+enum {START = 0, STOP, TO_GOOD, TO_BAD, RCR_GOOD, RCR_BAD, RCA, RCN, RTR, RTA,
+      RUC, RXJ_GOOD, RXJ_BAD, EVENTS};
+enum {INV = 0x10, IRC = 0x20, ZRC = 0x40, SCR = 0x80, SCA = 0x100,
+      SCN = 0x200, STR = 0x400, STA = 0x800, SCJ = 0x1000};
+
+#if DEBUG_STATE
+static const char *state_names[STATES] = {"Closed", "Stopped", "Stopping",
+					  "ReqSent", "AckRecv", "AckSent",
+					  "Opened"};
+static const char *event_names[EVENTS] = {"Start", "Stop", "TO+", "TO-",
+					  "RCR+", "RCR-", "RCA", "RCN",
+					  "RTR", "RTA", "RUC", "RXJ+", "RXJ-"};
+#endif
+
+static struct sk_buff_head tx_queue; /* used when holding the spin lock */
+
 static int ppp_ioctl(struct net_device *dev, struct ifreq *ifr);
 
+static inline struct ppp* get_ppp(struct net_device *dev)
+{
+	return (struct ppp *)dev_to_hdlc(dev)->state;
+}
 
-static inline struct ppp_state* state(hdlc_device *hdlc)
+static inline struct proto* get_proto(struct net_device *dev, u16 pid)
 {
-	return(struct ppp_state *)(hdlc->state);
+	struct ppp *ppp = get_ppp(dev);
+
+	switch (pid) {
+	case PID_LCP:
+		return &ppp->protos[IDX_LCP];
+	case PID_IPCP:
+		return &ppp->protos[IDX_IPCP];
+	case PID_IPV6CP:
+		return &ppp->protos[IDX_IPV6CP];
+	default:
+		return NULL;
+	}
 }
 
+static inline const char* proto_name(u16 pid)
+{
+	switch (pid) {
+	case PID_LCP:
+		return "LCP";
+	case PID_IPCP:
+		return "IPCP";
+	case PID_IPV6CP:
+		return "IPV6CP";
+	default:
+		return NULL;
+	}
+}
 
-static int ppp_open(struct net_device *dev)
+static __be16 ppp_type_trans(struct sk_buff *skb, struct net_device *dev)
 {
-	hdlc_device *hdlc = dev_to_hdlc(dev);
-	int (*old_ioctl)(struct net_device *, struct ifreq *, int);
-	int result;
+	struct hdlc_header *data = (struct hdlc_header*)skb->data;
 
-	dev->priv = &state(hdlc)->syncppp_ptr;
-	state(hdlc)->syncppp_ptr = &state(hdlc)->pppdev;
-	state(hdlc)->pppdev.dev = dev;
+	if (skb->len < sizeof(struct hdlc_header))
+		return htons(ETH_P_HDLC);
+	if (data->address != HDLC_ADDR_ALLSTATIONS ||
+	    data->control != HDLC_CTRL_UI)
+		return htons(ETH_P_HDLC);
 
-	old_ioctl = dev->do_ioctl;
-	state(hdlc)->old_change_mtu = dev->change_mtu;
-	sppp_attach(&state(hdlc)->pppdev);
-	/* sppp_attach nukes them. We don't need syncppp's ioctl */
-	dev->do_ioctl = old_ioctl;
-	state(hdlc)->pppdev.sppp.pp_flags &= ~PP_CISCO;
-	dev->type = ARPHRD_PPP;
-	result = sppp_open(dev);
-	if (result) {
-		sppp_detach(dev);
-		return result;
+	switch (data->protocol) {
+	case __constant_htons(PID_IP):
+		skb_pull(skb, sizeof(struct hdlc_header));
+		return htons(ETH_P_IP);
+
+	case __constant_htons(PID_IPV6):
+		skb_pull(skb, sizeof(struct hdlc_header));
+		return htons(ETH_P_IPV6);
+
+	default:
+		return htons(ETH_P_HDLC);
 	}
+}
 
-	return 0;
+
+static int ppp_hard_header(struct sk_buff *skb, struct net_device *dev,
+			   u16 type, const void *daddr, const void *saddr,
+			   unsigned int len)
+{
+	struct hdlc_header *data;
+#if DEBUG_HARD_HEADER
+	printk(KERN_DEBUG "%s: ppp_hard_header() called\n", dev->name);
+#endif
+
+	skb_push(skb, sizeof(struct hdlc_header));
+	data = (struct hdlc_header*)skb->data;
+
+	data->address = HDLC_ADDR_ALLSTATIONS;
+	data->control = HDLC_CTRL_UI;
+	switch (type) {
+	case ETH_P_IP:
+		data->protocol = htons(PID_IP);
+		break;
+	case ETH_P_IPV6:
+		data->protocol = htons(PID_IPV6);
+		break;
+	case PID_LCP:
+	case PID_IPCP:
+	case PID_IPV6CP:
+		data->protocol = htons(type);
+		break;
+	default:		/* unknown protocol */
+		data->protocol = 0;
+	}
+	return sizeof(struct hdlc_header);
 }
 
 
+static void ppp_tx_flush(void)
+{
+	struct sk_buff *skb;
+	while ((skb = skb_dequeue(&tx_queue)) != NULL)
+		dev_queue_xmit(skb);
+}
 
-static void ppp_close(struct net_device *dev)
+static void ppp_tx_cp(struct net_device *dev, u16 pid, u8 code,
+		      u8 id, unsigned int len, const void *data)
 {
-	hdlc_device *hdlc = dev_to_hdlc(dev);
+	struct sk_buff *skb;
+	struct cp_header *cp;
+	unsigned int magic_len = 0;
+	static u32 magic;
+
+#if DEBUG_CP
+	int i;
+	char *ptr;
+#endif
+
+	if (pid == PID_LCP && (code == LCP_ECHO_REQ || code == LCP_ECHO_REPLY))
+		magic_len = sizeof(magic);
+
+	skb = dev_alloc_skb(sizeof(struct hdlc_header) +
+			    sizeof(struct cp_header) + magic_len + len);
+	if (!skb) {
+		printk(KERN_WARNING "%s: out of memory in ppp_tx_cp()\n",
+		       dev->name);
+		return;
+	}
+	skb_reserve(skb, sizeof(struct hdlc_header));
+
+	cp = (struct cp_header *)skb_put(skb, sizeof(struct cp_header));
+	cp->code = code;
+	cp->id = id;
+	cp->len = htons(sizeof(struct cp_header) + magic_len + len);
+
+	if (magic_len)
+		memcpy(skb_put(skb, magic_len), &magic, magic_len);
+	if (len)
+		memcpy(skb_put(skb, len), data, len);
+
+#if DEBUG_CP
+	BUG_ON(code >= CP_CODES);
+	ptr = debug_buffer;
+	*ptr = '\x0';
+	for (i = 0; i < min_t(unsigned int, magic_len + len, DEBUG_CP); i++) {
+		sprintf(ptr, " %02X", skb->data[sizeof(struct cp_header) + i]);
+		ptr += strlen(ptr);
+	}
+	printk(KERN_DEBUG "%s: TX %s [%s id 0x%X]%s\n", dev->name,
+	       proto_name(pid), code_names[code], id, debug_buffer);
+#endif
 
-	sppp_close(dev);
-	sppp_detach(dev);
+	ppp_hard_header(skb, dev, pid, NULL, NULL, 0);
 
-	dev->change_mtu = state(hdlc)->old_change_mtu;
-	dev->mtu = HDLC_MAX_MTU;
-	dev->hard_header_len = 16;
+	skb->priority = TC_PRIO_CONTROL;
+	skb->dev = dev;
+	skb_reset_network_header(skb);
+	skb_queue_tail(&tx_queue, skb);
 }
 
 
+/* State transition table (compare STD-51)
+   Events                                   Actions
+   TO+  = Timeout with counter > 0          irc = Initialize-Restart-Count
+   TO-  = Timeout with counter expired      zrc = Zero-Restart-Count
+
+   RCR+ = Receive-Configure-Request (Good)  scr = Send-Configure-Request
+   RCR- = Receive-Configure-Request (Bad)
+   RCA  = Receive-Configure-Ack             sca = Send-Configure-Ack
+   RCN  = Receive-Configure-Nak/Rej         scn = Send-Configure-Nak/Rej
+
+   RTR  = Receive-Terminate-Request         str = Send-Terminate-Request
+   RTA  = Receive-Terminate-Ack             sta = Send-Terminate-Ack
+
+   RUC  = Receive-Unknown-Code              scj = Send-Code-Reject
+   RXJ+ = Receive-Code-Reject (permitted)
+       or Receive-Protocol-Reject
+   RXJ- = Receive-Code-Reject (catastrophic)
+       or Receive-Protocol-Reject
+*/
+static int cp_table[EVENTS][STATES] = {
+	/* CLOSED     STOPPED STOPPING REQ_SENT ACK_RECV ACK_SENT OPENED
+	     0           1         2       3       4      5          6    */
+	{IRC|SCR|3,     INV     , INV ,   INV   , INV ,  INV    ,   INV   }, /* START */
+	{   INV   ,      0      ,  0  ,    0    ,  0  ,   0     ,    0    }, /* STOP */
+	{   INV   ,     INV     ,STR|2,  SCR|3  ,SCR|3,  SCR|5  ,   INV   }, /* TO+ */
+	{   INV   ,     INV     ,  1  ,    1    ,  1  ,    1    ,   INV   }, /* TO- */
+	{  STA|0  ,IRC|SCR|SCA|5,  2  ,  SCA|5  ,SCA|6,  SCA|5  ,SCR|SCA|5}, /* RCR+ */
+	{  STA|0  ,IRC|SCR|SCN|3,  2  ,  SCN|3  ,SCN|4,  SCN|3  ,SCR|SCN|3}, /* RCR- */
+	{  STA|0  ,    STA|1    ,  2  ,  IRC|4  ,SCR|3,    6    , SCR|3   }, /* RCA */
+	{  STA|0  ,    STA|1    ,  2  ,IRC|SCR|3,SCR|3,IRC|SCR|5, SCR|3   }, /* RCN */
+	{  STA|0  ,    STA|1    ,STA|2,  STA|3  ,STA|3,  STA|3  ,ZRC|STA|2}, /* RTR */
+	{    0    ,      1      ,  1  ,    3    ,  3  ,    5    ,  SCR|3  }, /* RTA */
+	{  SCJ|0  ,    SCJ|1    ,SCJ|2,  SCJ|3  ,SCJ|4,  SCJ|5  ,  SCJ|6  }, /* RUC */
+	{    0    ,      1      ,  2  ,    3    ,  3  ,    5    ,    6    }, /* RXJ+ */
+	{    0    ,      1      ,  1  ,    1    ,  1  ,    1    ,IRC|STR|2}, /* RXJ- */
+};
 
-static __be16 ppp_type_trans(struct sk_buff *skb, struct net_device *dev)
+
+/* SCA: RCR+ must supply id, len and data
+   SCN: RCR- must supply code, id, len and data
+   STA: RTR must supply id
+   SCJ: RUC must supply CP packet len and data */
+static void ppp_cp_event(struct net_device *dev, u16 pid, u16 event, u8 code,
+			 u8 id, unsigned int len, void *data)
 {
-	return __constant_htons(ETH_P_WAN_PPP);
+	int old_state, action;
+	struct ppp *ppp = get_ppp(dev);
+	struct proto *proto = get_proto(dev, pid);
+
+	old_state = proto->state;
+	BUG_ON(old_state >= STATES);
+	BUG_ON(event >= EVENTS);
+
+#if DEBUG_STATE
+	printk(KERN_DEBUG "%s: %s ppp_cp_event(%s) %s ...\n", dev->name,
+	       proto_name(pid), event_names[event], state_names[proto->state]);
+#endif
+
+	action = cp_table[event][old_state];
+
+	proto->state = action & STATE_MASK;
+	if (action & (SCR | STR)) /* set Configure-Req/Terminate-Req timer */
+		mod_timer(&proto->timer, proto->timeout =
+			  jiffies + ppp->req_timeout * HZ);
+	if (action & ZRC)
+		proto->restart_counter = 0;
+	if (action & IRC)
+		proto->restart_counter = (proto->state == STOPPING) ?
+			ppp->term_retries : ppp->cr_retries;
+
+	if (action & SCR)	/* send Configure-Request */
+		ppp_tx_cp(dev, pid, CP_CONF_REQ, proto->cr_id = ++ppp->seq,
+			  0, NULL);
+	if (action & SCA)	/* send Configure-Ack */
+		ppp_tx_cp(dev, pid, CP_CONF_ACK, id, len, data);
+	if (action & SCN)	/* send Configure-Nak/Reject */
+		ppp_tx_cp(dev, pid, code, id, len, data);
+	if (action & STR)	/* send Terminate-Request */
+		ppp_tx_cp(dev, pid, CP_TERM_REQ, ++ppp->seq, 0, NULL);
+	if (action & STA)	/* send Terminate-Ack */
+		ppp_tx_cp(dev, pid, CP_TERM_ACK, id, 0, NULL);
+	if (action & SCJ)	/* send Code-Reject */
+		ppp_tx_cp(dev, pid, CP_CODE_REJ, ++ppp->seq, len, data);
+
+	if (old_state != OPENED && proto->state == OPENED) {
+		printk(KERN_INFO "%s: %s up\n", dev->name, proto_name(pid));
+		if (pid == PID_LCP) {
+			netif_dormant_off(dev);
+			ppp_cp_event(dev, PID_IPCP, START, 0, 0, 0, NULL);
+			ppp_cp_event(dev, PID_IPV6CP, START, 0, 0, 0, NULL);
+			ppp->last_pong = jiffies;
+			mod_timer(&proto->timer, proto->timeout =
+				  jiffies + ppp->keepalive_interval * HZ);
+		}
+	}
+	if (old_state == OPENED && proto->state != OPENED) {
+		printk(KERN_INFO "%s: %s down\n", dev->name, proto_name(pid));
+		if (pid == PID_LCP) {
+			netif_dormant_on(dev);
+			ppp_cp_event(dev, PID_IPCP, STOP, 0, 0, 0, NULL);
+			ppp_cp_event(dev, PID_IPV6CP, STOP, 0, 0, 0, NULL);
+		}
+	}
+	if (old_state != CLOSED && proto->state == CLOSED)
+		del_timer(&proto->timer);
+
+#if DEBUG_STATE
+	printk(KERN_DEBUG "%s: %s ppp_cp_event(%s) ... %s\n", dev->name,
+	       proto_name(pid), event_names[event], state_names[proto->state]);
+#endif
 }
 
 
+static void ppp_cp_parse_cr(struct net_device *dev, u16 pid, u8 id,
+			    unsigned int len, u8 *data)
+{
+	static u8 valid_accm[6] = { LCP_OPTION_ACCM, 6, 0, 0, 0, 0 };
+	u8 *opt, *out;
+	unsigned int nak_len = 0, rej_len = 0;
+
+	if (!(out = kmalloc(len, GFP_ATOMIC))) {
+		dev_to_hdlc(dev)->stats.rx_dropped++;
+		return;	/* out of memory, ignore CR packet */
+	}
+
+	for (opt = data; len; len -= opt[1], opt += opt[1]) {
+		if (len < 2 || len < opt[1]) {
+			dev_to_hdlc(dev)->stats.rx_errors++;
+			return; /* bad packet, drop silently */
+		}
+
+		if (pid == PID_LCP)
+			switch (opt[0]) {
+			case LCP_OPTION_MRU:
+				continue; /* MRU always OK and > 1500 bytes? */
+
+			case LCP_OPTION_ACCM: /* async control character map */
+				if (!memcmp(opt, valid_accm,
+					    sizeof(valid_accm)))
+					continue;
+				if (!rej_len) { /* NAK it */
+					memcpy(out + nak_len, valid_accm,
+					       sizeof(valid_accm));
+					nak_len += sizeof(valid_accm);
+					continue;
+				}
+				break;
+			case LCP_OPTION_MAGIC:
+				if (opt[1] != 6 || (!opt[2] && !opt[3] &&
+						    !opt[4] && !opt[5]))
+					break; /* reject invalid magic number */
+				continue;
+			}
+		/* reject this option */
+		memcpy(out + rej_len, opt, opt[1]);
+		rej_len += opt[1];
+	}
+
+	if (rej_len)
+		ppp_cp_event(dev, pid, RCR_BAD, CP_CONF_REJ, id, rej_len, out);
+	else if (nak_len)
+		ppp_cp_event(dev, pid, RCR_BAD, CP_CONF_NAK, id, nak_len, out);
+	else
+		ppp_cp_event(dev, pid, RCR_GOOD, CP_CONF_ACK, id, len, data);
+
+	kfree(out);
+}
+
+static int ppp_rx(struct sk_buff *skb)
+{
+	struct hdlc_header *hdr = (struct hdlc_header*)skb->data;
+	struct net_device *dev = skb->dev;
+	struct ppp *ppp = get_ppp(dev);
+	struct proto *proto;
+	struct cp_header *cp;
+	unsigned long flags;
+	unsigned int len;
+	u16 pid;
+#if DEBUG_CP
+	int i;
+	char *ptr;
+#endif
+
+	spin_lock_irqsave(&ppp->lock, flags);
+	/* Check HDLC header */
+	if (skb->len < sizeof(struct hdlc_header))
+		goto rx_error;
+	cp = (struct cp_header*)skb_pull(skb, sizeof(struct hdlc_header));
+	if (hdr->address != HDLC_ADDR_ALLSTATIONS ||
+	    hdr->control != HDLC_CTRL_UI)
+		goto rx_error;
+
+	pid = ntohs(hdr->protocol);
+	proto = get_proto(dev, pid);
+	if (!proto) {
+		if (ppp->protos[IDX_LCP].state == OPENED)
+			ppp_tx_cp(dev, PID_LCP, LCP_PROTO_REJ,
+				  ++ppp->seq, skb->len + 2, &hdr->protocol);
+		goto rx_error;
+	}
+
+	len = ntohs(cp->len);
+	if (len < sizeof(struct cp_header) /* no complete CP header? */ ||
+	    skb->len < len /* truncated packet? */)
+		goto rx_error;
+	skb_pull(skb, sizeof(struct cp_header));
+	len -= sizeof(struct cp_header);
+
+	/* HDLC and CP headers stripped from skb */
+#if DEBUG_CP
+	if (cp->code < CP_CODES)
+		sprintf(debug_buffer, "[%s id 0x%X]", code_names[cp->code],
+			cp->id);
+	else
+		sprintf(debug_buffer, "[code %u id 0x%X]", cp->code, cp->id);
+	ptr = debug_buffer + strlen(debug_buffer);
+	for (i = 0; i < min_t(unsigned int, len, DEBUG_CP); i++) {
+		sprintf(ptr, " %02X", skb->data[i]);
+		ptr += strlen(ptr);
+	}
+	printk(KERN_DEBUG "%s: RX %s %s\n", dev->name, proto_name(pid),
+	       debug_buffer);
+#endif
+
+	/* LCP only */
+	if (pid == PID_LCP)
+		switch (cp->code) {
+		case LCP_PROTO_REJ:
+			pid = ntohs(*(__be16*)skb->data);
+			if (pid == PID_LCP || pid == PID_IPCP ||
+			    pid == PID_IPV6CP)
+				ppp_cp_event(dev, pid, RXJ_BAD, 0, 0,
+					     0, NULL);
+			goto out;
+
+		case LCP_ECHO_REQ: /* send Echo-Reply */
+			if (len >= 4 && proto->state == OPENED)
+				ppp_tx_cp(dev, PID_LCP, LCP_ECHO_REPLY,
+					  cp->id, len - 4, skb->data + 4);
+			goto out;
+
+		case LCP_ECHO_REPLY:
+			if (cp->id == ppp->echo_id)
+				ppp->last_pong = jiffies;
+			goto out;
+
+		case LCP_DISC_REQ: /* discard */
+			goto out;
+		}
+
+	/* LCP, IPCP and IPV6CP */
+	switch (cp->code) {
+	case CP_CONF_REQ:
+		ppp_cp_parse_cr(dev, pid, cp->id, len, skb->data);
+		goto out;
+
+	case CP_CONF_ACK:
+		if (cp->id == proto->cr_id)
+			ppp_cp_event(dev, pid, RCA, 0, 0, 0, NULL);
+		goto out;
+
+	case CP_CONF_REJ:
+	case CP_CONF_NAK:
+		if (cp->id == proto->cr_id)
+			ppp_cp_event(dev, pid, RCN, 0, 0, 0, NULL);
+		goto out;
+
+	case CP_TERM_REQ:
+		ppp_cp_event(dev, pid, RTR, 0, cp->id, 0, NULL);
+		goto out;
+
+	case CP_TERM_ACK:
+		ppp_cp_event(dev, pid, RTA, 0, 0, 0, NULL);
+		goto out;
+
+	case CP_CODE_REJ:
+		ppp_cp_event(dev, pid, RXJ_BAD, 0, 0, 0, NULL);
+		goto out;
+
+	default:
+		len += sizeof(struct cp_header);
+		if (len > dev->mtu)
+			len = dev->mtu;
+		ppp_cp_event(dev, pid, RUC, 0, 0, len, cp);
+		goto out;
+	}
+	goto out;
+
+rx_error:
+	dev_to_hdlc(dev)->stats.rx_errors++;
+out:
+	spin_unlock_irqrestore(&ppp->lock, flags);
+	dev_kfree_skb_any(skb);
+	ppp_tx_flush();
+	return NET_RX_DROP;
+}
+
+
+static void ppp_timer(unsigned long arg)
+{
+	struct proto *proto = (struct proto *)arg;
+	struct ppp *ppp = get_ppp(proto->dev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&ppp->lock, flags);
+	switch (proto->state) {
+	case STOPPING:
+	case REQ_SENT:
+	case ACK_RECV:
+	case ACK_SENT:
+		if (proto->restart_counter) {
+			ppp_cp_event(proto->dev, proto->pid, TO_GOOD, 0, 0,
+				     0, NULL);
+			proto->restart_counter--;
+		} else
+			ppp_cp_event(proto->dev, proto->pid, TO_BAD, 0, 0,
+				     0, NULL);
+		break;
+
+	case OPENED:
+		if (proto->pid != PID_LCP)
+			break;
+		if (time_after(jiffies, ppp->last_pong +
+			       ppp->keepalive_timeout * HZ)) {
+			printk(KERN_INFO "%s: Link down\n", proto->dev->name);
+			ppp_cp_event(proto->dev, PID_LCP, STOP, 0, 0, 0, NULL);
+			ppp_cp_event(proto->dev, PID_LCP, START, 0, 0, 0, NULL);
+		} else {	/* send keep-alive packet */
+			ppp->echo_id = ++ppp->seq;
+			ppp_tx_cp(proto->dev, PID_LCP, LCP_ECHO_REQ,
+				  ppp->echo_id, 0, NULL);
+			proto->timer.expires = jiffies +
+				ppp->keepalive_interval * HZ;
+			add_timer(&proto->timer);
+		}
+		break;
+	}
+	spin_unlock_irqrestore(&ppp->lock, flags);
+	ppp_tx_flush();
+}
+
+
+static void ppp_start(struct net_device *dev)
+{
+	struct ppp *ppp = get_ppp(dev);
+	int i;
+
+	for (i = 0; i < IDX_COUNT; i++) {
+		struct proto *proto = &ppp->protos[i];
+		proto->dev = dev;
+		init_timer(&proto->timer);
+		proto->timer.function = ppp_timer;
+		proto->timer.data = (unsigned long)proto;
+		proto->state = CLOSED;
+	}
+	ppp->protos[IDX_LCP].pid = PID_LCP;
+	ppp->protos[IDX_IPCP].pid = PID_IPCP;
+	ppp->protos[IDX_IPV6CP].pid = PID_IPV6CP;
+
+	ppp_cp_event(dev, PID_LCP, START, 0, 0, 0, NULL);
+}
+
+static void ppp_stop(struct net_device *dev)
+{
+	ppp_cp_event(dev, PID_LCP, STOP, 0, 0, 0, NULL);
+}
 
 static struct hdlc_proto proto = {
-	.open		= ppp_open,
-	.close		= ppp_close,
+	.start		= ppp_start,
+	.stop		= ppp_stop,
 	.type_trans	= ppp_type_trans,
 	.ioctl		= ppp_ioctl,
+	.netif_rx	= ppp_rx,
 	.module		= THIS_MODULE,
 };
 
+static const struct header_ops ppp_header_ops = {
+	.create = ppp_hard_header,
+};
 
 static int ppp_ioctl(struct net_device *dev, struct ifreq *ifr)
 {
 	hdlc_device *hdlc = dev_to_hdlc(dev);
+	struct ppp *ppp;
 	int result;
 
 	switch (ifr->ifr_settings.type) {
@@ -110,25 +652,35 @@ static int ppp_ioctl(struct net_device *dev, struct ifreq *ifr)
 		return 0; /* return protocol only, no settable parameters */
 
 	case IF_PROTO_PPP:
-		if(!capable(CAP_NET_ADMIN))
+		if (!capable(CAP_NET_ADMIN))
 			return -EPERM;
 
-		if(dev->flags & IFF_UP)
+		if (dev->flags & IFF_UP)
 			return -EBUSY;
 
 		/* no settable parameters */
 
-		result=hdlc->attach(dev, ENCODING_NRZ,PARITY_CRC16_PR1_CCITT);
+		result = hdlc->attach(dev, ENCODING_NRZ,PARITY_CRC16_PR1_CCITT);
 		if (result)
 			return result;
 
-		result = attach_hdlc_protocol(dev, &proto,
-					      sizeof(struct ppp_state));
+		result = attach_hdlc_protocol(dev, &proto, sizeof(struct ppp));
 		if (result)
 			return result;
+
+		ppp = get_ppp(dev);
+		spin_lock_init(&ppp->lock);
+		ppp->req_timeout = 2;
+		ppp->cr_retries = 10;
+		ppp->term_retries = 2;
+		ppp->keepalive_interval = 10;
+		ppp->keepalive_timeout = 60;
+
 		dev->hard_start_xmit = hdlc->xmit;
+		dev->hard_header_len = sizeof(struct hdlc_header);
+		dev->header_ops = &ppp_header_ops;
 		dev->type = ARPHRD_PPP;
-		netif_dormant_off(dev);
+		netif_dormant_on(dev);
 		return 0;
 	}
 
@@ -138,12 +690,11 @@ static int ppp_ioctl(struct net_device *dev, struct ifreq *ifr)
 
 static int __init mod_init(void)
 {
+	skb_queue_head_init(&tx_queue);
 	register_hdlc_protocol(&proto);
 	return 0;
 }
 
-
-
 static void __exit mod_exit(void)
 {
 	unregister_hdlc_protocol(&proto);

Re: [PATCH] Re: WAN: new PPP code for generic HDLC

[Stephen Hemminger]

> +struct hdlc_header {
> +	u8 address;
> +	u8 control;
> +	__be16 protocol;
> +} __attribute__ ((packed));
> +
> +struct cp_header {
> +	u8 code;
> +	u8 id;
> +	__be16 len;
> +} __attribute__ ((packed));
> +

If I remember correctly, the packed is unnecessary for structures like this
and causes GCC to generate worse code.

Re: [PATCH] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

Stephen Hemminger <shemminger@vyatta.com> writes:

>> +struct hdlc_header {
>> +	u8 address;
>> +	u8 control;
>> +	__be16 protocol;
>> +} __attribute__ ((packed));
>> +
>> +struct cp_header {
>> +	u8 code;
>> +	u8 id;
>> +	__be16 len;
>> +} __attribute__ ((packed));
>> +
>
> If I remember correctly, the packed is unnecessary for structures like this
> and causes GCC to generate worse code.

Interesting. Gcc obviously will do the right thing without "packed",
at least on "current" architectures, but I don't know what the C
standard says. I will remove it, thanks.
-- 
Krzysztof Halasa

Re: [PATCH] Re: WAN: new PPP code for generic HDLC

[Jan Engelhardt]

On Mar 12 2008 19:30, Krzysztof Halasa wrote:
>+	/* LCP only */
>+	if (pid == PID_LCP)
>+		switch (cp->code) {
>+		case LCP_PROTO_REJ:
>+			pid = ntohs(*(__be16*)skb->data);

What if skb->data happens to be unaligned (can that even happen?)

BTW, here are some consts:


diff --git a/drivers/net/wan/hdlc_ppp.c b/drivers/net/wan/hdlc_ppp.c
index b626aae..0e4cb1f 100644
--- a/drivers/net/wan/hdlc_ppp.c
+++ b/drivers/net/wan/hdlc_ppp.c
@@ -40,7 +40,7 @@ enum {CP_CONF_REQ = 1, CP_CONF_ACK, CP_CONF_NAK, CP_CONF_REJ, CP_TERM_REQ,
       CP_TERM_ACK, CP_CODE_REJ, LCP_PROTO_REJ, LCP_ECHO_REQ, LCP_ECHO_REPLY,
       LCP_DISC_REQ, CP_CODES};
 #if DEBUG_CP
-static const char *code_names[CP_CODES] = {"0", "ConfReq", "ConfAck",
+static const char *const code_names[] = {"0", "ConfReq", "ConfAck",
 					   "ConfNak", "ConfRej", "TermReq",
 					   "TermAck", "CodeRej", "ProtoRej",
 					   "EchoReq", "EchoReply", "Discard"};
@@ -90,10 +90,10 @@ enum {INV = 0x10, IRC = 0x20, ZRC = 0x40, SCR = 0x80, SCA = 0x100,
       SCN = 0x200, STR = 0x400, STA = 0x800, SCJ = 0x1000};
 
 #if DEBUG_STATE
-static const char *state_names[STATES] = {"Closed", "Stopped", "Stopping",
+static const char *const state_names[] = {"Closed", "Stopped", "Stopping",
 					  "ReqSent", "AckRecv", "AckSent",
 					  "Opened"};
-static const char *event_names[EVENTS] = {"Start", "Stop", "TO+", "TO-",
+static const char *const event_names[] = {"Start", "Stop", "TO+", "TO-",
 					  "RCR+", "RCR-", "RCA", "RCN",
 					  "RTR", "RTA", "RUC", "RXJ+", "RXJ-"};
 #endif
@@ -374,7 +374,7 @@ static void ppp_cp_event(struct net_device *dev, u16 pid, u16 event, u8 code,
 static void ppp_cp_parse_cr(struct net_device *dev, u16 pid, u8 id,
 			    unsigned int len, u8 *data)
 {
-	static u8 valid_accm[6] = { LCP_OPTION_ACCM, 6, 0, 0, 0, 0 };
+	static const u8 valid_accm[] = { LCP_OPTION_ACCM, 6, 0, 0, 0, 0 };
 	u8 *opt, *out;
 	unsigned int nak_len = 0, rej_len = 0;
 

Re: [PATCH] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

Jan Engelhardt <jengelh@computergmbh.de> writes:

>>+	/* LCP only */
>>+	if (pid == PID_LCP)
>>+		switch (cp->code) {
>>+		case LCP_PROTO_REJ:
>>+			pid = ntohs(*(__be16*)skb->data);
>
> What if skb->data happens to be unaligned (can that even happen?)

It can't if the hdlc header is 32-bit aligned... however, I can now
see the above is buggy, skb->data doesn't point to the protocol
field. Unused code paths :-(

> -static const char *code_names[CP_CODES] = {"0", "ConfReq", "ConfAck",
> +static const char *const code_names[] = {"0", "ConfReq", "ConfAck",
>  					   "ConfNak", "ConfRej", "TermReq",
>  					   "TermAck", "CodeRej", "ProtoRej",
>  					   "EchoReq", "EchoReply", "Discard"};

The explicit sizes are there as a prevention.
Thanks.
-- 
Krzysztof Halasa

Re: [PATCH] Re: WAN: new PPP code for generic HDLC

[Jan Engelhardt]

On Mar 12 2008 21:10, Krzysztof Halasa wrote:
>
>> -static const char *code_names[CP_CODES] = {"0", "ConfReq", "ConfAck",
>> +static const char *const code_names[] = {"0", "ConfReq", "ConfAck",
>>  					   "ConfNak", "ConfRej", "TermReq",
>>  					   "TermAck", "CodeRej", "ProtoRej",
>>  					   "EchoReq", "EchoReply", "Discard"};
>
>The explicit sizes are there as a prevention.

You can keep them if you need, the main issue was const anyway.

Re: [PATCH] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

Krzysztof Halasa <khc@pm.waw.pl> writes:

> It can't if the hdlc header is 32-bit aligned... however, I can now
> see the above is buggy, skb->data doesn't point to the protocol
> field. Unused code paths :-(

Actually I was wrong, the code is correct and skb->data points to the
proto ID.

PATCH v2 is on the way.
-- 
Krzysztof Halasa

[PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

New synchronous PPP implementation for generic HDLC.

Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl>

 drivers/net/wan/Makefile   |    2 +-
 drivers/net/wan/hdlc_ppp.c |  649 ++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 602 insertions(+), 49 deletions(-)

diff --git a/drivers/net/wan/Makefile b/drivers/net/wan/Makefile
index d61fef3..3081683 100644
--- a/drivers/net/wan/Makefile
+++ b/drivers/net/wan/Makefile
@@ -14,7 +14,7 @@ obj-$(CONFIG_HDLC_RAW)		+= hdlc_raw.o
 obj-$(CONFIG_HDLC_RAW_ETH)	+= hdlc_raw_eth.o
 obj-$(CONFIG_HDLC_CISCO)	+= hdlc_cisco.o
 obj-$(CONFIG_HDLC_FR)		+= hdlc_fr.o
-obj-$(CONFIG_HDLC_PPP)		+= hdlc_ppp.o	syncppp.o
+obj-$(CONFIG_HDLC_PPP)		+= hdlc_ppp.o
 obj-$(CONFIG_HDLC_X25)		+= hdlc_x25.o
 
 pc300-y				:= pc300_drv.o
diff --git a/drivers/net/wan/hdlc_ppp.c b/drivers/net/wan/hdlc_ppp.c
index 10396d9..9fc20fb 100644
--- a/drivers/net/wan/hdlc_ppp.c
+++ b/drivers/net/wan/hdlc_ppp.c
@@ -2,7 +2,7 @@
  * Generic HDLC support routines for Linux
  * Point-to-point protocol support
  *
- * Copyright (C) 1999 - 2006 Krzysztof Halasa <khc@pm.waw.pl>
+ * Copyright (C) 1999 - 2008 Krzysztof Halasa <khc@pm.waw.pl>
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of version 2 of the GNU General Public License
@@ -19,87 +19,631 @@
 #include <linux/skbuff.h>
 #include <linux/pkt_sched.h>
 #include <linux/inetdevice.h>
-#include <linux/lapb.h>
-#include <linux/rtnetlink.h>
 #include <linux/hdlc.h>
-#include <net/syncppp.h>
+#include <linux/spinlock.h>
+
+#define DEBUG_CP		0 /* also bytes# to dump */
+#define DEBUG_STATE		0
+#define DEBUG_HARD_HEADER	0
+
+#define HDLC_ADDR_ALLSTATIONS	0xFF
+#define HDLC_CTRL_UI		0x03
+
+#define PID_LCP			0xC021
+#define PID_IP			0x0021
+#define PID_IPCP		0x8021
+#define PID_IPV6		0x0057
+#define PID_IPV6CP		0x8057
+
+enum {IDX_LCP = 0, IDX_IPCP, IDX_IPV6CP, IDX_COUNT};
+enum {CP_CONF_REQ = 1, CP_CONF_ACK, CP_CONF_NAK, CP_CONF_REJ, CP_TERM_REQ,
+      CP_TERM_ACK, CP_CODE_REJ, LCP_PROTO_REJ, LCP_ECHO_REQ, LCP_ECHO_REPLY,
+      LCP_DISC_REQ, CP_CODES};
+#if DEBUG_CP
+static const char *const code_names[CP_CODES] = {
+	"0", "ConfReq", "ConfAck", "ConfNak", "ConfRej", "TermReq",
+	"TermAck", "CodeRej", "ProtoRej", "EchoReq", "EchoReply", "Discard"
+};
+static char debug_buffer[64 + 3 * DEBUG_CP];
+#endif
+
+enum {LCP_OPTION_MRU = 1, LCP_OPTION_ACCM, LCP_OPTION_MAGIC = 5};
+
+struct hdlc_header {
+	u8 address;
+	u8 control;
+	__be16 protocol;
+};
+
+struct cp_header {
+	u8 code;
+	u8 id;
+	__be16 len;
+};
+
 
-struct ppp_state {
-	struct ppp_device pppdev;
-	struct ppp_device *syncppp_ptr;
-	int (*old_change_mtu)(struct net_device *dev, int new_mtu);
+struct proto {
+	struct net_device *dev;
+	struct timer_list timer;
+	unsigned long timeout;
+	u16 pid;		/* protocol ID */
+	u8 state;
+	u8 cr_id;		/* ID of last Configuration-Request */
+	u8 restart_counter;
 };
 
+struct ppp {
+	struct proto protos[IDX_COUNT];
+	spinlock_t lock;
+	unsigned long last_pong;
+	unsigned int req_timeout, cr_retries, term_retries;
+	unsigned int keepalive_interval, keepalive_timeout;
+	u8 seq;			/* local sequence number for requests */
+	u8 echo_id;		/* ID of last Echo-Request (LCP) */
+};
+
+enum {CLOSED = 0, STOPPED, STOPPING, REQ_SENT, ACK_RECV, ACK_SENT, OPENED,
+      STATES, STATE_MASK = 0xF};
+enum {START = 0, STOP, TO_GOOD, TO_BAD, RCR_GOOD, RCR_BAD, RCA, RCN, RTR, RTA,
+      RUC, RXJ_GOOD, RXJ_BAD, EVENTS};
+enum {INV = 0x10, IRC = 0x20, ZRC = 0x40, SCR = 0x80, SCA = 0x100,
+      SCN = 0x200, STR = 0x400, STA = 0x800, SCJ = 0x1000};
+
+#if DEBUG_STATE
+static const char *const state_names[STATES] = {
+	"Closed", "Stopped", "Stopping", "ReqSent", "AckRecv", "AckSent",
+	"Opened"
+};
+static const char *const event_names[EVENTS] = {
+	"Start", "Stop", "TO+", "TO-", "RCR+", "RCR-", "RCA", "RCN",
+	"RTR", "RTA", "RUC", "RXJ+", "RXJ-"
+};
+#endif
+
+static struct sk_buff_head tx_queue; /* used when holding the spin lock */
+
 static int ppp_ioctl(struct net_device *dev, struct ifreq *ifr);
 
+static inline struct ppp* get_ppp(struct net_device *dev)
+{
+	return (struct ppp *)dev_to_hdlc(dev)->state;
+}
 
-static inline struct ppp_state* state(hdlc_device *hdlc)
+static inline struct proto* get_proto(struct net_device *dev, u16 pid)
 {
-	return(struct ppp_state *)(hdlc->state);
+	struct ppp *ppp = get_ppp(dev);
+
+	switch (pid) {
+	case PID_LCP:
+		return &ppp->protos[IDX_LCP];
+	case PID_IPCP:
+		return &ppp->protos[IDX_IPCP];
+	case PID_IPV6CP:
+		return &ppp->protos[IDX_IPV6CP];
+	default:
+		return NULL;
+	}
 }
 
+static inline const char* proto_name(u16 pid)
+{
+	switch (pid) {
+	case PID_LCP:
+		return "LCP";
+	case PID_IPCP:
+		return "IPCP";
+	case PID_IPV6CP:
+		return "IPV6CP";
+	default:
+		return NULL;
+	}
+}
 
-static int ppp_open(struct net_device *dev)
+static __be16 ppp_type_trans(struct sk_buff *skb, struct net_device *dev)
 {
-	hdlc_device *hdlc = dev_to_hdlc(dev);
-	int (*old_ioctl)(struct net_device *, struct ifreq *, int);
-	int result;
+	struct hdlc_header *data = (struct hdlc_header*)skb->data;
+
+	if (skb->len < sizeof(struct hdlc_header))
+		return htons(ETH_P_HDLC);
+	if (data->address != HDLC_ADDR_ALLSTATIONS ||
+	    data->control != HDLC_CTRL_UI)
+		return htons(ETH_P_HDLC);
+
+	switch (data->protocol) {
+	case __constant_htons(PID_IP):
+		skb_pull(skb, sizeof(struct hdlc_header));
+		return htons(ETH_P_IP);
 
-	dev->priv = &state(hdlc)->syncppp_ptr;
-	state(hdlc)->syncppp_ptr = &state(hdlc)->pppdev;
-	state(hdlc)->pppdev.dev = dev;
+	case __constant_htons(PID_IPV6):
+		skb_pull(skb, sizeof(struct hdlc_header));
+		return htons(ETH_P_IPV6);
 
-	old_ioctl = dev->do_ioctl;
-	state(hdlc)->old_change_mtu = dev->change_mtu;
-	sppp_attach(&state(hdlc)->pppdev);
-	/* sppp_attach nukes them. We don't need syncppp's ioctl */
-	dev->do_ioctl = old_ioctl;
-	state(hdlc)->pppdev.sppp.pp_flags &= ~PP_CISCO;
-	dev->type = ARPHRD_PPP;
-	result = sppp_open(dev);
-	if (result) {
-		sppp_detach(dev);
-		return result;
+	default:
+		return htons(ETH_P_HDLC);
 	}
+}
 
-	return 0;
+
+static int ppp_hard_header(struct sk_buff *skb, struct net_device *dev,
+			   u16 type, const void *daddr, const void *saddr,
+			   unsigned int len)
+{
+	struct hdlc_header *data;
+#if DEBUG_HARD_HEADER
+	printk(KERN_DEBUG "%s: ppp_hard_header() called\n", dev->name);
+#endif
+
+	skb_push(skb, sizeof(struct hdlc_header));
+	data = (struct hdlc_header*)skb->data;
+
+	data->address = HDLC_ADDR_ALLSTATIONS;
+	data->control = HDLC_CTRL_UI;
+	switch (type) {
+	case ETH_P_IP:
+		data->protocol = htons(PID_IP);
+		break;
+	case ETH_P_IPV6:
+		data->protocol = htons(PID_IPV6);
+		break;
+	case PID_LCP:
+	case PID_IPCP:
+	case PID_IPV6CP:
+		data->protocol = htons(type);
+		break;
+	default:		/* unknown protocol */
+		data->protocol = 0;
+	}
+	return sizeof(struct hdlc_header);
 }
 
 
+static void ppp_tx_flush(void)
+{
+	struct sk_buff *skb;
+	while ((skb = skb_dequeue(&tx_queue)) != NULL)
+		dev_queue_xmit(skb);
+}
 
-static void ppp_close(struct net_device *dev)
+static void ppp_tx_cp(struct net_device *dev, u16 pid, u8 code,
+		      u8 id, unsigned int len, const void *data)
 {
-	hdlc_device *hdlc = dev_to_hdlc(dev);
+	struct sk_buff *skb;
+	struct cp_header *cp;
+	unsigned int magic_len = 0;
+	static u32 magic;
+
+#if DEBUG_CP
+	int i;
+	char *ptr;
+#endif
+
+	if (pid == PID_LCP && (code == LCP_ECHO_REQ || code == LCP_ECHO_REPLY))
+		magic_len = sizeof(magic);
+
+	skb = dev_alloc_skb(sizeof(struct hdlc_header) +
+			    sizeof(struct cp_header) + magic_len + len);
+	if (!skb) {
+		printk(KERN_WARNING "%s: out of memory in ppp_tx_cp()\n",
+		       dev->name);
+		return;
+	}
+	skb_reserve(skb, sizeof(struct hdlc_header));
+
+	cp = (struct cp_header *)skb_put(skb, sizeof(struct cp_header));
+	cp->code = code;
+	cp->id = id;
+	cp->len = htons(sizeof(struct cp_header) + magic_len + len);
+
+	if (magic_len)
+		memcpy(skb_put(skb, magic_len), &magic, magic_len);
+	if (len)
+		memcpy(skb_put(skb, len), data, len);
+
+#if DEBUG_CP
+	BUG_ON(code >= CP_CODES);
+	ptr = debug_buffer;
+	*ptr = '\x0';
+	for (i = 0; i < min_t(unsigned int, magic_len + len, DEBUG_CP); i++) {
+		sprintf(ptr, " %02X", skb->data[sizeof(struct cp_header) + i]);
+		ptr += strlen(ptr);
+	}
+	printk(KERN_DEBUG "%s: TX %s [%s id 0x%X]%s\n", dev->name,
+	       proto_name(pid), code_names[code], id, debug_buffer);
+#endif
 
-	sppp_close(dev);
-	sppp_detach(dev);
+	ppp_hard_header(skb, dev, pid, NULL, NULL, 0);
 
-	dev->change_mtu = state(hdlc)->old_change_mtu;
-	dev->mtu = HDLC_MAX_MTU;
-	dev->hard_header_len = 16;
+	skb->priority = TC_PRIO_CONTROL;
+	skb->dev = dev;
+	skb_reset_network_header(skb);
+	skb_queue_tail(&tx_queue, skb);
 }
 
 
+/* State transition table (compare STD-51)
+   Events                                   Actions
+   TO+  = Timeout with counter > 0          irc = Initialize-Restart-Count
+   TO-  = Timeout with counter expired      zrc = Zero-Restart-Count
+
+   RCR+ = Receive-Configure-Request (Good)  scr = Send-Configure-Request
+   RCR- = Receive-Configure-Request (Bad)
+   RCA  = Receive-Configure-Ack             sca = Send-Configure-Ack
+   RCN  = Receive-Configure-Nak/Rej         scn = Send-Configure-Nak/Rej
+
+   RTR  = Receive-Terminate-Request         str = Send-Terminate-Request
+   RTA  = Receive-Terminate-Ack             sta = Send-Terminate-Ack
+
+   RUC  = Receive-Unknown-Code              scj = Send-Code-Reject
+   RXJ+ = Receive-Code-Reject (permitted)
+       or Receive-Protocol-Reject
+   RXJ- = Receive-Code-Reject (catastrophic)
+       or Receive-Protocol-Reject
+*/
+static int cp_table[EVENTS][STATES] = {
+	/* CLOSED     STOPPED STOPPING REQ_SENT ACK_RECV ACK_SENT OPENED
+	     0           1         2       3       4      5          6    */
+	{IRC|SCR|3,     INV     , INV ,   INV   , INV ,  INV    ,   INV   }, /* START */
+	{   INV   ,      0      ,  0  ,    0    ,  0  ,   0     ,    0    }, /* STOP */
+	{   INV   ,     INV     ,STR|2,  SCR|3  ,SCR|3,  SCR|5  ,   INV   }, /* TO+ */
+	{   INV   ,     INV     ,  1  ,    1    ,  1  ,    1    ,   INV   }, /* TO- */
+	{  STA|0  ,IRC|SCR|SCA|5,  2  ,  SCA|5  ,SCA|6,  SCA|5  ,SCR|SCA|5}, /* RCR+ */
+	{  STA|0  ,IRC|SCR|SCN|3,  2  ,  SCN|3  ,SCN|4,  SCN|3  ,SCR|SCN|3}, /* RCR- */
+	{  STA|0  ,    STA|1    ,  2  ,  IRC|4  ,SCR|3,    6    , SCR|3   }, /* RCA */
+	{  STA|0  ,    STA|1    ,  2  ,IRC|SCR|3,SCR|3,IRC|SCR|5, SCR|3   }, /* RCN */
+	{  STA|0  ,    STA|1    ,STA|2,  STA|3  ,STA|3,  STA|3  ,ZRC|STA|2}, /* RTR */
+	{    0    ,      1      ,  1  ,    3    ,  3  ,    5    ,  SCR|3  }, /* RTA */
+	{  SCJ|0  ,    SCJ|1    ,SCJ|2,  SCJ|3  ,SCJ|4,  SCJ|5  ,  SCJ|6  }, /* RUC */
+	{    0    ,      1      ,  2  ,    3    ,  3  ,    5    ,    6    }, /* RXJ+ */
+	{    0    ,      1      ,  1  ,    1    ,  1  ,    1    ,IRC|STR|2}, /* RXJ- */
+};
+
 
-static __be16 ppp_type_trans(struct sk_buff *skb, struct net_device *dev)
+/* SCA: RCR+ must supply id, len and data
+   SCN: RCR- must supply code, id, len and data
+   STA: RTR must supply id
+   SCJ: RUC must supply CP packet len and data */
+static void ppp_cp_event(struct net_device *dev, u16 pid, u16 event, u8 code,
+			 u8 id, unsigned int len, void *data)
 {
-	return __constant_htons(ETH_P_WAN_PPP);
+	int old_state, action;
+	struct ppp *ppp = get_ppp(dev);
+	struct proto *proto = get_proto(dev, pid);
+
+	old_state = proto->state;
+	BUG_ON(old_state >= STATES);
+	BUG_ON(event >= EVENTS);
+
+#if DEBUG_STATE
+	printk(KERN_DEBUG "%s: %s ppp_cp_event(%s) %s ...\n", dev->name,
+	       proto_name(pid), event_names[event], state_names[proto->state]);
+#endif
+
+	action = cp_table[event][old_state];
+
+	proto->state = action & STATE_MASK;
+	if (action & (SCR | STR)) /* set Configure-Req/Terminate-Req timer */
+		mod_timer(&proto->timer, proto->timeout =
+			  jiffies + ppp->req_timeout * HZ);
+	if (action & ZRC)
+		proto->restart_counter = 0;
+	if (action & IRC)
+		proto->restart_counter = (proto->state == STOPPING) ?
+			ppp->term_retries : ppp->cr_retries;
+
+	if (action & SCR)	/* send Configure-Request */
+		ppp_tx_cp(dev, pid, CP_CONF_REQ, proto->cr_id = ++ppp->seq,
+			  0, NULL);
+	if (action & SCA)	/* send Configure-Ack */
+		ppp_tx_cp(dev, pid, CP_CONF_ACK, id, len, data);
+	if (action & SCN)	/* send Configure-Nak/Reject */
+		ppp_tx_cp(dev, pid, code, id, len, data);
+	if (action & STR)	/* send Terminate-Request */
+		ppp_tx_cp(dev, pid, CP_TERM_REQ, ++ppp->seq, 0, NULL);
+	if (action & STA)	/* send Terminate-Ack */
+		ppp_tx_cp(dev, pid, CP_TERM_ACK, id, 0, NULL);
+	if (action & SCJ)	/* send Code-Reject */
+		ppp_tx_cp(dev, pid, CP_CODE_REJ, ++ppp->seq, len, data);
+
+	if (old_state != OPENED && proto->state == OPENED) {
+		printk(KERN_INFO "%s: %s up\n", dev->name, proto_name(pid));
+		if (pid == PID_LCP) {
+			netif_dormant_off(dev);
+			ppp_cp_event(dev, PID_IPCP, START, 0, 0, 0, NULL);
+			ppp_cp_event(dev, PID_IPV6CP, START, 0, 0, 0, NULL);
+			ppp->last_pong = jiffies;
+			mod_timer(&proto->timer, proto->timeout =
+				  jiffies + ppp->keepalive_interval * HZ);
+		}
+	}
+	if (old_state == OPENED && proto->state != OPENED) {
+		printk(KERN_INFO "%s: %s down\n", dev->name, proto_name(pid));
+		if (pid == PID_LCP) {
+			netif_dormant_on(dev);
+			ppp_cp_event(dev, PID_IPCP, STOP, 0, 0, 0, NULL);
+			ppp_cp_event(dev, PID_IPV6CP, STOP, 0, 0, 0, NULL);
+		}
+	}
+	if (old_state != CLOSED && proto->state == CLOSED)
+		del_timer(&proto->timer);
+
+#if DEBUG_STATE
+	printk(KERN_DEBUG "%s: %s ppp_cp_event(%s) ... %s\n", dev->name,
+	       proto_name(pid), event_names[event], state_names[proto->state]);
+#endif
 }
 
 
+static void ppp_cp_parse_cr(struct net_device *dev, u16 pid, u8 id,
+			    unsigned int len, u8 *data)
+{
+	static u8 const valid_accm[6] = { LCP_OPTION_ACCM, 6, 0, 0, 0, 0 };
+	u8 *opt, *out;
+	unsigned int nak_len = 0, rej_len = 0;
+
+	if (!(out = kmalloc(len, GFP_ATOMIC))) {
+		dev_to_hdlc(dev)->stats.rx_dropped++;
+		return;	/* out of memory, ignore CR packet */
+	}
+
+	for (opt = data; len; len -= opt[1], opt += opt[1]) {
+		if (len < 2 || len < opt[1]) {
+			dev_to_hdlc(dev)->stats.rx_errors++;
+			return; /* bad packet, drop silently */
+		}
+
+		if (pid == PID_LCP)
+			switch (opt[0]) {
+			case LCP_OPTION_MRU:
+				continue; /* MRU always OK and > 1500 bytes? */
+
+			case LCP_OPTION_ACCM: /* async control character map */
+				if (!memcmp(opt, valid_accm,
+					    sizeof(valid_accm)))
+					continue;
+				if (!rej_len) { /* NAK it */
+					memcpy(out + nak_len, valid_accm,
+					       sizeof(valid_accm));
+					nak_len += sizeof(valid_accm);
+					continue;
+				}
+				break;
+			case LCP_OPTION_MAGIC:
+				if (opt[1] != 6 || (!opt[2] && !opt[3] &&
+						    !opt[4] && !opt[5]))
+					break; /* reject invalid magic number */
+				continue;
+			}
+		/* reject this option */
+		memcpy(out + rej_len, opt, opt[1]);
+		rej_len += opt[1];
+	}
+
+	if (rej_len)
+		ppp_cp_event(dev, pid, RCR_BAD, CP_CONF_REJ, id, rej_len, out);
+	else if (nak_len)
+		ppp_cp_event(dev, pid, RCR_BAD, CP_CONF_NAK, id, nak_len, out);
+	else
+		ppp_cp_event(dev, pid, RCR_GOOD, CP_CONF_ACK, id, len, data);
+
+	kfree(out);
+}
+
+static int ppp_rx(struct sk_buff *skb)
+{
+	struct hdlc_header *hdr = (struct hdlc_header*)skb->data;
+	struct net_device *dev = skb->dev;
+	struct ppp *ppp = get_ppp(dev);
+	struct proto *proto;
+	struct cp_header *cp;
+	unsigned long flags;
+	unsigned int len;
+	u16 pid;
+#if DEBUG_CP
+	int i;
+	char *ptr;
+#endif
+
+	spin_lock_irqsave(&ppp->lock, flags);
+	/* Check HDLC header */
+	if (skb->len < sizeof(struct hdlc_header))
+		goto rx_error;
+	cp = (struct cp_header*)skb_pull(skb, sizeof(struct hdlc_header));
+	if (hdr->address != HDLC_ADDR_ALLSTATIONS ||
+	    hdr->control != HDLC_CTRL_UI)
+		goto rx_error;
+
+	pid = ntohs(hdr->protocol);
+	proto = get_proto(dev, pid);
+	if (!proto) {
+		if (ppp->protos[IDX_LCP].state == OPENED)
+			ppp_tx_cp(dev, PID_LCP, LCP_PROTO_REJ,
+				  ++ppp->seq, skb->len + 2, &hdr->protocol);
+		goto rx_error;
+	}
+
+	len = ntohs(cp->len);
+	if (len < sizeof(struct cp_header) /* no complete CP header? */ ||
+	    skb->len < len /* truncated packet? */)
+		goto rx_error;
+	skb_pull(skb, sizeof(struct cp_header));
+	len -= sizeof(struct cp_header);
+
+	/* HDLC and CP headers stripped from skb */
+#if DEBUG_CP
+	if (cp->code < CP_CODES)
+		sprintf(debug_buffer, "[%s id 0x%X]", code_names[cp->code],
+			cp->id);
+	else
+		sprintf(debug_buffer, "[code %u id 0x%X]", cp->code, cp->id);
+	ptr = debug_buffer + strlen(debug_buffer);
+	for (i = 0; i < min_t(unsigned int, len, DEBUG_CP); i++) {
+		sprintf(ptr, " %02X", skb->data[i]);
+		ptr += strlen(ptr);
+	}
+	printk(KERN_DEBUG "%s: RX %s %s\n", dev->name, proto_name(pid),
+	       debug_buffer);
+#endif
+
+	/* LCP only */
+	if (pid == PID_LCP)
+		switch (cp->code) {
+		case LCP_PROTO_REJ:
+			pid = ntohs(*(__be16*)skb->data);
+			if (pid == PID_LCP || pid == PID_IPCP ||
+			    pid == PID_IPV6CP)
+				ppp_cp_event(dev, pid, RXJ_BAD, 0, 0,
+					     0, NULL);
+			goto out;
+
+		case LCP_ECHO_REQ: /* send Echo-Reply */
+			if (len >= 4 && proto->state == OPENED)
+				ppp_tx_cp(dev, PID_LCP, LCP_ECHO_REPLY,
+					  cp->id, len - 4, skb->data + 4);
+			goto out;
+
+		case LCP_ECHO_REPLY:
+			if (cp->id == ppp->echo_id)
+				ppp->last_pong = jiffies;
+			goto out;
+
+		case LCP_DISC_REQ: /* discard */
+			goto out;
+		}
+
+	/* LCP, IPCP and IPV6CP */
+	switch (cp->code) {
+	case CP_CONF_REQ:
+		ppp_cp_parse_cr(dev, pid, cp->id, len, skb->data);
+		goto out;
+
+	case CP_CONF_ACK:
+		if (cp->id == proto->cr_id)
+			ppp_cp_event(dev, pid, RCA, 0, 0, 0, NULL);
+		goto out;
+
+	case CP_CONF_REJ:
+	case CP_CONF_NAK:
+		if (cp->id == proto->cr_id)
+			ppp_cp_event(dev, pid, RCN, 0, 0, 0, NULL);
+		goto out;
+
+	case CP_TERM_REQ:
+		ppp_cp_event(dev, pid, RTR, 0, cp->id, 0, NULL);
+		goto out;
+
+	case CP_TERM_ACK:
+		ppp_cp_event(dev, pid, RTA, 0, 0, 0, NULL);
+		goto out;
+
+	case CP_CODE_REJ:
+		ppp_cp_event(dev, pid, RXJ_BAD, 0, 0, 0, NULL);
+		goto out;
+
+	default:
+		len += sizeof(struct cp_header);
+		if (len > dev->mtu)
+			len = dev->mtu;
+		ppp_cp_event(dev, pid, RUC, 0, 0, len, cp);
+		goto out;
+	}
+	goto out;
+
+rx_error:
+	dev_to_hdlc(dev)->stats.rx_errors++;
+out:
+	spin_unlock_irqrestore(&ppp->lock, flags);
+	dev_kfree_skb_any(skb);
+	ppp_tx_flush();
+	return NET_RX_DROP;
+}
+
+
+static void ppp_timer(unsigned long arg)
+{
+	struct proto *proto = (struct proto *)arg;
+	struct ppp *ppp = get_ppp(proto->dev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&ppp->lock, flags);
+	switch (proto->state) {
+	case STOPPING:
+	case REQ_SENT:
+	case ACK_RECV:
+	case ACK_SENT:
+		if (proto->restart_counter) {
+			ppp_cp_event(proto->dev, proto->pid, TO_GOOD, 0, 0,
+				     0, NULL);
+			proto->restart_counter--;
+		} else
+			ppp_cp_event(proto->dev, proto->pid, TO_BAD, 0, 0,
+				     0, NULL);
+		break;
+
+	case OPENED:
+		if (proto->pid != PID_LCP)
+			break;
+		if (time_after(jiffies, ppp->last_pong +
+			       ppp->keepalive_timeout * HZ)) {
+			printk(KERN_INFO "%s: Link down\n", proto->dev->name);
+			ppp_cp_event(proto->dev, PID_LCP, STOP, 0, 0, 0, NULL);
+			ppp_cp_event(proto->dev, PID_LCP, START, 0, 0, 0, NULL);
+		} else {	/* send keep-alive packet */
+			ppp->echo_id = ++ppp->seq;
+			ppp_tx_cp(proto->dev, PID_LCP, LCP_ECHO_REQ,
+				  ppp->echo_id, 0, NULL);
+			proto->timer.expires = jiffies +
+				ppp->keepalive_interval * HZ;
+			add_timer(&proto->timer);
+		}
+		break;
+	}
+	spin_unlock_irqrestore(&ppp->lock, flags);
+	ppp_tx_flush();
+}
+
+
+static void ppp_start(struct net_device *dev)
+{
+	struct ppp *ppp = get_ppp(dev);
+	int i;
+
+	for (i = 0; i < IDX_COUNT; i++) {
+		struct proto *proto = &ppp->protos[i];
+		proto->dev = dev;
+		init_timer(&proto->timer);
+		proto->timer.function = ppp_timer;
+		proto->timer.data = (unsigned long)proto;
+		proto->state = CLOSED;
+	}
+	ppp->protos[IDX_LCP].pid = PID_LCP;
+	ppp->protos[IDX_IPCP].pid = PID_IPCP;
+	ppp->protos[IDX_IPV6CP].pid = PID_IPV6CP;
+
+	ppp_cp_event(dev, PID_LCP, START, 0, 0, 0, NULL);
+}
+
+static void ppp_stop(struct net_device *dev)
+{
+	ppp_cp_event(dev, PID_LCP, STOP, 0, 0, 0, NULL);
+}
 
 static struct hdlc_proto proto = {
-	.open		= ppp_open,
-	.close		= ppp_close,
+	.start		= ppp_start,
+	.stop		= ppp_stop,
 	.type_trans	= ppp_type_trans,
 	.ioctl		= ppp_ioctl,
+	.netif_rx	= ppp_rx,
 	.module		= THIS_MODULE,
 };
 
+static const struct header_ops ppp_header_ops = {
+	.create = ppp_hard_header,
+};
 
 static int ppp_ioctl(struct net_device *dev, struct ifreq *ifr)
 {
 	hdlc_device *hdlc = dev_to_hdlc(dev);
+	struct ppp *ppp;
 	int result;
 
 	switch (ifr->ifr_settings.type) {
@@ -110,25 +654,35 @@ static int ppp_ioctl(struct net_device *dev, struct ifreq *ifr)
 		return 0; /* return protocol only, no settable parameters */
 
 	case IF_PROTO_PPP:
-		if(!capable(CAP_NET_ADMIN))
+		if (!capable(CAP_NET_ADMIN))
 			return -EPERM;
 
-		if(dev->flags & IFF_UP)
+		if (dev->flags & IFF_UP)
 			return -EBUSY;
 
 		/* no settable parameters */
 
-		result=hdlc->attach(dev, ENCODING_NRZ,PARITY_CRC16_PR1_CCITT);
+		result = hdlc->attach(dev, ENCODING_NRZ,PARITY_CRC16_PR1_CCITT);
 		if (result)
 			return result;
 
-		result = attach_hdlc_protocol(dev, &proto,
-					      sizeof(struct ppp_state));
+		result = attach_hdlc_protocol(dev, &proto, sizeof(struct ppp));
 		if (result)
 			return result;
+
+		ppp = get_ppp(dev);
+		spin_lock_init(&ppp->lock);
+		ppp->req_timeout = 2;
+		ppp->cr_retries = 10;
+		ppp->term_retries = 2;
+		ppp->keepalive_interval = 10;
+		ppp->keepalive_timeout = 60;
+
 		dev->hard_start_xmit = hdlc->xmit;
+		dev->hard_header_len = sizeof(struct hdlc_header);
+		dev->header_ops = &ppp_header_ops;
 		dev->type = ARPHRD_PPP;
-		netif_dormant_off(dev);
+		netif_dormant_on(dev);
 		return 0;
 	}
 
@@ -138,12 +692,11 @@ static int ppp_ioctl(struct net_device *dev, struct ifreq *ifr)
 
 static int __init mod_init(void)
 {
+	skb_queue_head_init(&tx_queue);
 	register_hdlc_protocol(&proto);
 	return 0;
 }
 
-
-
 static void __exit mod_exit(void)
 {
 	unregister_hdlc_protocol(&proto);

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

Hi,

I wrote:

> New synchronous PPP implementation for generic HDLC.

So are we leaving the generic-HDLC+PPP in broken state for 2.6.25,
aren't we?

If we do, perhaps a "|| BROKEN" in drivers/net/wan/Kconfig would make
sense? Any attempt to use it will cause kernel panic immediately (PPP
with generic HDLC only; drivers using syncppp.c directly are not
affected). I can make that trivial Kconfig patch of course.
-- 
Krzysztof Halasa

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Jeff Garzik]

Krzysztof Halasa wrote:
> Hi,
> 
> I wrote:
> 
>> New synchronous PPP implementation for generic HDLC.
> 
> So are we leaving the generic-HDLC+PPP in broken state for 2.6.25,
> aren't we?
> 
> If we do, perhaps a "|| BROKEN" in drivers/net/wan/Kconfig would make
> sense? Any attempt to use it will cause kernel panic immediately (PPP
> with generic HDLC only; drivers using syncppp.c directly are not
> affected). I can make that trivial Kconfig patch of course.

In your original email you said

	I guess it should go into 2.6.25, not sure about "stable"
	series. I will appreciate any feedback, review and/or test
	results.

At the time of the posting 2.6.25-rc6 had already been released, which 
seems like an inappropriate time for all that new code, which has been 
given so little exposure to real world testing.

Certainly your original message said PPP panics, but without even 
minimal testing how do we know that your new code doesn't have equally 
problematic issues?

So quite honestly a CONFIG_BROKEN patch might indeed be more appropriate 
since generic HDLC works with Frame Relay at least...

Comments?  IMO the current code is a known risk (PPP panic, FR works) 
whereas the new code is an unknown risk very late in 2.6.25-rc -- but 
with a good chance to make HDLC+PPP work again.

	Jeff

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

Jeff Garzik <jeff@garzik.org> writes:

> 	I guess it should go into 2.6.25, not sure about "stable"
> 	series. I will appreciate any feedback, review and/or test
> 	results.

Right. Perhaps I should care a bit more about the stable series...

> At the time of the posting 2.6.25-rc6 had already been released, which
> seems like an inappropriate time for all that new code, which has been
> given so little exposure to real world testing.

Sure.

> Certainly your original message said PPP panics, but without even
> minimal testing how do we know that your new code doesn't have equally
> problematic issues?

Well, there was something like "minimal testing", and it doesn't panic
100% like the old code does. But the probability that it won't work
correctly is quite high.

IOW: the new version is certainly better than the old one, though it's
not the normal quality (in terms of testing) I'd like to see.

> So quite honestly a CONFIG_BROKEN patch might indeed be more
> appropriate since generic HDLC works with Frame Relay at least...

Actually Frame Relay and other protocols are not affected, the PPP
patch doesn't change them a bit, it's a different module. The new code
only affect PPP protocol.

I'm fine with the Kconfig patch, actually I'm not sure what is better
at this time - a known broken module marked as such or a new module
with some small chances that it will crash the machine and with much
bigger chances that it won't work with a certain PPP implementation on
the other end.

Something like that?
Untested :-)

Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl>

--- a/drivers/net/wan/Kconfig
+++ b/drivers/net/wan/Kconfig
@@ -150,9 +150,13 @@ config HDLC_FR
 
 config HDLC_PPP
 	tristate "Synchronous Point-to-Point Protocol (PPP) support"
-	depends on HDLC
+	depends on HDLC && BROKEN
 	help
 	  Generic HDLC driver supporting PPP over WAN connections.
+	  This module is currently broken and will cause a kernel panic
+	  when a device configured in PPP mode is activated.
+
+	  It will be replaced by new PPP implementation in Linux 2.6.26.
 
 	  If unsure, say N.
 

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

Jeff Garzik <jeff@garzik.org> writes:

> So quite honestly a CONFIG_BROKEN patch might indeed be more
> appropriate since generic HDLC works with Frame Relay at least...

Actually my question was intended for Andrew as he has already
queued the PPP patch - I probably should have rearranged To/Cc:
headers, sorry about that.

The question still stands: I guess it's fair enough to have either
patch in 2.6.25 (new PPP or "BROKEN" in Kconfig) but I think we
shouldn't leave it as is.
-- 
Krzysztof Halasa

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Tue, 25 Mar 2008 15:39:49 +0100

> > New synchronous PPP implementation for generic HDLC.
> 
> So are we leaving the generic-HDLC+PPP in broken state for 2.6.25,
> aren't we?

Kyzysztof, this is the way you behave every time your
patches don't get looked at as quickly as you would
like them to.

And this behavior does not trigger us maintainers to
stop everything we're doing and apply your patches.

In fact it makes us want to review your patches even
less.

Sending a healthy ping asking if your patches need
to be resent, etc., is one thing.  But this isn't
what you're doing here.

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

David Miller <davem@davemloft.net> writes:

> Kyzysztof, this is the way you behave every time your
> patches don't get looked at as quickly as you would
> like them to.
>
> And this behavior does not trigger us maintainers to
> stop everything we're doing and apply your patches.
>
> In fact it makes us want to review your patches even
> less.
>
> Sending a healthy ping asking if your patches need
> to be resent, etc., is one thing.  But this isn't
> what you're doing here.

I don't know what are you talking about.

- someone broke my drivers, happens from time to time
- I asked about related change and was told about some "out of tree"
  drivers instead
- I had two modules to fix, one took 15 minutes and for the other one
  I had no time at the moment.
- I finally rewrote the other module
- got feedback, corrected it, resent.
- asked if we are including it in 2.6.25 or if we are to make
  temporary Kconfig change instead

What exactly don't you like in my "behaviour"? I didn't invent
synchronous serial devices. Actually, I'm doing it all in my free
time, and I'm sure I can find better things to do in that time.

I accept I may use not the best wording from time to time, I'm not
native English writter.

What is your problem about me?

I'd appreciate it if you point out the precise things I'm doing
wrong.

Thanks.
-- 
Krzysztof Halasa

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

Hi,

I wrote:

> So are we leaving the generic-HDLC+PPP in broken state for 2.6.25,
> aren't we?
>
> If we do, perhaps a "|| BROKEN" in drivers/net/wan/Kconfig would make
> sense?

Linus just made 2.6.25-rclast (I hope), I understand 2.6.25 will have
broken drivers/net/wan/hdlc_ppp, any chance to apply the temporary
Kconfig ("depends on BROKEN") patch?

Would you like me to resend it?

TIA.
-- 
Krzysztof Halasa

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Andrew Morton]

On Fri, 11 Apr 2008 23:35:23 +0200 Krzysztof Halasa <khc@pm.waw.pl> wrote:

> Hi,
> 
> I wrote:
> 
> > So are we leaving the generic-HDLC+PPP in broken state for 2.6.25,
> > aren't we?
> >
> > If we do, perhaps a "|| BROKEN" in drivers/net/wan/Kconfig would make
> > sense?
> 
> Linus just made 2.6.25-rclast (I hope), I understand 2.6.25 will have
> broken drivers/net/wan/hdlc_ppp, any chance to apply the temporary
> Kconfig ("depends on BROKEN") patch?
> 
> Would you like me to resend it?

It never hurts to resend.  It was 6000 patches ago and my mind is a blank.

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

Andrew Morton <akpm@linux-foundation.org> writes:

> It never hurts to resend.  It was 6000 patches ago and my mind is a blank.

Sure, here is it. Some description, well:

PPP support in generic HDLC in Linux 2.6.25 is broken and will cause
a kernel panic when a device configured in PPP mode is activated.

It will be replaced by new PPP implementation after Linux 2.6.25 is
released.

This affects only PPP support in generic HDLC (mostly Hitachi SCA
and SCA-II based drivers, wanxl, and few others). Standalone syncppp
and async PPP support are not affected.

Untested :-)

Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl>

--- a/drivers/net/wan/Kconfig
+++ b/drivers/net/wan/Kconfig
@@ -150,9 +150,13 @@ config HDLC_FR
 
 config HDLC_PPP
 	tristate "Synchronous Point-to-Point Protocol (PPP) support"
-	depends on HDLC
+	depends on HDLC && BROKEN
 	help
 	  Generic HDLC driver supporting PPP over WAN connections.
+	  This module is currently broken and will cause a kernel panic
+	  when a device configured in PPP mode is activated.
+
+	  It will be replaced by new PPP implementation in Linux 2.6.26.
 
 	  If unsure, say N.
 

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Jeff Garzik]

Krzysztof Halasa wrote:
> --- a/drivers/net/wan/Kconfig
> +++ b/drivers/net/wan/Kconfig
> @@ -150,9 +150,13 @@ config HDLC_FR
>  
>  config HDLC_PPP
>  	tristate "Synchronous Point-to-Point Protocol (PPP) support"
> -	depends on HDLC
> +	depends on HDLC && BROKEN
>  	help
>  	  Generic HDLC driver supporting PPP over WAN connections.
> +	  This module is currently broken and will cause a kernel panic
> +	  when a device configured in PPP mode is activated.
> +
> +	  It will be replaced by new PPP implementation in Linux 2.6.26.
>  
>  	  If unsure, say N.


applied

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

Jeff Garzik <jeff@garzik.org> writes:

> applied

[two of them]

Great, thanks.
-- 
Krzysztof Halasa

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Alan Cox]

On Sat, 12 Apr 2008 10:10:40 +0200
Krzysztof Halasa <khc@pm.waw.pl> wrote:

> Andrew Morton <akpm@linux-foundation.org> writes:
> 
> > It never hurts to resend.  It was 6000 patches ago and my mind is a blank.
> 
> Sure, here is it. Some description, well:
> 
> PPP support in generic HDLC in Linux 2.6.25 is broken and will cause
> a kernel panic when a device configured in PPP mode is activated.
> 
> It will be replaced by new PPP implementation after Linux 2.6.25 is
> released.

Thats a pretty bad regression. Surely the correct fix is to revert the
change series that caused the breakage then re-merge that and fixes for
HDLC PPP for 2.6.26, not just leave it broken ?

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

>> PPP support in generic HDLC in Linux 2.6.25 is broken and will cause
>> a kernel panic when a device configured in PPP mode is activated.
>> 
>> It will be replaced by new PPP implementation after Linux 2.6.25 is
>> released.
>
> Thats a pretty bad regression. Surely the correct fix is to revert the
> change series that caused the breakage then re-merge that and fixes for
> HDLC PPP for 2.6.26, not just leave it broken ?

It's not that simple - it was broken between 2.6.22 and 2.6.23 (2.6.23
was already broken), I just haven't noticed (I don't use PPP myself,
except for tests).

author	Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
	 Fri, 6 Jul 2007 20:36:20 +0000 (13:36 -0700)
commit	f25f4e44808f0f6c9875d94ef1c41ef86c288eb2

[CORE] Stack changes to add multiqueue hardware support API
Add the multiqueue hardware device support API to the core network
stack.  Allow drivers to allocate multiple queues and manage them at
the netdev level if they choose to do so.

Added a new field to sk_buff, namely queue_mapping, for drivers to
know which tx_ring to select based on OS classification of the flow.

And it included:
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -565,9 +578,7 @@ struct net_device
 
 static inline void *netdev_priv(const struct net_device *dev)
 {
-       return (char *)dev + ((sizeof(struct net_device)
-                                       + NETDEV_ALIGN_CONST)
-                               & ~NETDEV_ALIGN_CONST);
+       return dev->priv;
 }
 
 #define SET_MODULE_OWNER(dev) do { } while (0)


HDLC PPP sits between hardware drivers and (in this case) syncppp.c,
and it (hdlc_ppp module) was using netdev_priv() as a pointer to the
memory allocated with (after) the netdev structure. dev->priv was used
by syncppp.c.

HDLC FR was also broken by this change but the fix was easy because
there is no external protocol module to interface to.

I didn't want to add to the HDLC PPP mess anymore and have rewritten
it instead (in process, it turned out syncppp alone, and syncppp +
HDLC have much more problems than I thought). Obviously I was way too
late for 2.6.25 (past 2.6.25-rc5 and the new code wasn't much tested).


Yes, I could add another dirty "interface fix" to the HDLC + syncppp
combo (hdlc_ppp), but I don't really see a sense, especially that I
have the new implementation ready and working (my time resources are
currently quite limited and if I have to choose between adding another
glue fix and writing a better PPP replacement I pick the latter).


My original post: http://lkml.org/lkml/2008/3/12/277
-- 
Krzysztof Halasa

RE: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Waskiewicz Jr, Peter P]

> It's not that simple - it was broken between 2.6.22 and 2.6.23 (2.6.23
> was already broken), I just haven't noticed (I don't use PPP myself,
> except for tests).

> author	Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> 	 Fri, 6 Jul 2007 20:36:20 +0000 (13:36 -0700)
> commit	f25f4e44808f0f6c9875d94ef1c41ef86c288eb2

> [CORE] Stack changes to add multiqueue hardware support API
> Add the multiqueue hardware device support API to the core network
> stack.  Allow drivers to allocate multiple queues and manage them at
> the netdev level if they choose to do so.

> Added a new field to sk_buff, namely queue_mapping, for drivers to
> know which tx_ring to select based on OS classification of the flow.

> And it included:
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -565,9 +578,7 @@ struct net_device
 
> static inline void *netdev_priv(const struct net_device *dev)
> {
> -       return (char *)dev + ((sizeof(struct net_device)
> -                                       + NETDEV_ALIGN_CONST)
> -                               & ~NETDEV_ALIGN_CONST);
> +       return dev->priv;
> }

What I find a bit disturbing is I found my original patchsets I
submitted to the list that were committed for this patchset.  My
original patches don't have this change in them at all.  :-(

-PJ Waskiewicz

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[David Miller]

From: "Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@intel.com>
Date: Mon, 14 Apr 2008 12:16:55 -0700

> What I find a bit disturbing is I found my original patchsets I
> submitted to the list that were committed for this patchset.  My
> original patches don't have this change in them at all.  :-(

Ummmm, if you remember we had to put that in because it was the
only way to get your multiqueue changes to even work.

So don't even act as if that got slipped in behind your back,
unknowingly.  You were absolutely informed about this.

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

Update:

> Yes, I could add another dirty "interface fix" to the HDLC + syncppp
> combo (hdlc_ppp), but I don't really see a sense, especially that I
> have the new implementation ready and working (my time resources are
> currently quite limited and if I have to choose between adding another
> glue fix and writing a better PPP replacement I pick the latter).
>
>
> My original post: http://lkml.org/lkml/2008/3/12/277

David doesn't want the new PPP, insisting that I fix existing
hdlc_ppp.c + syncppp.c combo instead, and that there are related bugs
in drivers using syncppp.c alone (not hdlc_ppp) which I have to fix as
well.

Unfortunately I'm unable to do that. Even if I could add another
workaround to hdlc_ppp to make it work with syncppp.c again, I can't
fix the other drivers (if they are indeed broken) because of lack of
hardware, and I can't really make significant changes to syncppp.c
(to make them available to all drivers) because that could and would
break those drivers.

Comments?
-- 
Krzysztof Halasa

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Fri, 18 Apr 2008 17:58:48 +0200

> Comments?

I'll fix it properly because I know you won't do the work.
You don't need to keep making excuses.

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

David Miller <davem@davemloft.net> writes:

> I'll fix it properly because I know you won't do the work.

Great to read that.

Just few friendly after all notes so that you don't waste your time
learning that yourself:
- my patch didn't give us a third PPP implementation, we already have
  three: generic PPP, syncppp and PPP for ISDN :-)
- of those, both generic PPP and (I think) ISDN PPP are in a good
  shape
- generic PPP is specialized for dial-up async devices and has the
  required features (in kernel and in userspace pppd) - auth,
  multi-line, compression etc. It's a "lets give /dev/ttyS* a network
  device" implementation.
- ISDN PPP does for ISDN cards basically the same as generic PPP does
  for terminals. They are completely different from syncppp as the
  needs are completely different. Fixed-line PPP must be small and
  fast, and self-contained.


Now syncppp.c and sync serial (HDLC) cards. We basically have three
categories of hardware:
- mostly old ISA cards (cosa, cyclom-2x, z85230, s502/s508, personally
  I have c101 and n2 for tests, and PCI wanxl400). I think Alan
  maintains Zilog drivers, other "old" drivers are not maintained at
  all. They use syncppp.c directly (except c101, n2 and wanxl using
  generic HDLC).
  The list was longer but few drivers have been removed recently.

- cards with in-kernel obsolete drivers and users encouraged to use
  manufacturers' driver - lmc.

- a bit more modern PCI cards such as pc300, pci200syn, synclinks,
  dscc4, farsync, some off-tree ones. They use generic HDLC.

- I have offered to port any remaining old driver to generic HDLC if
  I'm sent a hardware sample. Synclink, dscc4 and farsync have been
  ported by other people.

I would be surprised to find that the "old" drivers are still
functional. Nevertheless syncppp.c is functional (though marginally)
and I think there are no bugs related to dev->priv and netdev_priv()
for those drivers.

The real problem with old drivers is the lack of hardware. Personally
I suspect they may have 0 users worldwide.


Syncppp.c is the problem for newer cards:
- the support has been broken recently due to netdev_priv() changes
  (could be band-aided).
- it's missing IPv6 and adding it is not trivial
- it's not RFC/STD-compliant at all, i.e. breaks many "MUST" rules and
  doesn't work with some other compliant implementations, and fixing
  that would be a real PITA,
- the code is really hard to parse/understand/maintain, it looks a bit
  like it was written in some other language and then "compiled" into
  C machine code :-)
- the lack of hardware makes doing significant changes to the drivers
  and syncppp almost impossible, without breaking them (if they aren't
  already broken).


That's why I came to an idea that leaving syncppp and the old drivers
in their current bit-rotting state which nobody can really fix, and
using another (fourth, and third when syncppp eventually dies) PPP
implementation is the only way out of this situation.

HTH.
-- 
Krzysztof Halasa

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[James Chapman]

Krzysztof Halasa wrote:
> David Miller <davem@davemloft.net> writes:
> 
>> I'll fix it properly because I know you won't do the work.
> 
> Great to read that.
> 
> Just few friendly after all notes so that you don't waste your time
> learning that yourself:
> - my patch didn't give us a third PPP implementation, we already have
>   three: generic PPP, syncppp and PPP for ISDN :-)
> - of those, both generic PPP and (I think) ISDN PPP are in a good
>   shape
> - generic PPP is specialized for dial-up async devices and has the
>   required features (in kernel and in userspace pppd) - auth,
>   multi-line, compression etc. It's a "lets give /dev/ttyS* a network
>   device" implementation.

Generic ppp isn't specialized at all, and it isn't limited to async 
serial devices. PPPoE, PPPoATM and L2TP use it.

> - ISDN PPP does for ISDN cards basically the same as generic PPP does
>   for terminals. They are completely different from syncppp as the
>   needs are completely different. Fixed-line PPP must be small and
>   fast, and self-contained.

<snip>

> That's why I came to an idea that leaving syncppp and the old drivers
> in their current bit-rotting state which nobody can really fix, and
> using another (fourth, and third when syncppp eventually dies) PPP
> implementation is the only way out of this situation.

Can you elaborate on why the code that uses syncppp can't use the 
generic ppp code together with a userspace ppp control protocol 
implementation like pppd?


-- 
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

James Chapman <jchapman@katalix.com> writes:

> Generic ppp isn't specialized at all, and it isn't limited to async
> serial devices. PPPoE, PPPoATM and L2TP use it.

Perhaps I should write "dial-up" and other non-fixed connections.

It's complex, I think kernel interface to generic HDLC would mean more
code than PPP implementation required for fixed lines.
Additional requirement - userspace daemon with additional plugin - may
not be the best thing for fixed lines either.

That would break backward compatibility, too.

Personally I think it could be acceptable but it's not for me to
decide.
-- 
Krzysztof Halasa

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Paul Fulghum]

Krzysztof Halasa wrote:
> It's complex, I think kernel interface to generic HDLC would mean more
> code than PPP implementation required for fixed lines.
> Additional requirement - userspace daemon with additional plugin - may
> not be the best thing for fixed lines either.
> 
> That would break backward compatibility, too.

I maintain both pppd and generic HDLC PPP
interfaces for the synclink drivers.
I would like to have a single PPP implementation,
but what Krzysztof writes about compatibility and complexity
(both in coding and user configuration) is a real issue.

Many customers who choose to use generic HDLC PPP are *dead*
set against the added complexity and (user space)
components of using pppd even though it has more features.
I say that having tried to persuade such users to use pppd.
The response is usually "support the simpler generic
HDLC PPP way of doing things or we will go elsewhere".
Others require the extra features of pppd.

I understand customer desires are not always rational
or a primary concern when making these architectural
decisions, but I know forcing the extra complexity and
components of pppd on generic HDLC users will cause a
lot of anger and defections.

-- 
Paul Fulghum
Microgate Systems, Ltd.

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Jeff Garzik]

Paul Fulghum wrote:
> Krzysztof Halasa wrote:
>> It's complex, I think kernel interface to generic HDLC would mean more
>> code than PPP implementation required for fixed lines.
>> Additional requirement - userspace daemon with additional plugin - may
>> not be the best thing for fixed lines either.
>>
>> That would break backward compatibility, too.
> 
> I maintain both pppd and generic HDLC PPP
> interfaces for the synclink drivers.
> I would like to have a single PPP implementation,
> but what Krzysztof writes about compatibility and complexity
> (both in coding and user configuration) is a real issue.
> 
> Many customers who choose to use generic HDLC PPP are *dead*
> set against the added complexity and (user space)
> components of using pppd even though it has more features.
> I say that having tried to persuade such users to use pppd.
> The response is usually "support the simpler generic
> HDLC PPP way of doing things or we will go elsewhere".
> Others require the extra features of pppd.
> 
> I understand customer desires are not always rational
> or a primary concern when making these architectural
> decisions, but I know forcing the extra complexity and
> components of pppd on generic HDLC users will cause a
> lot of anger and defections.

The fact that Krzysztof's solution was _small_ and _clean_ and easily 
maintainable was the reason I merged it [into my tree].

IMO sometimes "one size fits all" is not the best solution.

	Jeff

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[David Miller]

From: Jeff Garzik <jeff@garzik.org>
Date: Tue, 22 Apr 2008 16:50:41 -0400

> The fact that Krzysztof's solution was _small_ and _clean_ and easily 
> maintainable was the reason I merged it [into my tree].
> 
> IMO sometimes "one size fits all" is not the best solution.

This is besides the point.  We are discussing two things here:

1) How to "correctly" fix the syncppp private area bug.

2) How to, long term, support PPP properly in the kernel for
   various users.

The fact that non-HDLC users of syncppp got left broken is why I
objected to the change you merged in Jeff.  It simply duplicated the
majority of syncppp into the HDLC PPP code, which is just rediculious.

That had nothing to do with whether we should, in the long term, use
the generic PPP infrastructure we have now.

I would have been more than happy if syncppp was retained and fixed
properly, instead of being abandoned and duplicated in one fell swoop.
We don't do things like that Jeff.

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

David Miller <davem@davemloft.net> writes:

> The fact that non-HDLC users of syncppp got left broken

The fact is it isn't broken WRT dev->priv at least (I don't know about
random unrelated breakage).

The drivers:
a) alloc a netdev
b) set dev->priv to some private kmalloced area

It was like that for years, and it worked. The commit in question
changed netdev_priv() and the drivers don't use this function.

netdev_priv() was introduced at some point in time to allow
alloc_netdev() to optionally allocate additional memory for internal
use by the driver. It had nothing to do with dev->priv except "priv"
name. The drivers don't use it passing size 0.

This is probably suboptimal (two+ allocations instead of one), if the
drivers had maintainers with access to hardware I guess it would be
optimized, but it has nothing to do with the regression.

The regression is present in HDLC PPP only, becase HDLC PPP actually
used netdev_priv() (unlike non-HDLC PPP cases) as a means of addressing
the area following net_device struct and not for retrieving dev->priv,
and then the semantics suddenly changed.

> I would have been more than happy if syncppp was retained and fixed
> properly, instead of being abandoned and duplicated in one fell swoop.

I would be happy as well. The problem is that nobody shown any idea
how to do it.

I have offered: I will port any existing sync serial driver to generic
HDLC if I'm sent a free hardware sample. That includes old ISA cards
(I still have an old 2*PII-333 ISA test machine), and porting PC300
T1/E1 code to pc300too.

If some driver/hw has no users I think there is no point in keeping it
on life support here. The same goes for syncppp - if/when the number
of drivers using it drops to zero, we can let it go.
-- 
Krzysztof Halasa

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Wed, 23 Apr 2008 19:02:22 +0200

> netdev_priv() was introduced at some point in time to allow
> alloc_netdev() to optionally allocate additional memory for internal
> use by the driver. It had nothing to do with dev->priv except "priv"
> name. The drivers don't use it passing size 0.

No, it was added as an optimization since the private
area was allocated together with the struct netdev, and
thus at a constant offset computable a compile time.

It was never meant to provide a facility to have two private areas
associated with a device, but unfortunately some broken stuff decided
to use it that way.

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

David Miller <davem@davemloft.net> writes:

> No, it was added as an optimization since the private
> area was allocated together with the struct netdev, and
> thus at a constant offset computable a compile time.

That's essentially what I meant. And it has changed, call it whatever
you wish.
-- 
Krzysztof Halasa

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Thu, 24 Apr 2008 02:48:29 +0200

> David Miller <davem@davemloft.net> writes:
> 
> > No, it was added as an optimization since the private
> > area was allocated together with the struct netdev, and
> > thus at a constant offset computable a compile time.
> 
> That's essentially what I meant. And it has changed, call it whatever
> you wish.

The core kernel code sets dev->priv to the alloc_netdev() allocated
memory region.

It doesn't do that just for fun.

Any code which overrides that setting is asking for trouble.
What if the code kernel code that set it, needed to use it
during device release for some reason?

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

David Miller <davem@davemloft.net> writes:

> The core kernel code sets dev->priv to the alloc_netdev() allocated
> memory region.
>
> It doesn't do that just for fun.
>
> Any code which overrides that setting is asking for trouble.
> What if the code kernel code that set it, needed to use it
> during device release for some reason?

You are right, the point is Linux is a moving target, and I try to
write code as good as I can in given circumstances but never better.
I accept occasional breakage in obscure cases like syncppp+hdlc,
but I also need a way to fix it and/or remove the obscureness (or
whatever the right word is).

Historically we always have had two pointers:
- dev->priv
- first we had struct (net_)device *dev and by embedding it in larger
  structure we were able to have another private data area (even with
  Space.c)
- then netdev_alloc() and netdev_priv() came
- since 2.6.23 netdev_priv() returns dev->priv - one pointer is gone.

Anyway, I guess we should rather concentrate on what to do now with
the thing. Perhaps you have some idea?
-- 
Krzysztof Halasa

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Thu, 24 Apr 2008 15:12:17 +0200

> Anyway, I guess we should rather concentrate on what to do now with
> the thing. Perhaps you have some idea?

My current plan is to add an "official" mid-layer pointer for
these purposes.  So that the bug can have a simple fix while
we think about how better to handle this longer term.

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

David Miller <davem@davemloft.net> writes:

> My current plan is to add an "official" mid-layer pointer for
> these purposes.  So that the bug can have a simple fix while
> we think about how better to handle this longer term.

That would be great first step.

Though we will have to abandon syncppp use anyway, at least for
generic HDLC (I don't care much about the old ISA drivers).
-- 
Krzysztof Halasa

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Thu, 24 Apr 2008 15:39:47 +0200

> Though we will have to abandon syncppp use anyway, at least for
> generic HDLC (I don't care much about the old ISA drivers).

That, I still don't understand.

I would rather see you add small extensions to syncppp to
provide for HDLC's needs, rather than reimplement it.

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

David Miller <davem@davemloft.net> writes:

> I would rather see you add small extensions to syncppp to
> provide for HDLC's needs, rather than reimplement it.

It's not about extensions, syncppp is just unmaintainable. It's
implementation of two protocols - Cisco HDLC and (sync) PPP intermixed
together.

What would be needed is a) splitting them into two files (including
protocol selection in drivers - somehow), b) replacing existing
broken syncppp state machines distributed all over the place with a
single sane state machine, c) perhaps adding IPv6, people want it,
d) packet flow from hw drivers would need to be upgraded as well
(*_type_trans() and fast path for IP/IPv6 and perhaps IPX, though
I don't know anyone ever using IPX with sync PPP).

I guess then you'd get generic HDLC.

I think it's easier to a) simply remove syncppp.c, b) apply my patch
with new PPP, c) blindly port the old drivers to generic HDLC,
d) hope for the best, with a bit of luck nobody would notice.

Alternative c) simply remove the old drivers (d) unchanged).

I don't want the breakage resulting from either action thus I think
syncppp.c should stay there (basically unchanged, though we could
at last remove those variables used only by the original FreeBSD
code in 1994).

If the new implementation is my patch or if it uses generic PPP is
debatable. I think simplicity is a good thing, the interface to
generic PPP would be bigger than the whole sync-only PPP. But I'm not
against using generic PPP, it's (AFAICS) clean and *working* and
*maintained* (and I'm not worried about backward compatibility and
certainly not about users' visions).
-- 
Krzysztof Halasa

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Alan Cox]

> I think it's easier to a) simply remove syncppp.c, b) apply my patch
> with new PPP, c) blindly port the old drivers to generic HDLC,
> d) hope for the best, with a bit of luck nobody would notice.

I would agree with this. There isn't a sane way to sort out the old
kernel side syncppp (which is useless for todays networks as it doesn't
do compression, IPv6 or auth protocols) except by going to user space.

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

> I would agree with this. There isn't a sane way to sort out the old
> kernel side syncppp (which is useless for todays networks as it doesn't
> do compression, IPv6 or auth protocols) except by going to user space.

Well, I never used compression or auth on fixed line, and that would
require generic PPP. Perhaps we should use it, I don't know.
-- 
Krzysztof Halasa

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[David Miller]

From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Thu, 24 Apr 2008 21:44:57 +0100

> > I think it's easier to a) simply remove syncppp.c, b) apply my patch
> > with new PPP, c) blindly port the old drivers to generic HDLC,
> > d) hope for the best, with a bit of luck nobody would notice.
> 
> I would agree with this. There isn't a sane way to sort out the old
> kernel side syncppp (which is useless for todays networks as it doesn't
> do compression, IPv6 or auth protocols) except by going to user space.

Ok, but what we have in the tree should at least not be a crashing
mess that has to be marked BROKEN meanwhile :-)

This patch should address that issue.

Longer term I hope someone works on the infrastructure necessary that
would allow us to delete this and use generic PPP kernel+userspace
over these links.

Scanning over some of these older WAN drivers was truly scary.  No
locking at all over the HDLC protocol list, yikes!

commit 4951704b4e23d71b99ac933d8e6993bc6225ac13
Author: David S. Miller <davem@davemloft.net>
Date:   Mon May 12 03:29:11 2008 -0700

    syncppp: Fix crashes.
    
    The syncppp layer wants a mid-level netdev private pointer.
    
    It was using netdev->priv but that only worked by accident,
    and thus this scheme was broken when the device private
    allocation strategy changed.
    
    Add a proper mid-layer private pointer for uses like this,
    update syncppp and all users, and remove the HDLC_PPP broken
    tag from drivers/net/wan/Kconfig
    
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/net/wan/Kconfig b/drivers/net/wan/Kconfig
index 8005dd1..d5140ae 100644
--- a/drivers/net/wan/Kconfig
+++ b/drivers/net/wan/Kconfig
@@ -150,11 +150,9 @@ config HDLC_FR
 
 config HDLC_PPP
 	tristate "Synchronous Point-to-Point Protocol (PPP) support"
-	depends on HDLC && BROKEN
+	depends on HDLC
 	help
 	  Generic HDLC driver supporting PPP over WAN connections.
-	  This module is currently broken and will cause a kernel panic
-	  when a device configured in PPP mode is activated.
 
 	  It will be replaced by new PPP implementation in Linux 2.6.26.
 
diff --git a/drivers/net/wan/cosa.c b/drivers/net/wan/cosa.c
index 45ddfc9..b0fce13 100644
--- a/drivers/net/wan/cosa.c
+++ b/drivers/net/wan/cosa.c
@@ -629,7 +629,7 @@ static void sppp_channel_init(struct channel_data *chan)
 	d->base_addr = chan->cosa->datareg;
 	d->irq = chan->cosa->irq;
 	d->dma = chan->cosa->dma;
-	d->priv = chan;
+	d->ml_priv = chan;
 	sppp_attach(&chan->pppdev);
 	if (register_netdev(d)) {
 		printk(KERN_WARNING "%s: register_netdev failed.\n", d->name);
@@ -650,7 +650,7 @@ static void sppp_channel_delete(struct channel_data *chan)
 
 static int cosa_sppp_open(struct net_device *d)
 {
-	struct channel_data *chan = d->priv;
+	struct channel_data *chan = d->ml_priv;
 	int err;
 	unsigned long flags;
 
@@ -690,7 +690,7 @@ static int cosa_sppp_open(struct net_device *d)
 
 static int cosa_sppp_tx(struct sk_buff *skb, struct net_device *dev)
 {
-	struct channel_data *chan = dev->priv;
+	struct channel_data *chan = dev->ml_priv;
 
 	netif_stop_queue(dev);
 
@@ -701,7 +701,7 @@ static int cosa_sppp_tx(struct sk_buff *skb, struct net_device *dev)
 
 static void cosa_sppp_timeout(struct net_device *dev)
 {
-	struct channel_data *chan = dev->priv;
+	struct channel_data *chan = dev->ml_priv;
 
 	if (test_bit(RXBIT, &chan->cosa->rxtx)) {
 		chan->stats.rx_errors++;
@@ -720,7 +720,7 @@ static void cosa_sppp_timeout(struct net_device *dev)
 
 static int cosa_sppp_close(struct net_device *d)
 {
-	struct channel_data *chan = d->priv;
+	struct channel_data *chan = d->ml_priv;
 	unsigned long flags;
 
 	netif_stop_queue(d);
@@ -800,7 +800,7 @@ static int sppp_tx_done(struct channel_data *chan, int size)
 
 static struct net_device_stats *cosa_net_stats(struct net_device *dev)
 {
-	struct channel_data *chan = dev->priv;
+	struct channel_data *chan = dev->ml_priv;
 	return &chan->stats;
 }
 
@@ -1217,7 +1217,7 @@ static int cosa_sppp_ioctl(struct net_device *dev, struct ifreq *ifr,
 	int cmd)
 {
 	int rv;
-	struct channel_data *chan = dev->priv;
+	struct channel_data *chan = dev->ml_priv;
 	rv = cosa_ioctl_common(chan->cosa, chan, cmd, (unsigned long)ifr->ifr_data);
 	if (rv == -ENOIOCTLCMD) {
 		return sppp_do_ioctl(dev, ifr, cmd);
diff --git a/drivers/net/wan/hdlc_ppp.c b/drivers/net/wan/hdlc_ppp.c
index 10396d9..0030833 100644
--- a/drivers/net/wan/hdlc_ppp.c
+++ b/drivers/net/wan/hdlc_ppp.c
@@ -45,7 +45,7 @@ static int ppp_open(struct net_device *dev)
 	int (*old_ioctl)(struct net_device *, struct ifreq *, int);
 	int result;
 
-	dev->priv = &state(hdlc)->syncppp_ptr;
+	dev->ml_priv = &state(hdlc)->syncppp_ptr;
 	state(hdlc)->syncppp_ptr = &state(hdlc)->pppdev;
 	state(hdlc)->pppdev.dev = dev;
 
diff --git a/drivers/net/wan/hostess_sv11.c b/drivers/net/wan/hostess_sv11.c
index 83dbc92..f3065d3 100644
--- a/drivers/net/wan/hostess_sv11.c
+++ b/drivers/net/wan/hostess_sv11.c
@@ -75,7 +75,7 @@ static void hostess_input(struct z8530_channel *c, struct sk_buff *skb)
  
 static int hostess_open(struct net_device *d)
 {
-	struct sv11_device *sv11=d->priv;
+	struct sv11_device *sv11=d->ml_priv;
 	int err = -1;
 	
 	/*
@@ -128,7 +128,7 @@ static int hostess_open(struct net_device *d)
 
 static int hostess_close(struct net_device *d)
 {
-	struct sv11_device *sv11=d->priv;
+	struct sv11_device *sv11=d->ml_priv;
 	/*
 	 *	Discard new frames
 	 */
@@ -159,14 +159,14 @@ static int hostess_close(struct net_device *d)
 
 static int hostess_ioctl(struct net_device *d, struct ifreq *ifr, int cmd)
 {
-	/* struct sv11_device *sv11=d->priv;
+	/* struct sv11_device *sv11=d->ml_priv;
 	   z8530_ioctl(d,&sv11->sync.chanA,ifr,cmd) */
 	return sppp_do_ioctl(d, ifr,cmd);
 }
 
 static struct net_device_stats *hostess_get_stats(struct net_device *d)
 {
-	struct sv11_device *sv11=d->priv;
+	struct sv11_device *sv11=d->ml_priv;
 	if(sv11)
 		return z8530_get_stats(&sv11->sync.chanA);
 	else
@@ -179,7 +179,7 @@ static struct net_device_stats *hostess_get_stats(struct net_device *d)
  
 static int hostess_queue_xmit(struct sk_buff *skb, struct net_device *d)
 {
-	struct sv11_device *sv11=d->priv;
+	struct sv11_device *sv11=d->ml_priv;
 	return z8530_queue_xmit(&sv11->sync.chanA, skb);
 }
 
@@ -325,6 +325,7 @@ static struct sv11_device *sv11_init(int iobase, int irq)
 		/* 
 		 *	Initialise the PPP components
 		 */
+		d->ml_priv = sv;
 		sppp_attach(&sv->netdev);
 		
 		/*
@@ -333,7 +334,6 @@ static struct sv11_device *sv11_init(int iobase, int irq)
 		
 		d->base_addr = iobase;
 		d->irq = irq;
-		d->priv = sv;
 		
 		if(register_netdev(d))
 		{
diff --git a/drivers/net/wan/lmc/lmc_main.c b/drivers/net/wan/lmc/lmc_main.c
index 6635ece..62133ce 100644
--- a/drivers/net/wan/lmc/lmc_main.c
+++ b/drivers/net/wan/lmc/lmc_main.c
@@ -891,6 +891,7 @@ static int __devinit lmc_init_one(struct pci_dev *pdev,
 
     /* Initialize the sppp layer */
     /* An ioctl can cause a subsequent detach for raw frame interface */
+    dev->ml_priv = sc;
     sc->if_type = LMC_PPP;
     sc->check = 0xBEAFCAFE;
     dev->base_addr = pci_resource_start(pdev, 0);
diff --git a/drivers/net/wan/sealevel.c b/drivers/net/wan/sealevel.c
index 11276bf..44a89df 100644
--- a/drivers/net/wan/sealevel.c
+++ b/drivers/net/wan/sealevel.c
@@ -241,6 +241,7 @@ static inline struct slvl_device *slvl_alloc(int iobase, int irq)
 		return NULL;
 
 	sv = d->priv;
+	d->ml_priv = sv;
 	sv->if_ptr = &sv->pppdev;
 	sv->pppdev.dev = d;
 	d->base_addr = iobase;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 7c1d446..7469017 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -715,6 +715,9 @@ struct net_device
 	struct net		*nd_net;
 #endif
 
+	/* mid-layer private */
+	void			*ml_priv;
+
 	/* bridge stuff */
 	struct net_bridge_port	*br_port;
 	/* macvlan */
diff --git a/include/net/syncppp.h b/include/net/syncppp.h
index 877efa4..e43f407 100644
--- a/include/net/syncppp.h
+++ b/include/net/syncppp.h
@@ -59,7 +59,7 @@ struct ppp_device
 
 static inline struct sppp *sppp_of(struct net_device *dev) 
 {
-	struct ppp_device **ppp = dev->priv;
+	struct ppp_device **ppp = dev->ml_priv;
 	BUG_ON((*ppp)->dev != dev);
 	return &(*ppp)->sppp;
 }

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

David Miller <davem@davemloft.net> writes:

> No
> locking at all over the HDLC protocol list, yikes!

Right, I didn't think about ioctl() vs insmod/rmmod races. Will fix
when I'm back home and can test it, in few days.
-- 
Krzysztof Halasa

[PATCH] WAN: protect HDLC proto list while insmod/rmmod

[Krzysztof Halasa]

WAN: protect protocol list in hdlc.c with RTNL.
    
Signed-off-by: Krzysztof Hałasa <khc@pm.waw.pl>

--- a/drivers/net/wan/hdlc.c
+++ b/drivers/net/wan/hdlc.c
@@ -42,8 +42,7 @@ static const char* version = "HDLC support module revision 1.22";
 
 #undef DEBUG_LINK
 
-static struct hdlc_proto *first_proto = NULL;
-
+static struct hdlc_proto *first_proto;
 
 static int hdlc_change_mtu(struct net_device *dev, int new_mtu)
 {
@@ -313,21 +312,25 @@ void detach_hdlc_protocol(struct net_device *dev)
 
 void register_hdlc_protocol(struct hdlc_proto *proto)
 {
+	rtnl_lock();
 	proto->next = first_proto;
 	first_proto = proto;
+	rtnl_unlock();
 }
 
 
 void unregister_hdlc_protocol(struct hdlc_proto *proto)
 {
-	struct hdlc_proto **p = &first_proto;
-	while (*p) {
-		if (*p == proto) {
-			*p = proto->next;
-			return;
-		}
+	struct hdlc_proto **p;
+
+	rtnl_lock();
+	p = &first_proto;
+	while (*p != proto) {
+		BUG_ON(!*p);
 		p = &((*p)->next);
 	}
+	*p = proto->next;
+	rtnl_unlock();
 }
 
 

Re: [PATCH] WAN: protect HDLC proto list while insmod/rmmod

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Mon, 19 May 2008 19:00:51 +0200

> WAN: protect protocol list in hdlc.c with RTNL.
>     
> Signed-off-by: Krzysztof Hałasa <khc@pm.waw.pl>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH] WAN: protect HDLC proto list while insmod/rmmod

[Jeff Garzik]

Krzysztof Halasa wrote:
> WAN: protect protocol list in hdlc.c with RTNL.
>     
> Signed-off-by: Krzysztof Hałasa <khc@pm.waw.pl>

applied

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

BTW: The dev->ml_priv addition is obviously a good step but I guess
you don't need the dev->priv hackery duplicated with dev->ml_priv.

Can't test it at the moment but I'd leave dev->priv alone and only get
rid of the syncppp_ptr here. Then I'd simply use dev->ml_priv as
syncppp_ptr and dev->priv would be a private driver pointer, as
usual. Generic HDLC uses dev->priv for itself but the relevant hw
drivers use hdlc->priv instead so it's not a problem.

When syncppp.c is removed we can use dev->ml_priv for HDLC layer,
dev->priv for hw driver, and hdlc->priv would be no longer needed.

I mean the following:

David Miller <davem@davemloft.net> writes:

> --- a/include/net/syncppp.h
> +++ b/include/net/syncppp.h
> @@ -59,7 +59,7 @@ struct ppp_device
>  
>  static inline struct sppp *sppp_of(struct net_device *dev) 
>  {
> -	struct ppp_device **ppp = dev->priv;
> +	struct ppp_device **ppp = dev->ml_priv;
>  	BUG_ON((*ppp)->dev != dev);
>  	return &(*ppp)->sppp;
>  }

would become:

static inline struct sppp *sppp_of(struct net_device *dev) 
{
	return dev->ml_priv;
}

The changes are not automatic, I'll look at it when able.
-- 
Krzysztof Halasa

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Krzysztof Halasa]

> But I'm not
> against using generic PPP, it's (AFAICS) clean and *working* and
> *maintained* (and I'm not worried about backward compatibility and
> certainly not about users' visions).

IOW I value correctness over backward compatibility and users'
visions, obviously I try to keep compatibility and meet users'
needs when it actually makes sense.
-- 
Krzysztof Halasa

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[James Chapman]

Jeff Garzik wrote:
> Paul Fulghum wrote:
>> Krzysztof Halasa wrote:
>>> It's complex, I think kernel interface to generic HDLC would mean more
>>> code than PPP implementation required for fixed lines.
>>> Additional requirement - userspace daemon with additional plugin - may
>>> not be the best thing for fixed lines either.
>>>
>>> That would break backward compatibility, too.
>>
>> I maintain both pppd and generic HDLC PPP
>> interfaces for the synclink drivers.
>> I would like to have a single PPP implementation,
>> but what Krzysztof writes about compatibility and complexity
>> (both in coding and user configuration) is a real issue.
>>
>> Many customers who choose to use generic HDLC PPP are *dead*
>> set against the added complexity and (user space)
>> components of using pppd even though it has more features.
>> I say that having tried to persuade such users to use pppd.
>> The response is usually "support the simpler generic
>> HDLC PPP way of doing things or we will go elsewhere".
>> Others require the extra features of pppd.
>>
>> I understand customer desires are not always rational
>> or a primary concern when making these architectural
>> decisions, but I know forcing the extra complexity and
>> components of pppd on generic HDLC users will cause a
>> lot of anger and defections.

Are there technical reasons or is the complexity just a lack of familiarity?

> The fact that Krzysztof's solution was _small_ and _clean_ and easily 
> maintainable was the reason I merged it [into my tree].
> 
> IMO sometimes "one size fits all" is not the best solution.

I guess what caught my eye is a PPP control protocol implementation 
being in the kernel. I'd seen syncppp before but I assumed it was there 
for legacy reasons. A while ago there seemed to be strong desire to move 
control protocols such as bridge spanning tree into userspace. Is this 
no longer the case?

-- 
James Chapman
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[David Miller]

From: James Chapman <jchapman@katalix.com>
Date: Tue, 22 Apr 2008 23:23:31 +0100

> I guess what caught my eye is a PPP control protocol implementation 
> being in the kernel. I'd seen syncppp before but I assumed it was there 
> for legacy reasons. A while ago there seemed to be strong desire to move 
> control protocols such as bridge spanning tree into userspace. Is this 
> no longer the case?

It is still the case, believe me :-)

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[David Miller]

From: Paul Fulghum <paulkf@microgate.com>
Date: Tue, 22 Apr 2008 15:46:27 -0600

> The response is usually "support the simpler generic
> HDLC PPP way of doing things or we will go elsewhere".

Users say this to strong-hand developers, it's not something you
should ever take very seriously.  And even if Linux may simply not be
for them, well that's fine too, and implementing something as obscure
as HDLC PPP one way or the other is not going to change that.

I mean, be realistic here.

Are we going to have three copies of code implementing the same
thing because some HDLC PPP users threatened to defect?  That's
simply rediculious.

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Paul Fulghum]

David Miller wrote:
 > Users say this to strong-hand developers, it's not something you
 > should ever take very seriously.  And even if Linux may simply not be
 > for them, well that's fine too, and implementing something as obscure
 > as HDLC PPP one way or the other is not going to change that.

Certainly not a big deal for Linux, but more
significant for vendors of HDLC hardware :-)

David Miller wrote:
 > I would have been more than happy if syncppp was retained and fixed
 > properly, instead of being abandoned and duplicated in one fell swoop.

I'd be happy with that also. I was responding to the
suggestion of merging generic HDLC PPP with the pppd implementation.
It's been suggested before, but doing so looks messy.

James Chapman wrote:
 > Paul Fulghum wrote:
 >> Many customers who choose to use generic HDLC PPP are *dead*
 >> set against the added complexity and (user space)
 >> components of using pppd even though it has more features.
 >
 > Are there technical reasons or is the complexity just a lack of
 > familiarity?

 From what I can tell it was an existing investment in scripts,
training, tools, naming conventions, etc. Even when
provided with new tools and scripts that do the same thing (as
far as I could tell) the response was suprisingly vehement against
change.

--
Paul

Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

[Jeff Garzik]

Krzysztof Halasa wrote:
> New synchronous PPP implementation for generic HDLC.
> 
> Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl>
> 
>  drivers/net/wan/Makefile   |    2 +-
>  drivers/net/wan/hdlc_ppp.c |  649 ++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 602 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/net/wan/Makefile b/drivers/net/wan/Makefile
> index d61fef3..3081683 100644
> --- a/drivers/net/wan/Makefile
> +++ b/drivers/net/wan/Makefile
> @@ -14,7 +14,7 @@ obj-$(CONFIG_HDLC_RAW)		+= hdlc_raw.o
>  obj-$(CONFIG_HDLC_RAW_ETH)	+= hdlc_raw_eth.o
>  obj-$(CONFIG_HDLC_CISCO)	+= hdlc_cisco.o
>  obj-$(CONFIG_HDLC_FR)		+= hdlc_fr.o
> -obj-$(CONFIG_HDLC_PPP)		+= hdlc_ppp.o	syncppp.o
> +obj-$(CONFIG_HDLC_PPP)		+= hdlc_ppp.o
>  obj-$(CONFIG_HDLC_X25)		+= hdlc_x25.o

applied