Skip to contentSkip to conversation

WAN: converting generic HDLC to use netif_dormant*()

PATCH Applied Krzysztof Halasa 3 participants 12 messages Jul 11, 2006 – Jul 17, 2006

Subsystems (6)
Networking Network drivers Char and misc drivers X.25 stack +2 more subsystems
lore
Share & tools
lore

Krzysztof Halasa

author maintainer
#0 lore raw

PATCH 1/1 WAN: converting generic HDLC to use netif_dormant*()

Hi,

This patch converts generic HDLC (and WAN drivers using it) from hdlc_set_carrier() to netif_dormant*() interface.
WAN hardware drivers should now use netif_carrier_on|off() like other network drivers.

Please apply. Thanks.

Signed-off-by Krzysztof Halasa <[email protected]>
drivers/net/wan/c101.c +16 -10
index 43d854a..3796a59 100644
@@ -116,27 +116,33 @@ static inline void openwin(card_t *card,
 #include "hd6457x.c"
 
 
+static inline void set_carrier(port_t *port)
+{
+	if (!sca_in(MSCI1_OFFSET + ST3, port) & ST3_DCD)
+		netif_carrier_on(port_to_dev(port));
+	else
+		netif_carrier_off(port_to_dev(port));
+}
+
+
 static void sca_msci_intr(port_t *port)
 {
-	struct net_device *dev = port_to_dev(port);
-	card_t* card = port_to_card(port);
-	u8 stat = sca_in(MSCI1_OFFSET + ST1, card); /* read MSCI ST1 status */
+	u8 stat = sca_in(MSCI1_OFFSET + ST1, port); /* read MSCI ST1 status */
 
 	/* Reset MSCI TX underrun status bit */
-	sca_out(stat & ST1_UDRN, MSCI0_OFFSET + ST1, card);
+	sca_out(stat & ST1_UDRN, MSCI0_OFFSET + ST1, port);
 
 	if (stat & ST1_UDRN) {
-		struct net_device_stats *stats = hdlc_stats(dev);
+		struct net_device_stats *stats = hdlc_stats(port_to_dev(port));
 		stats->tx_errors++; /* TX Underrun error detected */
 		stats->tx_fifo_errors++;
 	}
 
 	/* Reset MSCI CDCD status bit - uses ch#2 DCD input */
-	sca_out(stat & ST1_CDCD, MSCI1_OFFSET + ST1, card);
+	sca_out(stat & ST1_CDCD, MSCI1_OFFSET + ST1, port);
 
 	if (stat & ST1_CDCD)
-		hdlc_set_carrier(!(sca_in(MSCI1_OFFSET + ST3, card) & ST3_DCD),
-				 dev);
+		set_carrier(port);
 }
 
 

@@ -190,7 +196,7 @@ static int c101_open(struct net_device *
 	sca_out(IE1_UDRN, MSCI0_OFFSET + IE1, port);
 	sca_out(IE0_TXINT, MSCI0_OFFSET + IE0, port);
 
-	hdlc_set_carrier(!(sca_in(MSCI1_OFFSET + ST3, port) & ST3_DCD), dev);
+	set_carrier(port);
 	printk(KERN_DEBUG "0x%X\n", sca_in(MSCI1_OFFSET + ST3, port));
 
 	/* enable MSCI1 CDCD interrupt */

@@ -378,7 +384,7 @@ static int __init c101_run(unsigned long
 	}
 
 	sca_init_sync_port(card); /* Set up C101 memory */
-	hdlc_set_carrier(!(sca_in(MSCI1_OFFSET + ST3, card) & ST3_DCD), dev);
+	set_carrier(card);
 
 	printk(KERN_INFO "%s: Moxa C101 on IRQ%u,"
 	       " using %u TX + %u RX packets rings\n",
drivers/net/wan/hd6457x.c +20 -6
index d374332..dce2bb3 100644
@@ -168,6 +168,23 @@ static inline u32 buffer_offset(port_t *
 }
 
 
+static inline void sca_set_carrier(port_t *port)
+{
+	if (!(sca_in(get_msci(port) + ST3, port_to_card(port)) & ST3_DCD)) {
+#ifdef DEBUG_LINK
+		printk(KERN_DEBUG "%s: sca_set_carrier on\n",
+		       port_to_dev(port)->name);
+#endif
+		netif_carrier_on(port_to_dev(port));
+	} else {
+#ifdef DEBUG_LINK
+		printk(KERN_DEBUG "%s: sca_set_carrier off\n",
+		       port_to_dev(port)->name);
+#endif
+		netif_carrier_off(port_to_dev(port));
+	}
+}
+
 
 static void sca_init_sync_port(port_t *port)
 {

@@ -237,9 +254,7 @@ #endif
 			sca_out(DIR_BOFE, DIR_TX(phy_node(port)), card);
 		}
 	}
-
-	hdlc_set_carrier(!(sca_in(get_msci(port) + ST3, card) & ST3_DCD),
-			 port_to_dev(port));
+	sca_set_carrier(port);
 }
 
 

@@ -262,8 +277,7 @@ static inline void sca_msci_intr(port_t 
 	}
 
 	if (stat & ST1_CDCD)
-		hdlc_set_carrier(!(sca_in(msci + ST3, card) & ST3_DCD),
-				 port_to_dev(port));
+		sca_set_carrier(port);
 }
 #endif
 

@@ -566,7 +580,7 @@ #endif
    - all DMA interrupts
 */
 
-	hdlc_set_carrier(!(sca_in(msci + ST3, card) & ST3_DCD), dev);
+	sca_set_carrier(port);
 
 #ifdef __HD64570_H
 	/* MSCI TX INT and RX INT A IRQ enable */
drivers/net/wan/hdlc_cisco.c +4 -10
index 1fd0466..f289dab 100644
@@ -192,9 +192,7 @@ static int cisco_rx(struct sk_buff *skb)
 					       "uptime %ud%uh%um%us)\n",
 					       dev->name, days, hrs,
 					       min, sec);
-#if 0
-					netif_carrier_on(dev);
-#endif
+					netif_dormant_off(dev);
 					hdlc->state.cisco.up = 1;
 				}
 			}

@@ -227,9 +225,7 @@ static void cisco_timer(unsigned long ar
 		       hdlc->state.cisco.settings.timeout * HZ)) {
 		hdlc->state.cisco.up = 0;
 		printk(KERN_INFO "%s: Link down\n", dev->name);
-#if 0
-		netif_carrier_off(dev);
-#endif
+		netif_dormant_on(dev);
 	}
 
 	cisco_keepalive_send(dev, CISCO_KEEPALIVE_REQ,

@@ -265,10 +261,7 @@ static void cisco_stop(struct net_device
 {
 	hdlc_device *hdlc = dev_to_hdlc(dev);
 	del_timer_sync(&hdlc->state.cisco.timer);
-#if 0
-	if (netif_carrier_ok(dev))
-		netif_carrier_off(dev);
-#endif
+	netif_dormant_on(dev);
 	hdlc->state.cisco.up = 0;
 	hdlc->state.cisco.request_sent = 0;
 }

@@ -328,6 +321,7 @@ int hdlc_cisco_ioctl(struct net_device *
 		dev->type = ARPHRD_CISCO;
 		dev->flags = IFF_POINTOPOINT | IFF_NOARP;
 		dev->addr_len = 0;
+		netif_dormant_on(dev);
 		return 0;
 	}
drivers/net/wan/hdlc_fr.c +3 -11
index 523afe1..7bb737b 100644
@@ -301,7 +301,7 @@ static int pvc_open(struct net_device *d
 	if (pvc->open_count++ == 0) {
 		hdlc_device *hdlc = dev_to_hdlc(pvc->master);
 		if (hdlc->state.fr.settings.lmi == LMI_NONE)
-			pvc->state.active = hdlc->carrier;
+			pvc->state.active = netif_carrier_ok(pvc->master);
 
 		pvc_carrier(pvc->state.active, pvc);
 		hdlc->state.fr.dce_changed = 1;

@@ -545,11 +545,7 @@ static void fr_set_link_state(int reliab
 
 	hdlc->state.fr.reliable = reliable;
 	if (reliable) {
-#if 0
-		if (!netif_carrier_ok(dev))
-			netif_carrier_on(dev);
-#endif
-
+		netif_dormant_off(dev);
 		hdlc->state.fr.n391cnt = 0; /* Request full status */
 		hdlc->state.fr.dce_changed = 1;
 

@@ -562,11 +558,7 @@ #endif
 			}
 		}
 	} else {
-#if 0
-		if (netif_carrier_ok(dev))
-			netif_carrier_off(dev);
-#endif
-
+		netif_dormant_on(dev);
 		while (pvc) {		/* Deactivate all PVCs */
 			pvc_carrier(0, pvc);
 			pvc->state.exist = pvc->state.active = 0;
drivers/net/wan/hdlc_generic.c +35 -30

152 lines not on this page · raw message

drivers/net/wan/wanxl.c +4 -1
index 29a756d..006f3c5 100644
@@ -149,7 +149,10 @@ static inline void wanxl_cable_intr(port
 	printk(KERN_INFO "%s: %s%s module, %s cable%s%s\n",
 	       port->dev->name, pm, dte, cable, dsr, dcd);
 
-	hdlc_set_carrier(value & STATUS_CABLE_DCD, port->dev);
+	if (value & STATUS_CABLE_DCD)
+		netif_carrier_on(port->dev);
+	else
+		netif_carrier_off(port->dev);
 }
include/linux/hdlc.h +0 -2
index df695e9..9e46b25 100644
@@ -224,8 +224,6 @@ static __inline__ void debug_frame(const
 int hdlc_open(struct net_device *dev);
 /* Must be called by hardware driver when HDLC device is being closed */
 void hdlc_close(struct net_device *dev);
-/* Called by hardware driver when DCD line level changes */
-void hdlc_set_carrier(int on, struct net_device *dev);
 
 /* May be used by hardware driver to gain control over HDLC device */
 static __inline__ void hdlc_proto_detach(hdlc_device *hdlc)
maintainer Applied
#1 lore raw

From: Krzysztof Halasa <[email protected]>
Date: Tue, 11 Jul 2006 23:18:45 +0200

Quoted 8 lines

This patch converts generic HDLC (and WAN drivers using it) from hdlc_set_carrier() to netif_dormant*() interface.
WAN hardware drivers should now use netif_carrier_on|off() like other network drivers.

Please apply. Thanks.

Signed-off-by Krzysztof Halasa <[email protected]>

Applied.

#2 lore raw
Quoted 8 lines · On Wed, 12 Jul 2006 13:46:32 -0700 (PDT) David Miller <[email protected]>…

From: Krzysztof Halasa <[email protected]>
Date: Tue, 11 Jul 2006 23:18:45 +0200

Quoted 5 lines

This patch converts generic HDLC (and WAN drivers using it) from hdlc_set_carrier() to netif_dormant*() interface.
WAN hardware drivers should now use netif_carrier_on|off() like other network drivers.

Quoted 5 lines
Quoted 3 lines

Please apply. Thanks.

Signed-off-by Krzysztof Halasa <[email protected]>

Applied.

Broken ;)

WARNING: "hdlc_set_carrier" [drivers/char/synclinkmp.ko] undefined!
WARNING: "hdlc_set_carrier" [drivers/char/synclink_gt.ko] undefined!
WARNING: "hdlc_set_carrier" [drivers/char/synclink.ko] undefined!
WARNING: "hdlc_set_carrier" [drivers/char/pcmcia/synclink_cs.ko] undefined!

maintainer
#3 lore raw

From: Andrew Morton <[email protected]>
Date: Thu, 13 Jul 2006 19:45:04 -0700

Quoted 15 lines
Quoted 12 lines · On Wed, 12 Jul 2006 13:46:32 -0700 (PDT) David Miller <[email protected]>…

From: Krzysztof Halasa <[email protected]>
Date: Tue, 11 Jul 2006 23:18:45 +0200

Quoted 8 lines

This patch converts generic HDLC (and WAN drivers using it) from hdlc_set_carrier() to netif_dormant*() interface.
WAN hardware drivers should now use netif_carrier_on|off() like other network drivers.

Please apply. Thanks.

Signed-off-by Krzysztof Halasa <[email protected]>
Quoted 8 lines
Quoted 1 line

Applied.

Broken ;)

WARNING: "hdlc_set_carrier" [drivers/char/synclinkmp.ko] undefined!
WARNING: "hdlc_set_carrier" [drivers/char/synclink_gt.ko] undefined!
WARNING: "hdlc_set_carrier" [drivers/char/synclink.ko] undefined!
WARNING: "hdlc_set_carrier" [drivers/char/pcmcia/synclink_cs.ko] undefined!

Krzysztof please provide a fix for this, this function you added calls to does not exist anywhere, which suggests you either produced a patch against a different tree than the current one or you didn't even compile test this patch.

Thanks.

author maintainer
#4 lore raw
Quoted 6 lines · David Miller
Quoted 4 lines

WARNING: "hdlc_set_carrier" [drivers/char/synclinkmp.ko] undefined!
WARNING: "hdlc_set_carrier" [drivers/char/synclink_gt.ko] undefined!
WARNING: "hdlc_set_carrier" [drivers/char/synclink.ko] undefined!
WARNING: "hdlc_set_carrier" [drivers/char/pcmcia/synclink_cs.ko] undefined!

Krzysztof please provide a fix for this,

Sure

Quoted 2 lines

this function you added
calls to does not exist anywhere,

The function doesn't exist anymore, I've removed the calls (but unfortunately not all of them). Now, instead of hdlc_set_carrier(), normal netif_carrier_on|off() should be used (it looks like only synclink drivers and drivers written by me used hdlc_set_carrier()
- other drivers always used netif_carrier_*() only).

Quoted 3 lines

which suggests you either produced
a patch against a different tree than the current one or you didn't even compile test this patch.

Actually I wasn't at the moment aware that there are WAN drivers in drivers/char (only checked drivers/net).

I'm just posting a patch.

Signature · 1 line
Krzysztof Halasa
author maintainer
#5 lore raw

PATCH 1/1 WAN: Added missing netif_dormant_off() to generic HDLC

In reply to Krzysztof Halasa

WAN: Fixed a problem with PPP/raw HDLC/X.25 protocols not doing netif_dormant_off() at startup.

Signed-off-by Krzysztof Halasa <[email protected]>
drivers/net/wan/hdlc_ppp.c +1 -0
index b81263e..fbaab5b 100644
@@ -107,6 +107,7 @@ int hdlc_ppp_ioctl(struct net_device *de
 		dev->hard_header = NULL;
 		dev->type = ARPHRD_PPP;
 		dev->addr_len = 0;
+		netif_dormant_off(dev);
 		return 0;
 	}
drivers/net/wan/hdlc_raw.c +1 -0
index 9456d31..f15aa6b 100644
@@ -82,6 +82,7 @@ int hdlc_raw_ioctl(struct net_device *de
 		dev->type = ARPHRD_RAWHDLC;
 		dev->flags = IFF_POINTOPOINT | IFF_NOARP;
 		dev->addr_len = 0;
+		netif_dormant_off(dev);
 		return 0;
 	}
drivers/net/wan/hdlc_raw_eth.c +1 -0
index b1285cc..d188498 100644
@@ -100,6 +100,7 @@ int hdlc_raw_eth_ioctl(struct net_device
 		dev->tx_queue_len = old_qlen;
 		memcpy(dev->dev_addr, "\x00\x01", 2);
                 get_random_bytes(dev->dev_addr + 2, ETH_ALEN - 2);
+		netif_dormant_off(dev);
 		return 0;
 	}
drivers/net/wan/hdlc_x25.c +1 -0
index 07e5eef..a867fb4 100644
@@ -212,6 +212,7 @@ int hdlc_x25_ioctl(struct net_device *de
 		dev->hard_header = NULL;
 		dev->type = ARPHRD_X25;
 		dev->addr_len = 0;
+		netif_dormant_off(dev);
 		return 0;
 	}
maintainer Applied
#6 lore raw

Re: [PATCH] WAN: Added missing netif_dormant_off() to generic HDLC

From: Krzysztof Halasa <[email protected]>
Date: Sun, 16 Jul 2006 14:17:20 +0200

Quoted 4 lines

WAN: Fixed a problem with PPP/raw HDLC/X.25 protocols not doing netif_dormant_off() at startup.

Signed-off-by Krzysztof Halasa <[email protected]>

Applied, thanks.

author maintainer
#7 lore raw

PATCH 1/1 WAN: Cosmetic changes to N2 and C101 drivers

In reply to Krzysztof Halasa

WAN: Cosmetic changes to N2 and C101 drivers

Signed-off-by Krzysztof Halasa <[email protected]>
drivers/net/wan/c101.c +2 -2
index 3796a59..2911be3 100644
@@ -197,7 +197,6 @@ static int c101_open(struct net_device *
 	sca_out(IE0_TXINT, MSCI0_OFFSET + IE0, port);
 
 	set_carrier(port);
-	printk(KERN_DEBUG "0x%X\n", sca_in(MSCI1_OFFSET + ST3, port));
 
 	/* enable MSCI1 CDCD interrupt */
 	sca_out(IE1_CDCD, MSCI1_OFFSET + IE1, port);

@@ -449,4 +448,5 @@ module_exit(c101_cleanup);
 MODULE_AUTHOR("Krzysztof Halasa <[email protected]>");
 MODULE_DESCRIPTION("Moxa C101 serial port driver");
 MODULE_LICENSE("GPL v2");
-module_param(hw, charp, 0444);	/* hw=irq,ram:irq,... */
+module_param(hw, charp, 0444);
+MODULE_PARM_DESC(hw, "irq,ram:irq,...");
drivers/net/wan/n2.c +2 -1
index cd32751..0a17ad6 100644
@@ -559,4 +559,5 @@ module_exit(n2_cleanup);
 MODULE_AUTHOR("Krzysztof Halasa <[email protected]>");
 MODULE_DESCRIPTION("RISCom/N2 serial port driver");
 MODULE_LICENSE("GPL v2");
-module_param(hw, charp, 0444);	/* hw=io,irq,ram,ports:io,irq,... */
+module_param(hw, charp, 0444);
+MODULE_PARM_DESC(hw, "io,irq,ram,ports:io,irq,...");
author maintainer
#9 lore raw

PATCH 1/1 WAN: Converted synclink drivers to use netif_carrier_*()

In reply to Krzysztof Halasa

WAN: Converted synclink drivers to use netif_carrier_*() instead of hdlc_set_carrier().

Signed-off-by Krzysztof Halasa <[email protected]>
drivers/char/pcmcia/synclink_cs.c +10 -4
index 0721345..bdd09d9 100644
@@ -1174,8 +1174,12 @@ static void dcd_change(MGSLPC_INFO *info
 	else
 		info->input_signal_events.dcd_down++;
 #ifdef CONFIG_HDLC
-	if (info->netcount)
-		hdlc_set_carrier(info->serial_signals & SerialSignal_DCD, info->netdev);
+	if (info->netcount) {
+		if (info->serial_signals & SerialSignal_DCD)
+			netif_carrier_on(info->netdev);
+		else
+			netif_carrier_off(info->netdev);
+	}
 #endif
 	wake_up_interruptible(&info->status_event_wait_q);
 	wake_up_interruptible(&info->event_wait_q);

@@ -4251,8 +4255,10 @@ static int hdlcdev_open(struct net_devic
 	spin_lock_irqsave(&info->lock, flags);
 	get_signals(info);
 	spin_unlock_irqrestore(&info->lock, flags);
-	hdlc_set_carrier(info->serial_signals & SerialSignal_DCD, dev);
-
+	if (info->serial_signals & SerialSignal_DCD)
+		netif_carrier_on(dev);
+	else
+		netif_carrier_off(dev);
 	return 0;
 }
drivers/char/synclink.c +10 -4
index fee2aca..ecf4786 100644
@@ -1344,8 +1344,12 @@ static void mgsl_isr_io_pin( struct mgsl
 			} else
 				info->input_signal_events.dcd_down++;
 #ifdef CONFIG_HDLC
-			if (info->netcount)
-				hdlc_set_carrier(status & MISCSTATUS_DCD, info->netdev);
+			if (info->netcount) {
+				if (status & MISCSTATUS_DCD)
+					netif_carrier_on(info->netdev);
+				else
+					netif_carrier_off(info->netdev);
+			}
 #endif
 		}
 		if (status & MISCSTATUS_CTS_LATCHED)

@@ -7844,8 +7848,10 @@ static int hdlcdev_open(struct net_devic
 	spin_lock_irqsave(&info->irq_spinlock, flags);
 	usc_get_serial_signals(info);
 	spin_unlock_irqrestore(&info->irq_spinlock, flags);
-	hdlc_set_carrier(info->serial_signals & SerialSignal_DCD, dev);
-
+	if (info->serial_signals & SerialSignal_DCD)
+		netif_carrier_on(dev);
+	else
+		netif_carrier_off(dev);
 	return 0;
 }
drivers/char/synclink_gt.c +10 -4
index b4d1f4e..03f5cdf 100644
@@ -1497,8 +1497,10 @@ static int hdlcdev_open(struct net_devic
 	spin_lock_irqsave(&info->lock, flags);
 	get_signals(info);
 	spin_unlock_irqrestore(&info->lock, flags);
-	hdlc_set_carrier(info->signals & SerialSignal_DCD, dev);
-
+	if (info->signals & SerialSignal_DCD)
+		netif_carrier_on(dev);
+	else
+		netif_carrier_off(dev);
 	return 0;
 }
 

@@ -1997,8 +1999,12 @@ static void dcd_change(struct slgt_info 
 		info->input_signal_events.dcd_down++;
 	}
 #ifdef CONFIG_HDLC
-	if (info->netcount)
-		hdlc_set_carrier(info->signals & SerialSignal_DCD, info->netdev);
+	if (info->netcount) {
+		if (info->signals & SerialSignal_DCD)
+			netif_carrier_on(info->netdev);
+		else
+			netif_carrier_off(info->netdev);
+	}
 #endif
 	wake_up_interruptible(&info->status_event_wait_q);
 	wake_up_interruptible(&info->event_wait_q);
drivers/char/synclinkmp.c +10 -4
index 8587401..9681255 100644
@@ -1753,8 +1753,10 @@ static int hdlcdev_open(struct net_devic
 	spin_lock_irqsave(&info->lock, flags);
 	get_signals(info);
 	spin_unlock_irqrestore(&info->lock, flags);
-	hdlc_set_carrier(info->serial_signals & SerialSignal_DCD, dev);
-
+	if (info->serial_signals & SerialSignal_DCD)
+		netif_carrier_on(dev);
+	else
+		netif_carrier_off(dev);
 	return 0;
 }
 

@@ -2523,8 +2525,12 @@ void isr_io_pin( SLMP_INFO *info, u16 st
 			} else
 				info->input_signal_events.dcd_down++;
 #ifdef CONFIG_HDLC
-			if (info->netcount)
-				hdlc_set_carrier(status & SerialSignal_DCD, info->netdev);
+			if (info->netcount) {
+				if (status & SerialSignal_DCD)
+					netif_carrier_on(info->netdev);
+				else
+					netif_carrier_off(info->netdev);
+			}
 #endif
 		}
 		if (status & MISCSTATUS_CTS_LATCHED)
maintainer
#10 lore raw

Re: [PATCH] WAN: Converted synclink drivers to use netif_carrier_*()

From: Krzysztof Halasa <[email protected]>
Date: Sun, 16 Jul 2006 14:23:49 +0200

Quoted 4 lines

WAN: Converted synclink drivers to use netif_carrier_*() instead of hdlc_set_carrier().

Signed-off-by Krzysztof Halasa <[email protected]>

Also applied, thanks for fixing this up Krzysztof.

author maintainer
#11 lore raw

Generic HDLC changes, Was: Re: [PATCH] WAN: Converted synclink drivers to use netif_carrier_*()

Quoted 1 line · David Miller

Also applied, thanks for fixing this up Krzysztof.

You're welcome.

I have another thing: generic HDLC is a set of medium-level protocols (PPP, Cisco HDLC, FR etc.), used by another set of low-level hardware drivers. I'd like to modularize the protocols (they are currently all linked together into hdlc.ko or vmlinux), aiming at 2.6.19 or so.

(The Cyclades PC300 maintainer is Cc:ed).

This includes:

linux/hdlc.h +1 -17
@@ -107,82 +58,18 @@ typedef struct hdlc_device_struct {
 	/* hardware driver must handle this instead of dev->hard_start_xmit */
 	int (*xmit)(struct sk_buff *skb, struct net_device *dev);
 
-
 	/* Things below are for HDLC layer internal use only */
-	struct {
-		int (*open)(struct net_device *dev);
-		void (*close)(struct net_device *dev);
-
-		/* if open & DCD */
-		void (*start)(struct net_device *dev);
-		/* if open & !DCD */
-		void (*stop)(struct net_device *dev);
-
-		void (*detach)(struct hdlc_device_struct *hdlc);
-		int (*netif_rx)(struct sk_buff *skb);
-		unsigned short (*type_trans)(struct sk_buff *skb,
-					     struct net_device *dev);
-		int id;		/* IF_PROTO_HDLC/CISCO/FR/etc. */
-	}proto;
-
+	const struct hdlc_proto *proto;

There is a problem with a particular hardware driver, PC300. It provides some additional protocol support for PC300 cards, relaying on things clearly marked as internal to HDLC (for example, it reads and writes proto.id). This is no longer possible if the drivers are modularized, the variables wouldn't exist (and even if not deleted accessing them would be unsafe).

It looks access to this private variable is needed to implement a separate protocol control channel, parallel to the one used by generic HDLC (to have one tool for hardware and protocol control?).

I believe the correct fix to this problem would be either:
- using attach/detach mechanism provided by generic HDLC (the hardware driver can provide additional support with it), or
- using the same ioctl() control channel that generic HDLC uses (i.e., using generic HDLC public ioctls in PC300 config program)

Alternatively, the additional protocol support could become a part of generic HDLC (rather than PC300 hw driver) and be made available to all hardware drivers.

I can, of course, make the PC300 compile after the change, but it will probably (at least partially) break it:

--- drivers/net/wan/pc300_drv.c
+++ drivers/net/wan/pc300_drv.c
#ifdef CONFIG_PC300_MLPPP
if (conf->proto != PC300_PROTO_MLPPP) {
- conf->proto = hdlc->proto.id;
+ conf->proto = /* FIXME hdlc->proto.id */ 0;
}
#else
- conf->proto = hdlc->proto.id;
+ conf->proto = /* FIXME hdlc->proto.id */ 0;
#endif

etc.

Personally I think it would be best if PC300 maintainer(s) changed their driver to not abuse internal HDLC variables and then the modularizing patch would be applied.

Any opinions/objections?

A working version of the patch (not intended to be included upstream yet) is attached.

Signature · 1 line
Krzysztof Halasa
drivers/net/wan/Kconfig +6 -6
index b5328b0..0e4c56f 100644
@@ -166,7 +166,7 @@ config HDLC
 	  If unsure, say N.
 
 config HDLC_RAW
-	bool "Raw HDLC support"
+	tristate "Raw HDLC support"
 	depends on HDLC
 	help
 	  Generic HDLC driver supporting raw HDLC over WAN connections.

@@ -174,7 +174,7 @@ config HDLC_RAW
 	  If unsure, say N.
 
 config HDLC_RAW_ETH
-	bool "Raw HDLC Ethernet device support"
+	tristate "Raw HDLC Ethernet device support"
 	depends on HDLC
 	help
 	  Generic HDLC driver supporting raw HDLC Ethernet device emulation

@@ -185,7 +185,7 @@ config HDLC_RAW_ETH
 	  If unsure, say N.
 
 config HDLC_CISCO
-	bool "Cisco HDLC support"
+	tristate "Cisco HDLC support"
 	depends on HDLC
 	help
 	  Generic HDLC driver supporting Cisco HDLC over WAN connections.

@@ -193,7 +193,7 @@ config HDLC_CISCO
 	  If unsure, say N.
 
 config HDLC_FR
-	bool "Frame Relay support"
+	tristate "Frame Relay support"
 	depends on HDLC
 	help
 	  Generic HDLC driver supporting Frame Relay over WAN connections.

@@ -201,7 +201,7 @@ config HDLC_FR
 	  If unsure, say N.
 
 config HDLC_PPP
-	bool "Synchronous Point-to-Point Protocol (PPP) support"
+	tristate "Synchronous Point-to-Point Protocol (PPP) support"
 	depends on HDLC
 	help
 	  Generic HDLC driver supporting PPP over WAN connections.

@@ -209,7 +209,7 @@ config HDLC_PPP
 	  If unsure, say N.
 
 config HDLC_X25
-	bool "X.25 protocol support"
+	tristate "X.25 protocol support"
 	depends on HDLC && (LAPB=m && HDLC=m || LAPB=y)
 	help
 	  Generic HDLC driver supporting X.25 over WAN connections.
drivers/net/wan/Makefile +7 -12
index 823c6d5..3a9e34c 100644
@@ -9,14 +9,13 @@ cyclomx-y                       := cycx_
 cyclomx-$(CONFIG_CYCLOMX_X25)	+= cycx_x25.o
 cyclomx-objs			:= $(cyclomx-y)  
 
-hdlc-y				:= hdlc_generic.o
-hdlc-$(CONFIG_HDLC_RAW)		+= hdlc_raw.o
-hdlc-$(CONFIG_HDLC_RAW_ETH)	+= hdlc_raw_eth.o
-hdlc-$(CONFIG_HDLC_CISCO)	+= hdlc_cisco.o
-hdlc-$(CONFIG_HDLC_FR)		+= hdlc_fr.o
-hdlc-$(CONFIG_HDLC_PPP)		+= hdlc_ppp.o
-hdlc-$(CONFIG_HDLC_X25)		+= hdlc_x25.o
-hdlc-objs			:= $(hdlc-y)
+obj-$(CONFIG_HDLC)		+= hdlc.o
+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_X25)		+= hdlc_x25.o
 
 pc300-y				:= pc300_drv.o
 pc300-$(CONFIG_PC300_MLPPP)	+= pc300_tty.o

@@ -39,10 +38,6 @@ obj-$(CONFIG_CYCLADES_SYNC)	+= cycx_drv.
 obj-$(CONFIG_LAPBETHER)		+= lapbether.o
 obj-$(CONFIG_SBNI)		+= sbni.o
 obj-$(CONFIG_PC300)		+= pc300.o
-obj-$(CONFIG_HDLC)		+= hdlc.o
-ifeq ($(CONFIG_HDLC_PPP),y)
-  obj-$(CONFIG_HDLC)		+= syncppp.o
-endif
 obj-$(CONFIG_N2)		+= n2.o
 obj-$(CONFIG_C101)		+= c101.o
 obj-$(CONFIG_WANXL)		+= wanxl.o
drivers/net/wan/hdlc.c renamed from drivers/net/wan/hdlc_generic.c +99 -70

276 lines not on this page · raw message

drivers/net/wan/hdlc_cisco.c +134 -64

343 lines not on this page · raw message

drivers/net/wan/hdlc_fr.c +237 -152

806 lines not on this page · raw message

drivers/net/wan/hdlc_ppp.c +61 -16

126 lines not on this page · raw message

drivers/net/wan/hdlc_raw.c +41 -9

88 lines not on this page · raw message

drivers/net/wan/hdlc_raw_eth.c +40 -9

87 lines not on this page · raw message

drivers/net/wan/hdlc_x25.c +41 -13

101 lines not on this page · raw message

drivers/net/wan/pc300_drv.c +14 -16
index a3e65d1..622c9aa 100644
@@ -2016,7 +2016,6 @@ static void sca_intr(pc300_t * card)
 			pc300ch_t *chan = &card->chan[ch];
 			pc300dev_t *d = &chan->d;
 			struct net_device *dev = d->dev;
-			hdlc_device *hdlc = dev_to_hdlc(dev);
 
 			spin_lock(&card->card_lock);
 

@@ -2049,8 +2048,8 @@ #endif
 							}
 							cpc_net_rx(dev);
 							/* Discard invalid frames */
-							hdlc->stats.rx_errors++;
-							hdlc->stats.rx_over_errors++;
+							hdlc_stats(dev)->rx_errors++;
+							hdlc_stats(dev)->rx_over_errors++;
 							chan->rx_first_bd = 0;
 							chan->rx_last_bd = N_DMA_RX_BUF - 1;
 							rx_dma_start(card, ch);

@@ -2116,8 +2115,8 @@ #endif
 										   card->hw.cpld_reg2) &
 								   ~ (CPLD_REG2_FALC_LED1 << (2 * ch)));
 						}
-						hdlc->stats.tx_errors++;
-						hdlc->stats.tx_fifo_errors++;
+						hdlc_stats(dev)->tx_errors++;
+						hdlc_stats(dev)->tx_fifo_errors++;
 						sca_tx_intr(d);
 					}
 				}

@@ -2534,7 +2533,6 @@ static int cpc_change_mtu(struct net_dev
 
 static int cpc_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 {
-	hdlc_device *hdlc = dev_to_hdlc(dev);
 	pc300dev_t *d = (pc300dev_t *) dev->priv;
 	pc300ch_t *chan = (pc300ch_t *) d->chan;
 	pc300_t *card = (pc300_t *) chan->card;

@@ -2552,10 +2550,10 @@ static int cpc_ioctl(struct net_device *
 		case SIOCGPC300CONF:
 #ifdef CONFIG_PC300_MLPPP
 			if (conf->proto != PC300_PROTO_MLPPP) {
-				conf->proto = hdlc->proto.id;
+				conf->proto = /* FIXME hdlc->proto.id */ 0;
 			}
 #else
-			conf->proto = hdlc->proto.id;
+			conf->proto = /* FIXME hdlc->proto.id */ 0;
 #endif
 			memcpy(&conf_aux.conf, conf, sizeof(pc300chconf_t));
 			memcpy(&conf_aux.hw, &card->hw, sizeof(pc300hw_t));

@@ -2588,12 +2586,12 @@ #ifdef CONFIG_PC300_MLPPP
 					}
 				} else {
 					memcpy(conf, &conf_aux.conf, sizeof(pc300chconf_t));
-					hdlc->proto.id = conf->proto;
+					/* FIXME hdlc->proto.id = conf->proto; */
 				}
 			}
 #else
 			memcpy(conf, &conf_aux.conf, sizeof(pc300chconf_t));
-			hdlc->proto.id = conf->proto;
+			/* FIXME hdlc->proto.id = conf->proto; */
 #endif
 			return 0;
 		case SIOCGPC300STATUS:

@@ -2606,7 +2604,7 @@ #endif
 		case SIOCGPC300UTILSTATS:
 			{
 				if (!arg) {	/* clear statistics */
-					memset(&hdlc->stats, 0, sizeof(struct net_device_stats));
+					memset(hdlc_stats(dev), 0, sizeof(struct net_device_stats));
 					if (card->hw.type == PC300_TE) {
 						memset(&chan->falc, 0, sizeof(falc_t));
 					}

@@ -2617,7 +2615,7 @@ #endif
 					pc300stats.hw_type = card->hw.type;
 					pc300stats.line_on = card->chan[ch].d.line_on;
 					pc300stats.line_off = card->chan[ch].d.line_off;
-					memcpy(&pc300stats.gen_stats, &hdlc->stats,
+					memcpy(&pc300stats.gen_stats, hdlc_stats(dev),
 					       sizeof(struct net_device_stats));
 					if (card->hw.type == PC300_TE)
 						memcpy(&pc300stats.te_stats,&chan->falc,sizeof(falc_t));

@@ -3147,7 +3145,6 @@ static void cpc_closech(pc300dev_t * d)
 
 int cpc_open(struct net_device *dev)
 {
-	hdlc_device *hdlc = dev_to_hdlc(dev);
 	pc300dev_t *d = (pc300dev_t *) dev->priv;
 	struct ifreq ifr;
 	int result;

@@ -3156,12 +3153,14 @@ #ifdef	PC300_DEBUG_OTHER
 	printk("pc300: cpc_open");
 #endif
 
+#ifdef FIXME
 	if (hdlc->proto.id == IF_PROTO_PPP) {
 		d->if_ptr = &hdlc->state.ppp.pppdev;
 	}
+#endif
 
 	result = hdlc_open(dev);
-	if (hdlc->proto.id == IF_PROTO_PPP) {
+	if (/* FIXME hdlc->proto.id == IF_PROTO_PPP*/ 0) {
 		dev->priv = d;
 	}
 	if (result) {

@@ -3176,7 +3175,6 @@ #endif
 
 static int cpc_close(struct net_device *dev)
 {
-	hdlc_device *hdlc = dev_to_hdlc(dev);
 	pc300dev_t *d = (pc300dev_t *) dev->priv;
 	pc300ch_t *chan = (pc300ch_t *) d->chan;
 	pc300_t *card = (pc300_t *) chan->card;

@@ -3193,7 +3191,7 @@ #endif
 	CPC_UNLOCK(card, flags);
 
 	hdlc_close(dev);
-	if (hdlc->proto.id == IF_PROTO_PPP) {
+	if (/* FIXME hdlc->proto.id == IF_PROTO_PPP*/ 0) {
 		d->if_ptr = NULL;
 	}
 #ifdef CONFIG_PC300_MLPPP
include/linux/hdlc.h +44 -157

264 lines not on this page · raw message

include/linux/hdlc/ioctl.h +33 -0
index 78430ba..5839723 100644
@@ -1,6 +1,39 @@
 #ifndef __HDLC_IOCTL_H__
 #define __HDLC_IOCTL_H__
 
+
+#define GENERIC_HDLC_VERSION 4	/* For synchronization with sethdlc utility */
+
+#define CLOCK_DEFAULT   0	/* Default setting */
+#define CLOCK_EXT	1	/* External TX and RX clock - DTE */
+#define CLOCK_INT	2	/* Internal TX and RX clock - DCE */
+#define CLOCK_TXINT	3	/* Internal TX and external RX clock */
+#define CLOCK_TXFROMRX	4	/* TX clock derived from external RX clock */
+
+
+#define ENCODING_DEFAULT	0 /* Default setting */
+#define ENCODING_NRZ		1
+#define ENCODING_NRZI		2
+#define ENCODING_FM_MARK	3
+#define ENCODING_FM_SPACE	4
+#define ENCODING_MANCHESTER	5
+
+
+#define PARITY_DEFAULT		0 /* Default setting */
+#define PARITY_NONE		1 /* No parity */
+#define PARITY_CRC16_PR0	2 /* CRC16, initial value 0x0000 */
+#define PARITY_CRC16_PR1	3 /* CRC16, initial value 0xFFFF */
+#define PARITY_CRC16_PR0_CCITT	4 /* CRC16, initial 0x0000, ITU-T version */
+#define PARITY_CRC16_PR1_CCITT	5 /* CRC16, initial 0xFFFF, ITU-T version */
+#define PARITY_CRC32_PR0_CCITT	6 /* CRC32, initial value 0x00000000 */
+#define PARITY_CRC32_PR1_CCITT	7 /* CRC32, initial value 0xFFFFFFFF */
+
+#define LMI_DEFAULT		0 /* Default setting */
+#define LMI_NONE		1 /* No LMI, all PVCs are static */
+#define LMI_ANSI		2 /* ANSI Annex D */
+#define LMI_CCITT		3 /* ITU-T Annex A */
+#define LMI_CISCO		4 /* The "original" LMI, aka Gang of Four */
+
 typedef struct { 
 	unsigned int clock_rate; /* bits per second */
 	unsigned int clock_type; /* internal, external, TX-internal etc. */