[PATCH net-next v2 0/6] netdev event handling + neighbour config

[PATCH net-next v2 0/6] netdev event handling + neighbour config

[Martin Schiller]

Martin Schiller (6):
  net/x25: handle additional netdev events
  net/x25: make neighbour params configurable
  net/x25: replace x25_kill_by_device with x25_kill_by_neigh
  net/x25: support NETDEV_CHANGE notifier
  net/lapb: support netdev events
  net/lapb: fix t1 timer handling

 include/net/x25.h        |  10 +-
 include/uapi/linux/x25.h |  56 ++++++-----
 net/lapb/lapb_iface.c    |  83 ++++++++++++++++
 net/lapb/lapb_timer.c    |  11 ++-
 net/x25/af_x25.c         | 206 +++++++++++++++++++++++++++++++--------
 net/x25/x25_facilities.c |   6 +-
 net/x25/x25_link.c       | 142 +++++++++++++++++++++++----
 net/x25/x25_subr.c       |  22 ++++-
 8 files changed, 445 insertions(+), 91 deletions(-)

-- 
2.20.1

[PATCH net-next v2 1/6] net/x25: handle additional netdev events

[Martin Schiller]

Add / remove x25_link_device by NETDEV_REGISTER/UNREGISTER and also by
NETDEV_POST_TYPE_CHANGE/NETDEV_PRE_TYPE_CHANGE.

This change is needed so that the x25_neigh struct for an interface is
already created when it shows up and is kept independently if the
interface goes UP or DOWN.

This is used in the next commit, where x25 params of an neighbour will
get configurable through ioctls.

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---

Change from v1:
fix 'subject_prefix' and 'checkpatch' warnings

---
 include/net/x25.h  |  2 ++
 net/x25/af_x25.c   | 26 ++++++++++++++++++++++++++
 net/x25/x25_link.c | 45 +++++++++++++++++++++++++++++++++++++++------
 3 files changed, 67 insertions(+), 6 deletions(-)

diff --git a/include/net/x25.h b/include/net/x25.h
index d7d6c2b4ffa7..4c1502e8b2b2 100644
--- a/include/net/x25.h
+++ b/include/net/x25.h
@@ -231,6 +231,8 @@ int x25_backlog_rcv(struct sock *, struct sk_buff *);
 
 /* x25_link.c */
 void x25_link_control(struct sk_buff *, struct x25_neigh *, unsigned short);
+void x25_link_device_add(struct net_device *dev);
+void x25_link_device_remove(struct net_device *dev);
 void x25_link_device_up(struct net_device *);
 void x25_link_device_down(struct net_device *);
 void x25_link_established(struct x25_neigh *);
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 046d3fee66a9..d2a52c254cca 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -233,10 +233,24 @@ static int x25_device_event(struct notifier_block *this, unsigned long event,
 #endif
 	 ) {
 		switch (event) {
+		case NETDEV_REGISTER:
+			pr_debug("X.25: got event NETDEV_REGISTER for device: %s\n",
+				 dev->name);
+			x25_link_device_add(dev);
+			break;
+		case NETDEV_POST_TYPE_CHANGE:
+			pr_debug("X.25: got event NETDEV_POST_TYPE_CHANGE for device: %s\n",
+				 dev->name);
+			x25_link_device_add(dev);
+			break;
 		case NETDEV_UP:
+			pr_debug("X.25: got event NETDEV_UP for device: %s\n",
+				 dev->name);
 			x25_link_device_up(dev);
 			break;
 		case NETDEV_GOING_DOWN:
+			pr_debug("X.25: got event NETDEV_GOING_DOWN for device: %s\n",
+				 dev->name);
 			nb = x25_get_neigh(dev);
 			if (nb) {
 				x25_terminate_link(nb);
@@ -244,10 +258,22 @@ static int x25_device_event(struct notifier_block *this, unsigned long event,
 			}
 			break;
 		case NETDEV_DOWN:
+			pr_debug("X.25: got event NETDEV_DOWN for device: %s\n",
+				 dev->name);
 			x25_kill_by_device(dev);
 			x25_route_device_down(dev);
 			x25_link_device_down(dev);
 			break;
+		case NETDEV_PRE_TYPE_CHANGE:
+			pr_debug("X.25: got event NETDEV_PRE_TYPE_CHANGE for device: %s\n",
+				 dev->name);
+			x25_link_device_remove(dev);
+			break;
+		case NETDEV_UNREGISTER:
+			pr_debug("X.25: got event NETDEV_UNREGISTER for device: %s\n",
+				 dev->name);
+			x25_link_device_remove(dev);
+			break;
 		}
 	}
 
diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
index fdae054b7dc1..92828a8a4ada 100644
--- a/net/x25/x25_link.c
+++ b/net/x25/x25_link.c
@@ -239,9 +239,17 @@ void x25_link_terminated(struct x25_neigh *nb)
 /*
  *	Add a new device.
  */
-void x25_link_device_up(struct net_device *dev)
+void x25_link_device_add(struct net_device *dev)
 {
-	struct x25_neigh *nb = kmalloc(sizeof(*nb), GFP_ATOMIC);
+	struct x25_neigh *nb = x25_get_neigh(dev);
+
+	/* Check, if we already have a neighbour for this device */
+	if (nb) {
+		x25_neigh_put(nb);
+		return;
+	}
+
+	nb = kmalloc(sizeof(*nb), GFP_ATOMIC);
 
 	if (!nb)
 		return;
@@ -268,6 +276,20 @@ void x25_link_device_up(struct net_device *dev)
 	write_unlock_bh(&x25_neigh_list_lock);
 }
 
+/* A device is coming up */
+void x25_link_device_up(struct net_device *dev)
+{
+	struct x25_neigh *nb = x25_get_neigh(dev);
+
+	if (!nb)
+		return;
+
+	nb->state = X25_LINK_STATE_1;
+	x25_establish_link(nb);
+
+	x25_neigh_put(nb);
+}
+
 /**
  *	__x25_remove_neigh - remove neighbour from x25_neigh_list
  *	@nb: - neigh to remove
@@ -277,9 +299,6 @@ void x25_link_device_up(struct net_device *dev)
  */
 static void __x25_remove_neigh(struct x25_neigh *nb)
 {
-	skb_queue_purge(&nb->queue);
-	x25_stop_t20timer(nb);
-
 	if (nb->node.next) {
 		list_del(&nb->node);
 		x25_neigh_put(nb);
@@ -289,7 +308,7 @@ static void __x25_remove_neigh(struct x25_neigh *nb)
 /*
  *	A device has been removed, remove its links.
  */
-void x25_link_device_down(struct net_device *dev)
+void x25_link_device_remove(struct net_device *dev)
 {
 	struct x25_neigh *nb;
 	struct list_head *entry, *tmp;
@@ -308,6 +327,20 @@ void x25_link_device_down(struct net_device *dev)
 	write_unlock_bh(&x25_neigh_list_lock);
 }
 
+/* A device is going down */
+void x25_link_device_down(struct net_device *dev)
+{
+	struct x25_neigh *nb = x25_get_neigh(dev);
+
+	if (!nb)
+		return;
+
+	skb_queue_purge(&nb->queue);
+	x25_stop_t20timer(nb);
+
+	x25_neigh_put(nb);
+}
+
 /*
  *	Given a device, return the neighbour address.
  */
-- 
2.20.1

[PATCH net-next v2 2/6] net/x25: make neighbour params configurable

[Martin Schiller]

Extended struct x25_neigh and x25_subscrip_struct to configure following
params through SIOCX25SSUBSCRIP:
  o mode (DTE/DCE)
  o number of channels
  o facilities (packet size, window size)
  o timer T20

Based on this configuration options the following changes/extensions
where made:
  o DTE/DCE handling to select the next lc (DCE=from bottom / DTE=from
    top)
  o DTE/DCE handling to set correct clear/reset/restart cause
  o take default facilities from neighbour settings

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---

Change from v1:
o fix 'subject_prefix' and 'checkpatch' warnings
o fix incompatible assignment of 'struct compat_x25_facilities'

---
 include/net/x25.h        |   8 ++-
 include/uapi/linux/x25.h |  56 ++++++++-------
 net/x25/af_x25.c         | 145 ++++++++++++++++++++++++++++++++-------
 net/x25/x25_facilities.c |   6 +-
 net/x25/x25_link.c       |  97 ++++++++++++++++++++++----
 net/x25/x25_subr.c       |  22 +++++-
 6 files changed, 268 insertions(+), 66 deletions(-)

diff --git a/include/net/x25.h b/include/net/x25.h
index 4c1502e8b2b2..ec00f595fcc6 100644
--- a/include/net/x25.h
+++ b/include/net/x25.h
@@ -140,6 +140,9 @@ struct x25_neigh {
 	struct net_device	*dev;
 	unsigned int		state;
 	unsigned int		extended;
+	unsigned int		dce;
+	unsigned int		lc;
+	struct x25_facilities	facilities;
 	struct sk_buff_head	queue;
 	unsigned long		t20;
 	struct timer_list	t20timer;
@@ -164,6 +167,8 @@ struct x25_sock {
 	struct timer_list	timer;
 	struct x25_causediag	causediag;
 	struct x25_facilities	facilities;
+	/* set, if facilities changed by SIOCX25SFACILITIES */
+	unsigned int		socket_defined_facilities;
 	struct x25_dte_facilities dte_facilities;
 	struct x25_calluserdata	calluserdata;
 	unsigned long 		vc_facil_mask;	/* inc_call facilities mask */
@@ -215,7 +220,8 @@ int x25_create_facilities(unsigned char *, struct x25_facilities *,
 			  struct x25_dte_facilities *, unsigned long);
 int x25_negotiate_facilities(struct sk_buff *, struct sock *,
 			     struct x25_facilities *,
-			     struct x25_dte_facilities *);
+				struct x25_dte_facilities *,
+				struct x25_neigh *);
 void x25_limit_facilities(struct x25_facilities *, struct x25_neigh *);
 
 /* x25_forward.c */
diff --git a/include/uapi/linux/x25.h b/include/uapi/linux/x25.h
index 034b7dc5593a..094dc2cff37b 100644
--- a/include/uapi/linux/x25.h
+++ b/include/uapi/linux/x25.h
@@ -63,31 +63,6 @@ struct sockaddr_x25 {
 	struct x25_address sx25_addr;		/* X.121 Address */
 };
 
-/*
- *	DTE/DCE subscription options.
- *
- *      As this is missing lots of options, user should expect major
- *	changes of this structure in 2.5.x which might break compatibilty.
- *      The somewhat ugly dimension 200-sizeof() is needed to maintain
- *	backward compatibility.
- */
-struct x25_subscrip_struct {
-	char device[200-sizeof(unsigned long)];
-	unsigned long	global_facil_mask;	/* 0 to disable negotiation */
-	unsigned int	extended;
-};
-
-/* values for above global_facil_mask */
-
-#define	X25_MASK_REVERSE	0x01	
-#define	X25_MASK_THROUGHPUT	0x02
-#define	X25_MASK_PACKET_SIZE	0x04
-#define	X25_MASK_WINDOW_SIZE	0x08
-
-#define X25_MASK_CALLING_AE 0x10
-#define X25_MASK_CALLED_AE 0x20
-
-
 /*
  *	Routing table control structure.
  */
@@ -127,6 +102,37 @@ struct x25_dte_facilities {
 	__u8 called_ae[20];
 };
 
+/*
+ *	DTE/DCE subscription options.
+ *
+ *      As this is missing lots of options, user should expect major
+ *	changes of this structure in 2.5.x which might break compatibility.
+ *      The somewhat ugly dimension 200-sizeof() is needed to maintain
+ *	backward compatibility.
+ */
+struct x25_subscrip_struct {
+	char device[200 - ((2 * sizeof(unsigned long)) +
+		    sizeof(struct x25_facilities) +
+		    (2 * sizeof(unsigned int)))];
+	unsigned int		dce;
+	unsigned int		lc;
+	struct x25_facilities	facilities;
+	unsigned long		t20;
+	unsigned long		global_facil_mask;	/* 0 to disable negotiation */
+	unsigned int		extended;
+};
+
+/* values for above global_facil_mask */
+
+#define	X25_MASK_REVERSE	0x01
+#define	X25_MASK_THROUGHPUT	0x02
+#define	X25_MASK_PACKET_SIZE	0x04
+#define	X25_MASK_WINDOW_SIZE	0x08
+
+#define X25_MASK_CALLING_AE 0x10
+#define X25_MASK_CALLED_AE 0x20
+
+
 /*
  *	Call User Data structure.
  */
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index d2a52c254cca..4c2a395fdbdb 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -72,8 +72,21 @@ static const struct proto_ops x25_proto_ops;
 static const struct x25_address null_x25_address = {"               "};
 
 #ifdef CONFIG_COMPAT
+struct compat_x25_facilities {
+	compat_uint_t	winsize_in, winsize_out;
+	compat_uint_t	pacsize_in, pacsize_out;
+	compat_uint_t	throughput;
+	compat_uint_t	reverse;
+};
+
 struct compat_x25_subscrip_struct {
-	char device[200-sizeof(compat_ulong_t)];
+	char device[200 - ((2 * sizeof(compat_ulong_t)) +
+		    sizeof(struct compat_x25_facilities) +
+		    (2 * sizeof(compat_uint_t)))];
+	compat_uint_t		dce;
+	compat_uint_t		lc;
+	struct compat_x25_facilities	facilities;
+	compat_ulong_t		t20;
 	compat_ulong_t global_facil_mask;
 	compat_uint_t extended;
 };
@@ -373,13 +386,26 @@ static unsigned int x25_new_lci(struct x25_neigh *nb)
 	unsigned int lci = 1;
 	struct sock *sk;
 
-	while ((sk = x25_find_socket(lci, nb)) != NULL) {
-		sock_put(sk);
-		if (++lci == 4096) {
-			lci = 0;
-			break;
+	if (nb->dce) {
+		while ((sk = x25_find_socket(lci, nb)) != NULL) {
+			sock_put(sk);
+			if (++lci > nb->lc) {
+				lci = 0;
+				break;
+			}
+			cond_resched();
+		}
+	} else {
+		lci = nb->lc;
+
+		while ((sk = x25_find_socket(lci, nb)) != NULL) {
+			sock_put(sk);
+			if (--lci == 0) {
+				lci = 0;
+				break;
+			}
+			cond_resched();
 		}
-		cond_resched();
 	}
 
 	return lci;
@@ -813,6 +839,10 @@ static int x25_connect(struct socket *sock, struct sockaddr *uaddr,
 	if (!x25->neighbour)
 		goto out_put_route;
 
+	if (!x25->socket_defined_facilities)
+		memcpy(&x25->facilities, &x25->neighbour->facilities,
+		       sizeof(struct x25_facilities));
+
 	x25_limit_facilities(&x25->facilities, x25->neighbour);
 
 	x25->lci = x25_new_lci(x25->neighbour);
@@ -1046,7 +1076,7 @@ int x25_rx_call_request(struct sk_buff *skb, struct x25_neigh *nb,
 	/*
 	 *	Try to reach a compromise on the requested facilities.
 	 */
-	len = x25_negotiate_facilities(skb, sk, &facilities, &dte_facilities);
+	len = x25_negotiate_facilities(skb, sk, &facilities, &dte_facilities, nb);
 	if (len == -1)
 		goto out_sock_put;
 
@@ -1460,10 +1490,15 @@ static int x25_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 		rc = x25_subscr_ioctl(cmd, argp);
 		break;
 	case SIOCX25GFACILITIES: {
+		rc = -EINVAL;
 		lock_sock(sk);
+		if (sk->sk_state != TCP_ESTABLISHED &&
+		    !x25->socket_defined_facilities)
+			goto out_gfac_release;
 		rc = copy_to_user(argp, &x25->facilities,
 				  sizeof(x25->facilities))
 			? -EFAULT : 0;
+out_gfac_release:
 		release_sock(sk);
 		break;
 	}
@@ -1477,16 +1512,16 @@ static int x25_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 		lock_sock(sk);
 		if (sk->sk_state != TCP_LISTEN &&
 		    sk->sk_state != TCP_CLOSE)
-			goto out_fac_release;
+			goto out_sfac_release;
 		if (facilities.pacsize_in < X25_PS16 ||
 		    facilities.pacsize_in > X25_PS4096)
-			goto out_fac_release;
+			goto out_sfac_release;
 		if (facilities.pacsize_out < X25_PS16 ||
 		    facilities.pacsize_out > X25_PS4096)
-			goto out_fac_release;
+			goto out_sfac_release;
 		if (facilities.winsize_in < 1 ||
 		    facilities.winsize_in > 127)
-			goto out_fac_release;
+			goto out_sfac_release;
 		if (facilities.throughput) {
 			int out = facilities.throughput & 0xf0;
 			int in  = facilities.throughput & 0x0f;
@@ -1494,19 +1529,20 @@ static int x25_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 				facilities.throughput |=
 					X25_DEFAULT_THROUGHPUT << 4;
 			else if (out < 0x30 || out > 0xD0)
-				goto out_fac_release;
+				goto out_sfac_release;
 			if (!in)
 				facilities.throughput |=
 					X25_DEFAULT_THROUGHPUT;
 			else if (in < 0x03 || in > 0x0D)
-				goto out_fac_release;
+				goto out_sfac_release;
 		}
 		if (facilities.reverse &&
 		    (facilities.reverse & 0x81) != 0x81)
-			goto out_fac_release;
+			goto out_sfac_release;
 		x25->facilities = facilities;
+		x25->socket_defined_facilities = 1;
 		rc = 0;
-out_fac_release:
+out_sfac_release:
 		release_sock(sk);
 		break;
 	}
@@ -1658,6 +1694,9 @@ static int compat_x25_subscr_ioctl(unsigned int cmd,
 	struct net_device *dev;
 	int rc = -EINVAL;
 
+	if (cmd != SIOCX25GSUBSCRIP && cmd != SIOCX25SSUBSCRIP)
+		goto out;
+
 	rc = -EFAULT;
 	if (copy_from_user(&x25_subscr, x25_subscr32, sizeof(*x25_subscr32)))
 		goto out;
@@ -1671,28 +1710,86 @@ static int compat_x25_subscr_ioctl(unsigned int cmd,
 	if (nb == NULL)
 		goto out_dev_put;
 
-	dev_put(dev);
-
 	if (cmd == SIOCX25GSUBSCRIP) {
 		read_lock_bh(&x25_neigh_list_lock);
 		x25_subscr.extended = nb->extended;
+		x25_subscr.dce		     = nb->dce;
+		x25_subscr.lc		     = nb->lc;
+		x25_subscr.facilities.winsize_in = nb->facilities.winsize_in;
+		x25_subscr.facilities.winsize_out = nb->facilities.winsize_out;
+		x25_subscr.facilities.pacsize_in = nb->facilities.pacsize_in;
+		x25_subscr.facilities.pacsize_out = nb->facilities.pacsize_out;
+		x25_subscr.facilities.throughput = nb->facilities.throughput;
+		x25_subscr.facilities.reverse = nb->facilities.reverse;
+		x25_subscr.t20		     = nb->t20;
 		x25_subscr.global_facil_mask = nb->global_facil_mask;
 		read_unlock_bh(&x25_neigh_list_lock);
 		rc = copy_to_user(x25_subscr32, &x25_subscr,
 				sizeof(*x25_subscr32)) ? -EFAULT : 0;
 	} else {
 		rc = -EINVAL;
-		if (x25_subscr.extended == 0 || x25_subscr.extended == 1) {
-			rc = 0;
-			write_lock_bh(&x25_neigh_list_lock);
-			nb->extended = x25_subscr.extended;
-			nb->global_facil_mask = x25_subscr.global_facil_mask;
-			write_unlock_bh(&x25_neigh_list_lock);
+
+		if (dev->flags & IFF_UP)
+			return -EBUSY;
+
+		if (x25_subscr.extended != 0 && x25_subscr.extended != 1)
+			goto out_dev_and_neigh_put;
+		if (x25_subscr.dce != 0 && x25_subscr.dce != 1)
+			goto out_dev_and_neigh_put;
+		if (x25_subscr.lc < 1 || x25_subscr.lc > 4095)
+			goto out_dev_and_neigh_put;
+		if (x25_subscr.facilities.pacsize_in < X25_PS16 ||
+		    x25_subscr.facilities.pacsize_in > X25_PS4096)
+			goto out_dev_and_neigh_put;
+		if (x25_subscr.facilities.pacsize_out < X25_PS16 ||
+		    x25_subscr.facilities.pacsize_out > X25_PS4096)
+			goto out_dev_and_neigh_put;
+		if (x25_subscr.facilities.winsize_in < 1 ||
+		    x25_subscr.facilities.winsize_in > 127)
+			goto out_dev_and_neigh_put;
+		if (x25_subscr.facilities.throughput) {
+			int out = x25_subscr.facilities.throughput & 0xf0;
+			int in  = x25_subscr.facilities.throughput & 0x0f;
+
+			if (!out)
+				x25_subscr.facilities.throughput |=
+					X25_DEFAULT_THROUGHPUT << 4;
+			else if (out < 0x30 || out > 0xD0)
+				goto out_dev_and_neigh_put;
+			if (!in)
+				x25_subscr.facilities.throughput |=
+					X25_DEFAULT_THROUGHPUT;
+			else if (in < 0x03 || in > 0x0D)
+				goto out_dev_and_neigh_put;
 		}
+		if (x25_subscr.facilities.reverse &&
+		    (x25_subscr.facilities.reverse & 0x81) != 0x81)
+			goto out_dev_and_neigh_put;
+		if (x25_subscr.t20 < 1 * HZ || x25_subscr.t20 > 300 * HZ)
+			goto out_dev_and_neigh_put;
+
+		rc = 0;
+		write_lock_bh(&x25_neigh_list_lock);
+		nb->extended	      = x25_subscr.extended;
+		nb->dce		      = x25_subscr.dce;
+		nb->lc		      = x25_subscr.lc;
+		nb->facilities.winsize_in = x25_subscr.facilities.winsize_in;
+		nb->facilities.winsize_out = x25_subscr.facilities.winsize_out;
+		nb->facilities.pacsize_in = x25_subscr.facilities.pacsize_in;
+		nb->facilities.pacsize_out = x25_subscr.facilities.pacsize_out;
+		nb->facilities.throughput = x25_subscr.facilities.throughput;
+		nb->facilities.reverse = x25_subscr.facilities.reverse;
+		nb->t20		      = x25_subscr.t20;
+		nb->global_facil_mask = x25_subscr.global_facil_mask;
+		write_unlock_bh(&x25_neigh_list_lock);
 	}
+	dev_put(dev);
+
 	x25_neigh_put(nb);
 out:
 	return rc;
+out_dev_and_neigh_put:
+	x25_neigh_put(nb);
 out_dev_put:
 	dev_put(dev);
 	goto out;
diff --git a/net/x25/x25_facilities.c b/net/x25/x25_facilities.c
index 8e1a49b0c0dc..e6c9f9376206 100644
--- a/net/x25/x25_facilities.c
+++ b/net/x25/x25_facilities.c
@@ -263,13 +263,17 @@ int x25_create_facilities(unsigned char *buffer,
  *	The only real problem is with reverse charging.
  */
 int x25_negotiate_facilities(struct sk_buff *skb, struct sock *sk,
-		struct x25_facilities *new, struct x25_dte_facilities *dte)
+		struct x25_facilities *new, struct x25_dte_facilities *dte,
+		struct x25_neigh *nb)
 {
 	struct x25_sock *x25 = x25_sk(sk);
 	struct x25_facilities *ours = &x25->facilities;
 	struct x25_facilities theirs;
 	int len;
 
+	if (!x25->socket_defined_facilities)
+		ours = &nb->facilities;
+
 	memset(&theirs, 0, sizeof(theirs));
 	memcpy(new, ours, sizeof(*new));
 	memset(dte, 0, sizeof(*dte));
diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
index 92828a8a4ada..2af50d585b4b 100644
--- a/net/x25/x25_link.c
+++ b/net/x25/x25_link.c
@@ -125,8 +125,16 @@ static void x25_transmit_restart_request(struct x25_neigh *nb)
 	*dptr++ = nb->extended ? X25_GFI_EXTSEQ : X25_GFI_STDSEQ;
 	*dptr++ = 0x00;
 	*dptr++ = X25_RESTART_REQUEST;
-	*dptr++ = 0x00;
-	*dptr++ = 0;
+
+	*dptr = 0x00;	/* cause */
+
+	/* set bit 8, if DTE and cause != 0x00 */
+	if (!nb->dce && *dptr != 0x00)
+		*dptr |= (unsigned char)0x80;
+
+	dptr++;
+
+	*dptr++ = 0x00;	/* diagnostic */
 
 	skb->sk = NULL;
 
@@ -181,8 +189,16 @@ void x25_transmit_clear_request(struct x25_neigh *nb, unsigned int lci,
 					 X25_GFI_STDSEQ);
 	*dptr++ = (lci >> 0) & 0xFF;
 	*dptr++ = X25_CLEAR_REQUEST;
-	*dptr++ = cause;
-	*dptr++ = 0x00;
+
+	*dptr = cause;	/* cause */
+
+	/* set bit 8, if DTE and cause != 0x00 */
+	if (!nb->dce && *dptr != 0x00)
+		*dptr |= (unsigned char)0x80;
+
+	dptr++;
+
+	*dptr++ = 0x00;	/* diagnostic */
 
 	skb->sk = NULL;
 
@@ -261,6 +277,15 @@ void x25_link_device_add(struct net_device *dev)
 	nb->dev      = dev;
 	nb->state    = X25_LINK_STATE_0;
 	nb->extended = 0;
+	nb->dce      = 0;
+	nb->lc       = 10;
+	nb->facilities.winsize_in  = X25_DEFAULT_WINDOW_SIZE;
+	nb->facilities.winsize_out = X25_DEFAULT_WINDOW_SIZE;
+	nb->facilities.pacsize_in  = X25_DEFAULT_PACKET_SIZE;
+	nb->facilities.pacsize_out = X25_DEFAULT_PACKET_SIZE;
+	/* by default don't negotiate throughput */
+	nb->facilities.throughput  = 0;
+	nb->facilities.reverse     = X25_DEFAULT_REVERSE;
 	/*
 	 * Enables negotiation
 	 */
@@ -389,28 +414,76 @@ int x25_subscr_ioctl(unsigned int cmd, void __user *arg)
 	if ((nb = x25_get_neigh(dev)) == NULL)
 		goto out_dev_put;
 
-	dev_put(dev);
-
 	if (cmd == SIOCX25GSUBSCRIP) {
 		read_lock_bh(&x25_neigh_list_lock);
 		x25_subscr.extended	     = nb->extended;
+		x25_subscr.dce		     = nb->dce;
+		x25_subscr.lc		     = nb->lc;
+		x25_subscr.facilities	     = nb->facilities;
+		x25_subscr.t20		     = nb->t20;
 		x25_subscr.global_facil_mask = nb->global_facil_mask;
 		read_unlock_bh(&x25_neigh_list_lock);
 		rc = copy_to_user(arg, &x25_subscr,
 				  sizeof(x25_subscr)) ? -EFAULT : 0;
 	} else {
 		rc = -EINVAL;
-		if (!(x25_subscr.extended && x25_subscr.extended != 1)) {
-			rc = 0;
-			write_lock_bh(&x25_neigh_list_lock);
-			nb->extended	     = x25_subscr.extended;
-			nb->global_facil_mask = x25_subscr.global_facil_mask;
-			write_unlock_bh(&x25_neigh_list_lock);
+
+		if (dev->flags & IFF_UP)
+			return -EBUSY;
+
+		if (x25_subscr.extended != 0 && x25_subscr.extended != 1)
+			goto out_dev_and_neigh_put;
+		if (x25_subscr.dce != 0 && x25_subscr.dce != 1)
+			goto out_dev_and_neigh_put;
+		if (x25_subscr.lc < 1 || x25_subscr.lc > 4095)
+			goto out_dev_and_neigh_put;
+		if (x25_subscr.facilities.pacsize_in < X25_PS16 ||
+		    x25_subscr.facilities.pacsize_in > X25_PS4096)
+			goto out_dev_and_neigh_put;
+		if (x25_subscr.facilities.pacsize_out < X25_PS16 ||
+		    x25_subscr.facilities.pacsize_out > X25_PS4096)
+			goto out_dev_and_neigh_put;
+		if (x25_subscr.facilities.winsize_in < 1 ||
+		    x25_subscr.facilities.winsize_in > 127)
+			goto out_dev_and_neigh_put;
+		if (x25_subscr.facilities.throughput) {
+			int out = x25_subscr.facilities.throughput & 0xf0;
+			int in  = x25_subscr.facilities.throughput & 0x0f;
+
+			if (!out)
+				x25_subscr.facilities.throughput |=
+					X25_DEFAULT_THROUGHPUT << 4;
+			else if (out < 0x30 || out > 0xD0)
+				goto out_dev_and_neigh_put;
+			if (!in)
+				x25_subscr.facilities.throughput |=
+					X25_DEFAULT_THROUGHPUT;
+			else if (in < 0x03 || in > 0x0D)
+				goto out_dev_and_neigh_put;
 		}
+		if (x25_subscr.facilities.reverse &&
+		    (x25_subscr.facilities.reverse & 0x81) != 0x81)
+			goto out_dev_and_neigh_put;
+		if (x25_subscr.t20 < 1 * HZ || x25_subscr.t20 > 300 * HZ)
+			goto out_dev_and_neigh_put;
+
+		rc = 0;
+		write_lock_bh(&x25_neigh_list_lock);
+		nb->extended	      = x25_subscr.extended;
+		nb->dce		      = x25_subscr.dce;
+		nb->lc		      = x25_subscr.lc;
+		nb->facilities	      = x25_subscr.facilities;
+		nb->t20		      = x25_subscr.t20;
+		nb->global_facil_mask = x25_subscr.global_facil_mask;
+		write_unlock_bh(&x25_neigh_list_lock);
 	}
+	dev_put(dev);
+
 	x25_neigh_put(nb);
 out:
 	return rc;
+out_dev_and_neigh_put:
+	x25_neigh_put(nb);
 out_dev_put:
 	dev_put(dev);
 	goto out;
diff --git a/net/x25/x25_subr.c b/net/x25/x25_subr.c
index 0285aaa1e93c..c195d1c89ad7 100644
--- a/net/x25/x25_subr.c
+++ b/net/x25/x25_subr.c
@@ -218,15 +218,31 @@ void x25_write_internal(struct sock *sk, int frametype)
 		case X25_CLEAR_REQUEST:
 			dptr    = skb_put(skb, 3);
 			*dptr++ = frametype;
-			*dptr++ = x25->causediag.cause;
+
+			*dptr = x25->causediag.cause;
+
+			/* set bit 8, if DTE and cause != 0x00 */
+			if (!x25->neighbour->dce && *dptr != 0x00)
+				*dptr |= (unsigned char)0x80;
+
+			dptr++;
+
 			*dptr++ = x25->causediag.diagnostic;
 			break;
 
 		case X25_RESET_REQUEST:
 			dptr    = skb_put(skb, 3);
 			*dptr++ = frametype;
-			*dptr++ = 0x00;		/* XXX */
-			*dptr++ = 0x00;		/* XXX */
+
+			*dptr = 0x00;	/* cause */
+
+			/* set bit 8, if DTE and cause != 0x00 */
+			if (!x25->neighbour->dce && *dptr != 0x00)
+				*dptr |= (unsigned char)0x80;
+
+			dptr++;
+
+			*dptr++ = 0x00;	/* diagnostic */
 			break;
 
 		case X25_RR:
-- 
2.20.1

[PATCH net-next v2 3/6] net/x25: replace x25_kill_by_device with x25_kill_by_neigh

[Martin Schiller]

Remove unnecessary function x25_kill_by_device.

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---

Change from v1:
fix 'subject_prefix' warning

---
 net/x25/af_x25.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 4c2a395fdbdb..25726120fcc7 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -212,22 +212,6 @@ static void x25_remove_socket(struct sock *sk)
 	write_unlock_bh(&x25_list_lock);
 }
 
-/*
- *	Kill all bound sockets on a dropped device.
- */
-static void x25_kill_by_device(struct net_device *dev)
-{
-	struct sock *s;
-
-	write_lock_bh(&x25_list_lock);
-
-	sk_for_each(s, &x25_list)
-		if (x25_sk(s)->neighbour && x25_sk(s)->neighbour->dev == dev)
-			x25_disconnect(s, ENETUNREACH, 0, 0);
-
-	write_unlock_bh(&x25_list_lock);
-}
-
 /*
  *	Handle device status changes.
  */
@@ -273,7 +257,11 @@ static int x25_device_event(struct notifier_block *this, unsigned long event,
 		case NETDEV_DOWN:
 			pr_debug("X.25: got event NETDEV_DOWN for device: %s\n",
 				 dev->name);
-			x25_kill_by_device(dev);
+			nb = x25_get_neigh(dev);
+			if (nb) {
+				x25_kill_by_neigh(nb);
+				x25_neigh_put(nb);
+			}
 			x25_route_device_down(dev);
 			x25_link_device_down(dev);
 			break;
-- 
2.20.1

[PATCH net-next v2 4/6] net/x25: support NETDEV_CHANGE notifier

[Martin Schiller]

This makes it possible to handle carrier lost and detection.
In case of carrier lost, we shutdown layer 3 and flush all sessions.

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---

Change from v1:
fix 'subject_prefix' and 'checkpatch' warnings

---
 net/x25/af_x25.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 25726120fcc7..6a95ca11694e 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -275,6 +275,19 @@ static int x25_device_event(struct notifier_block *this, unsigned long event,
 				 dev->name);
 			x25_link_device_remove(dev);
 			break;
+		case NETDEV_CHANGE:
+			pr_debug("X.25: got event NETDEV_CHANGE for device: %s\n",
+				 dev->name);
+			if (!netif_carrier_ok(dev)) {
+				pr_debug("X.25: Carrier lost -> set link state down: %s\n",
+					 dev->name);
+				nb = x25_get_neigh(dev);
+				if (nb) {
+					x25_link_terminated(nb);
+					x25_neigh_put(nb);
+				}
+			}
+			break;
 		}
 	}
 
-- 
2.20.1

[PATCH net-next v2 5/6] net/lapb: support netdev events

[Martin Schiller]

This makes it possible to handle carrier loss and detection.
In case of Carrier Loss, layer 2 is terminated
In case of Carrier Detection, we start timer t1 on a DCE interface,
and on a DTE interface we change to state LAPB_STATE_1 and start
sending SABM(E).

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---

Change from v1:
fix 'subject_prefix' and 'checkpatch' warnings

---
 net/lapb/lapb_iface.c | 83 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 3c03f6512c5f..63124cdf1926 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -418,14 +418,97 @@ int lapb_data_transmit(struct lapb_cb *lapb, struct sk_buff *skb)
 	return used;
 }
 
+/* Handle device status changes. */
+static int lapb_device_event(struct notifier_block *this, unsigned long event,
+			     void *ptr)
+{
+	struct net_device *dev = ptr;
+	struct lapb_cb *lapb;
+
+	if (!net_eq(dev_net(dev), &init_net))
+		return NOTIFY_DONE;
+
+	if (dev->type == ARPHRD_X25) {
+		switch (event) {
+		case NETDEV_REGISTER:
+			lapb_dbg(0, "(%p): got event NETDEV_REGISTER for device: %s\n",
+				 dev, dev->name);
+			break;
+		case NETDEV_POST_TYPE_CHANGE:
+			lapb_dbg(0, "(%p): got event NETDEV_POST_TYPE_CHANGE for device: %s\n",
+				 dev, dev->name);
+			break;
+		case NETDEV_UP:
+			lapb_dbg(0, "(%p): got event NETDEV_UP for device: %s\n",
+				 dev, dev->name);
+			break;
+		case NETDEV_GOING_DOWN:
+			lapb_dbg(0, "(%p): got event NETDEV_GOING_DOWN for device: %s\n",
+				 dev, dev->name);
+			break;
+		case NETDEV_DOWN:
+			lapb_dbg(0, "(%p): got event NETDEV_DOWN for device: %s\n",
+				 dev, dev->name);
+			break;
+		case NETDEV_PRE_TYPE_CHANGE:
+			lapb_dbg(0, "(%p): got event NETDEV_PRE_TYPE_CHANGE for device: %s\n",
+				 dev, dev->name);
+			break;
+		case NETDEV_UNREGISTER:
+			lapb_dbg(0, "(%p): got event NETDEV_UNREGISTER for device: %s\n",
+				 dev, dev->name);
+			break;
+		case NETDEV_CHANGE:
+			lapb_dbg(0, "(%p): got event NETDEV_CHANGE for device: %s\n",
+				 dev, dev->name);
+			lapb = lapb_devtostruct(dev);
+			if (!lapb)
+				break;
+
+			if (!netif_carrier_ok(dev)) {
+				lapb_dbg(0, "(%p): Carrier lost -> Entering LAPB_STATE_0: %s\n",
+					 dev, dev->name);
+				lapb_disconnect_indication(lapb, LAPB_OK);
+				lapb_clear_queues(lapb);
+				lapb->state = LAPB_STATE_0;
+				lapb->n2count   = 0;
+				lapb_stop_t1timer(lapb);
+				lapb_stop_t2timer(lapb);
+			} else {
+				lapb_dbg(0, "(%p): Carrier detected: %s\n",
+					 dev, dev->name);
+				if (lapb->mode & LAPB_DCE) {
+					lapb_start_t1timer(lapb);
+				} else {
+					if (lapb->state == LAPB_STATE_0) {
+						lapb->state = LAPB_STATE_1;
+						lapb_establish_data_link(lapb);
+					}
+				}
+			}
+			break;
+		}
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block lapb_dev_notifier = {
+	.notifier_call = lapb_device_event,
+};
+
 static int __init lapb_init(void)
 {
+	register_netdevice_notifier(&lapb_dev_notifier);
+
 	return 0;
 }
 
 static void __exit lapb_exit(void)
 {
 	WARN_ON(!list_empty(&lapb_list));
+
+	unregister_netdevice_notifier(&lapb_dev_notifier);
 }
 
 MODULE_AUTHOR("Jonathan Naylor <g4klx@g4klx.demon.co.uk>");
-- 
2.20.1

[PATCH net-next v2 6/6] net/lapb: fix t1 timer handling

[Martin Schiller]

fix t1 timer handling in LAPB_STATE_0:
 o DTE interface changes immediately to LAPB_STATE_1 and start sending
   SABM(E).
 o DCE interface sends N2-times DM and changes to LAPB_STATE_1
   afterwards if there is no response in the meantime.

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---

Change from v1:
fix 'subject_prefix' and 'checkpatch' warnings

---
 net/lapb/lapb_timer.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/lapb/lapb_timer.c b/net/lapb/lapb_timer.c
index 8f5b17001a07..baa247fe4ed0 100644
--- a/net/lapb/lapb_timer.c
+++ b/net/lapb/lapb_timer.c
@@ -85,11 +85,18 @@ static void lapb_t1timer_expiry(struct timer_list *t)
 	switch (lapb->state) {
 
 		/*
-		 *	If we are a DCE, keep going DM .. DM .. DM
+		 *	If we are a DCE, send DM up to N2 times, then switch to
+		 *	STATE_1 and send SABM(E).
 		 */
 		case LAPB_STATE_0:
-			if (lapb->mode & LAPB_DCE)
+			if (lapb->mode & LAPB_DCE &&
+			    lapb->n2count != lapb->n2) {
+				lapb->n2count++;
 				lapb_send_control(lapb, LAPB_DM, LAPB_POLLOFF, LAPB_RESPONSE);
+			} else {
+				lapb->state = LAPB_STATE_1;
+				lapb_establish_data_link(lapb);
+			}
 			break;
 
 		/*
-- 
2.20.1

RE: [PATCH net-next v2 2/6] net/x25: make neighbour params configurable

[David Laight]

From: Martin Schiller
> Sent: 16 November 2020 13:55
> Extended struct x25_neigh and x25_subscrip_struct to configure following
> params through SIOCX25SSUBSCRIP:
>   o mode (DTE/DCE)
>   o number of channels
>   o facilities (packet size, window size)
>   o timer T20
> 
> Based on this configuration options the following changes/extensions
> where made:
>   o DTE/DCE handling to select the next lc (DCE=from bottom / DTE=from
>     top)
>   o DTE/DCE handling to set correct clear/reset/restart cause
>   o take default facilities from neighbour settings
> 
...
> +/*
> + *	DTE/DCE subscription options.
> + *
> + *      As this is missing lots of options, user should expect major
> + *	changes of this structure in 2.5.x which might break compatibility.

A little out of date!

> + *      The somewhat ugly dimension 200-sizeof() is needed to maintain
> + *	backward compatibility.
> + */
> +struct x25_subscrip_struct {
> +	char device[200 - ((2 * sizeof(unsigned long)) +
> +		    sizeof(struct x25_facilities) +
> +		    (2 * sizeof(unsigned int)))];
> +	unsigned int		dce;
> +	unsigned int		lc;
> +	struct x25_facilities	facilities;
> +	unsigned long		t20;
> +	unsigned long		global_facil_mask;	/* 0 to disable negotiation */
> +	unsigned int		extended;
> +};

Would it be better to used fixed size integer types to avoid
'compat_32' issues?

It might even be worth adding padding after the existing
32bit layout to align any additional fields at the same offset
in both 64bit and 32bit systems.

I was also wondering if you can use an anonymous structure
member for the actual fields and then use 200 - sizeof (struct foo)
for the pad?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH net-next v2 5/6] net/lapb: support netdev events

[Xie He]

On Mon, Nov 16, 2020 at 6:01 AM Martin Schiller <ms@dev.tdt.de> wrote:
>
> This makes it possible to handle carrier loss and detection.
> In case of Carrier Loss, layer 2 is terminated
> In case of Carrier Detection, we start timer t1 on a DCE interface,
> and on a DTE interface we change to state LAPB_STATE_1 and start
> sending SABM(E).

> +                               lapb_dbg(0, "(%p): Carrier detected: %s\n",
> +                                        dev, dev->name);
> +                               if (lapb->mode & LAPB_DCE) {
> +                                       lapb_start_t1timer(lapb);
> +                               } else {
> +                                       if (lapb->state == LAPB_STATE_0) {
> +                                               lapb->state = LAPB_STATE_1;
> +                                               lapb_establish_data_link(lapb);
> +                                       }
> +                               }

Do you mean we will now automatically establish LAPB connections
without upper layers instructing us to do so?

If that is the case, is the one-byte header for instructing the LAPB
layer to connect / disconnect no longer needed?

Re: [PATCH net-next v2 5/6] net/lapb: support netdev events

[Martin Schiller]

On 2020-11-16 21:16, Xie He wrote:
> Do you mean we will now automatically establish LAPB connections
> without upper layers instructing us to do so?

Yes, as soon as the physical link is established, the L2 and also the
L3 layer (restart handshake) is established.

In this context I also noticed that I should add another patch to this
patch-set to correct the restart handling.

As already mentioned I have a stack of fixes and extensions lying around
that I would like to get upstream.

> If that is the case, is the one-byte header for instructing the LAPB
> layer to connect / disconnect no longer needed?

The one-byte header is still needed to signal the status of the LAPB
connection to the upper layer.

Re: [PATCH net-next v2 5/6] net/lapb: support netdev events

[Xie He]

On Tue, Nov 17, 2020 at 1:53 AM Martin Schiller <ms@dev.tdt.de> wrote:
>
> On 2020-11-16 21:16, Xie He wrote:
> > Do you mean we will now automatically establish LAPB connections
> > without upper layers instructing us to do so?
>
> Yes, as soon as the physical link is established, the L2 and also the
> L3 layer (restart handshake) is established.

I see. Looking at your code in Patch 1 and this patch, I see after the
device goes up, L3 code will instruct L2 to establish the connection,
and before the device goes down, L3 will instruct L2 to terminate the
connection. But if there is a carrier up/down event, L2 will
automatically handle this without being instructed by L3, and it will
establish the connection automatically when the carrier goes up. L2
will notify L3 on any L2 link status change.

Is this right? I think for a DCE, it doesn't need to initiate the L2
connection on device-up. It just needs to wait for a connection to
come. But L3 seems to be still instructing it to initiate the L2
connection. This seems to be a problem.

It feels unclean to me that the L2 connection will sometimes be
initiated by L3 and sometimes by L2 itself. Can we make L2 connections
always be initiated by L2 itself? If L3 needs to do something after L2
links up, L2 will notify it anyway.

> In this context I also noticed that I should add another patch to this
> patch-set to correct the restart handling.

Do you mean you will add code to let L3 restart any L3 connections
previously abnormally terminated after L2 link up?

> As already mentioned I have a stack of fixes and extensions lying around
> that I would like to get upstream.

Please do so! Thanks!

I previously found a locking problem in X.25 code and tried to address it in:
https://patchwork.kernel.org/project/netdevbpf/patch/20201114103625.323919-1-xie.he.0141@gmail.com/
But later I found I needed to fix more code than I previously thought.
Do you already have a fix for this problem?

> > If that is the case, is the one-byte header for instructing the LAPB
> > layer to connect / disconnect no longer needed?
>
> The one-byte header is still needed to signal the status of the LAPB
> connection to the upper layer.

Re: [PATCH net-next v2 4/6] net/x25: support NETDEV_CHANGE notifier

[Xie He]

On Mon, Nov 16, 2020 at 6:00 AM Martin Schiller <ms@dev.tdt.de> wrote:
>
> This makes it possible to handle carrier lost and detection.
> In case of carrier lost, we shutdown layer 3 and flush all sessions.
>
> @@ -275,6 +275,19 @@ static int x25_device_event(struct notifier_block *this, unsigned long event,
>                                  dev->name);
>                         x25_link_device_remove(dev);
>                         break;
> +               case NETDEV_CHANGE:
> +                       pr_debug("X.25: got event NETDEV_CHANGE for device: %s\n",
> +                                dev->name);
> +                       if (!netif_carrier_ok(dev)) {
> +                               pr_debug("X.25: Carrier lost -> set link state down: %s\n",
> +                                        dev->name);
> +                               nb = x25_get_neigh(dev);
> +                               if (nb) {
> +                                       x25_link_terminated(nb);
> +                                       x25_neigh_put(nb);
> +                               }
> +                       }
> +                       break;
>                 }
>         }

I think L2 will notify L3 if the L2 connection is terminated. Is this
patch necessary?

Re: [PATCH net-next v2 4/6] net/x25: support NETDEV_CHANGE notifier

[Martin Schiller]

On 2020-11-17 12:41, Xie He wrote:
> On Mon, Nov 16, 2020 at 6:00 AM Martin Schiller <ms@dev.tdt.de> wrote:
>> 
>> This makes it possible to handle carrier lost and detection.
>> In case of carrier lost, we shutdown layer 3 and flush all sessions.
>> 
>> @@ -275,6 +275,19 @@ static int x25_device_event(struct notifier_block 
>> *this, unsigned long event,
>>                                  dev->name);
>>                         x25_link_device_remove(dev);
>>                         break;
>> +               case NETDEV_CHANGE:
>> +                       pr_debug("X.25: got event NETDEV_CHANGE for 
>> device: %s\n",
>> +                                dev->name);
>> +                       if (!netif_carrier_ok(dev)) {
>> +                               pr_debug("X.25: Carrier lost -> set 
>> link state down: %s\n",
>> +                                        dev->name);
>> +                               nb = x25_get_neigh(dev);
>> +                               if (nb) {
>> +                                       x25_link_terminated(nb);
>> +                                       x25_neigh_put(nb);
>> +                               }
>> +                       }
>> +                       break;
>>                 }
>>         }
> 
> I think L2 will notify L3 if the L2 connection is terminated. Is this
> patch necessary?

Hmm... well I guess you're right. Admittedly, these patches were made
about 7 - 8 years ago and I have to keep thinking about them.
But I can't think of any situation where this patch should be necessary
at the moment.

I will drop this patch from the patch-set.

Re: [PATCH net-next v2 5/6] net/lapb: support netdev events

[Martin Schiller]

On 2020-11-17 12:32, Xie He wrote:
> On Tue, Nov 17, 2020 at 1:53 AM Martin Schiller <ms@dev.tdt.de> wrote:
>> 
>> On 2020-11-16 21:16, Xie He wrote:
>> > Do you mean we will now automatically establish LAPB connections
>> > without upper layers instructing us to do so?
>> 
>> Yes, as soon as the physical link is established, the L2 and also the
>> L3 layer (restart handshake) is established.
> 
> I see. Looking at your code in Patch 1 and this patch, I see after the
> device goes up, L3 code will instruct L2 to establish the connection,
> and before the device goes down, L3 will instruct L2 to terminate the
> connection. But if there is a carrier up/down event, L2 will
> automatically handle this without being instructed by L3, and it will
> establish the connection automatically when the carrier goes up. L2
> will notify L3 on any L2 link status change.
> 
> Is this right?

Yes, this is right.

> I think for a DCE, it doesn't need to initiate the L2
> connection on device-up. It just needs to wait for a connection to
> come. But L3 seems to be still instructing it to initiate the L2
> connection. This seems to be a problem.

The "ITU-T Recommendation X.25 (10/96) aka "Blue Book" [1] says under
point 2.4.4.1:
"Either the DTE or the DCE may initiate data link set-up."

Experience shows that there are also DTEs that do not establish a
connection themselves.

That is also the reason why I've added this patch:
https://patchwork.kernel.org/project/netdevbpf/patch/20201116135522.21791-7-ms@dev.tdt.de/

> It feels unclean to me that the L2 connection will sometimes be
> initiated by L3 and sometimes by L2 itself. Can we make L2 connections
> always be initiated by L2 itself? If L3 needs to do something after L2
> links up, L2 will notify it anyway.

My original goal was to change as little as possible of the original
code. And in the original code the NETDEV_UP/NETDEV_DOWN events were/are
handled in L3. But it is of course conceivable to shift this to L2.

But you have to keep in mind that the X.25 L3 stack can also be used
with tap interfaces (e.g. for XOT), where you do not have a L2 at all.

> 
>> In this context I also noticed that I should add another patch to this
>> patch-set to correct the restart handling.
> 
> Do you mean you will add code to let L3 restart any L3 connections
> previously abnormally terminated after L2 link up?

No, I mean the handling of Restart Request and Restart Confirm is buggy
and needs to be fixed also.

> 
>> As already mentioned I have a stack of fixes and extensions lying 
>> around
>> that I would like to get upstream.
> 
> Please do so! Thanks!
> 
> I previously found a locking problem in X.25 code and tried to address 
> it in:
> https://patchwork.kernel.org/project/netdevbpf/patch/20201114103625.323919-1-xie.he.0141@gmail.com/
> But later I found I needed to fix more code than I previously thought.
> Do you already have a fix for this problem?

No, sorry.


[1] https://www.itu.int/rec/T-REC-X.25-199610-I/

Re: [PATCH net-next v2 5/6] net/lapb: support netdev events

[Xie He]

On Tue, Nov 17, 2020 at 5:26 AM Martin Schiller <ms@dev.tdt.de> wrote:
>
> On 2020-11-17 12:32, Xie He wrote:
> >
> > I think for a DCE, it doesn't need to initiate the L2
> > connection on device-up. It just needs to wait for a connection to
> > come. But L3 seems to be still instructing it to initiate the L2
> > connection. This seems to be a problem.
>
> The "ITU-T Recommendation X.25 (10/96) aka "Blue Book" [1] says under
> point 2.4.4.1:
> "Either the DTE or the DCE may initiate data link set-up."
>
> Experience shows that there are also DTEs that do not establish a
> connection themselves.
>
> That is also the reason why I've added this patch:
> https://patchwork.kernel.org/project/netdevbpf/patch/20201116135522.21791-7-ms@dev.tdt.de/

Yes, I understand that either the DTE or the DCE *may* initiate the L2
connection. This is also the way the current code (before this patch
set) works. But I see both the DTE and the DCE will try to initiate
the L2 connection after device-up, because according to your 1st
patch, L3 will always instruct L2 to do this on device-up. However,
looking at your 6th patch (in the link you gave), you seem to want the
DCE to wait for a while before initiating the connection by itself. So
I'm unclear which way you want to go. Making DCE initiate the L2
connection on device-up, or making DCE wait for a while before
initiating the L2 connection? I think the second way is more
reasonable.

> > It feels unclean to me that the L2 connection will sometimes be
> > initiated by L3 and sometimes by L2 itself. Can we make L2 connections
> > always be initiated by L2 itself? If L3 needs to do something after L2
> > links up, L2 will notify it anyway.
>
> My original goal was to change as little as possible of the original
> code. And in the original code the NETDEV_UP/NETDEV_DOWN events were/are
> handled in L3. But it is of course conceivable to shift this to L2.

I suggested moving L2 connection handling to L2 because I think having
both L2 and L3 to handle this makes the logic of the code too complex.
For example, after a device-up event, L3 will instruct L2 to initiate
the L2 connection. But L2 code has its own way of initiating
connections. For a DCE, L2 wants to wait a while before initiating the
connection. So currently L2 and L3 want to do things differently and
they are doing things at the same time.

> But you have to keep in mind that the X.25 L3 stack can also be used
> with tap interfaces (e.g. for XOT), where you do not have a L2 at all.

Can we treat XOT the same as LAPB? I think XOT should be considered a
L2 in this case. So maybe XOT can establish the TCP connection by
itself without being instructed by L3. I'm not sure if this is
feasible in practice but it'd be good if it is.

This also simplifies the L3 code.

Re: [PATCH net-next v2 3/6] net/x25: replace x25_kill_by_device with x25_kill_by_neigh

[Xie He]

On Mon, Nov 16, 2020 at 6:00 AM Martin Schiller <ms@dev.tdt.de> wrote:
>
> Remove unnecessary function x25_kill_by_device.

> -/*
> - *     Kill all bound sockets on a dropped device.
> - */
> -static void x25_kill_by_device(struct net_device *dev)
> -{
> -       struct sock *s;
> -
> -       write_lock_bh(&x25_list_lock);
> -
> -       sk_for_each(s, &x25_list)
> -               if (x25_sk(s)->neighbour && x25_sk(s)->neighbour->dev == dev)
> -                       x25_disconnect(s, ENETUNREACH, 0, 0);
> -
> -       write_unlock_bh(&x25_list_lock);
> -}
> -
>  /*
>   *     Handle device status changes.
>   */
> @@ -273,7 +257,11 @@ static int x25_device_event(struct notifier_block *this, unsigned long event,
>                 case NETDEV_DOWN:
>                         pr_debug("X.25: got event NETDEV_DOWN for device: %s\n",
>                                  dev->name);
> -                       x25_kill_by_device(dev);
> +                       nb = x25_get_neigh(dev);
> +                       if (nb) {
> +                               x25_kill_by_neigh(nb);
> +                               x25_neigh_put(nb);
> +                       }
>                         x25_route_device_down(dev);
>                         x25_link_device_down(dev);
>                         break;

This patch might not be entirely necessary. x25_kill_by_neigh and
x25_kill_by_device are just two helper functions. One function takes
nb as the argument and the other one takes dev as the argument. But
they do almost the same things. It doesn't harm to keep both. In C++
we often have different functions with the same name doing almost the
same things.

The original code also seems to be a little more efficient than the new code.

Re: [PATCH net-next v2 3/6] net/x25: replace x25_kill_by_device with x25_kill_by_neigh

[Martin Schiller]

On 2020-11-17 20:50, Xie He wrote:
> On Mon, Nov 16, 2020 at 6:00 AM Martin Schiller <ms@dev.tdt.de> wrote:
>> 
>> Remove unnecessary function x25_kill_by_device.
> 
>> -/*
>> - *     Kill all bound sockets on a dropped device.
>> - */
>> -static void x25_kill_by_device(struct net_device *dev)
>> -{
>> -       struct sock *s;
>> -
>> -       write_lock_bh(&x25_list_lock);
>> -
>> -       sk_for_each(s, &x25_list)
>> -               if (x25_sk(s)->neighbour && x25_sk(s)->neighbour->dev 
>> == dev)
>> -                       x25_disconnect(s, ENETUNREACH, 0, 0);
>> -
>> -       write_unlock_bh(&x25_list_lock);
>> -}
>> -
>>  /*
>>   *     Handle device status changes.
>>   */
>> @@ -273,7 +257,11 @@ static int x25_device_event(struct notifier_block 
>> *this, unsigned long event,
>>                 case NETDEV_DOWN:
>>                         pr_debug("X.25: got event NETDEV_DOWN for 
>> device: %s\n",
>>                                  dev->name);
>> -                       x25_kill_by_device(dev);
>> +                       nb = x25_get_neigh(dev);
>> +                       if (nb) {
>> +                               x25_kill_by_neigh(nb);
>> +                               x25_neigh_put(nb);
>> +                       }
>>                         x25_route_device_down(dev);
>>                         x25_link_device_down(dev);
>>                         break;
> 
> This patch might not be entirely necessary. x25_kill_by_neigh and
> x25_kill_by_device are just two helper functions. One function takes
> nb as the argument and the other one takes dev as the argument. But
> they do almost the same things. It doesn't harm to keep both. In C++
> we often have different functions with the same name doing almost the
> same things.
> 

Well I don't like to have 2 functions doing the same thing.
But after another look at this code, I've found that I also need to
remove the call to x25_clear_forward_by_dev() in the function
x25_route_device_down(). Otherwise, it will be called twice.

> The original code also seems to be a little more efficient than the new 
> code.

The only difference would be the x25_get_neigh() and x25_neigh_put()
calls. That shouldn't cost to much.

Re: [PATCH net-next v2 5/6] net/lapb: support netdev events

[Martin Schiller]

On 2020-11-17 19:28, Xie He wrote:
> On Tue, Nov 17, 2020 at 5:26 AM Martin Schiller <ms@dev.tdt.de> wrote:
>> 
>> On 2020-11-17 12:32, Xie He wrote:
>> >
>> > I think for a DCE, it doesn't need to initiate the L2
>> > connection on device-up. It just needs to wait for a connection to
>> > come. But L3 seems to be still instructing it to initiate the L2
>> > connection. This seems to be a problem.
>> 
>> The "ITU-T Recommendation X.25 (10/96) aka "Blue Book" [1] says under
>> point 2.4.4.1:
>> "Either the DTE or the DCE may initiate data link set-up."
>> 
>> Experience shows that there are also DTEs that do not establish a
>> connection themselves.
>> 
>> That is also the reason why I've added this patch:
>> https://patchwork.kernel.org/project/netdevbpf/patch/20201116135522.21791-7-ms@dev.tdt.de/
> 
> Yes, I understand that either the DTE or the DCE *may* initiate the L2
> connection. This is also the way the current code (before this patch
> set) works. But I see both the DTE and the DCE will try to initiate
> the L2 connection after device-up, because according to your 1st
> patch, L3 will always instruct L2 to do this on device-up. However,
> looking at your 6th patch (in the link you gave), you seem to want the
> DCE to wait for a while before initiating the connection by itself. So
> I'm unclear which way you want to go. Making DCE initiate the L2
> connection on device-up, or making DCE wait for a while before
> initiating the L2 connection? I think the second way is more
> reasonable.

Ah, ok. Now I see what you mean.
Yes, we should check the lapb->mode in lapb_connect_request().

>> > It feels unclean to me that the L2 connection will sometimes be
>> > initiated by L3 and sometimes by L2 itself. Can we make L2 connections
>> > always be initiated by L2 itself? If L3 needs to do something after L2
>> > links up, L2 will notify it anyway.
>> 
>> My original goal was to change as little as possible of the original
>> code. And in the original code the NETDEV_UP/NETDEV_DOWN events 
>> were/are
>> handled in L3. But it is of course conceivable to shift this to L2.
> 
> I suggested moving L2 connection handling to L2 because I think having
> both L2 and L3 to handle this makes the logic of the code too complex.
> For example, after a device-up event, L3 will instruct L2 to initiate
> the L2 connection. But L2 code has its own way of initiating
> connections. For a DCE, L2 wants to wait a while before initiating the
> connection. So currently L2 and L3 want to do things differently and
> they are doing things at the same time.
> 
>> But you have to keep in mind that the X.25 L3 stack can also be used
>> with tap interfaces (e.g. for XOT), where you do not have a L2 at all.
> 
> Can we treat XOT the same as LAPB? I think XOT should be considered a
> L2 in this case. So maybe XOT can establish the TCP connection by
> itself without being instructed by L3. I'm not sure if this is
> feasible in practice but it'd be good if it is.
> 
> This also simplifies the L3 code.

I also have a patch here that implements an "on demand" link feature,
which we used for ISDN dialing connections.
As ISDN is de facto dead, this is not relevant anymore. But if we want
such kind of feature, I think we need to stay with the method to control
L2 link state from L3.

Re: [PATCH net-next v2 5/6] net/lapb: support netdev events

[Xie He]

On Wed, Nov 18, 2020 at 12:49 AM Martin Schiller <ms@dev.tdt.de> wrote:
>
> Ah, ok. Now I see what you mean.
> Yes, we should check the lapb->mode in lapb_connect_request().
...
> I also have a patch here that implements an "on demand" link feature,
> which we used for ISDN dialing connections.
> As ISDN is de facto dead, this is not relevant anymore. But if we want
> such kind of feature, I think we need to stay with the method to control
> L2 link state from L3.

I see. Hmm...

I guess for ISDN, the current code (before this patch series) is the
best. We only establish the connection when L3 has packets to send.

Can we do this? We let L2 handle all device-up / device-down /
carrier-up / carrier-down events. And when L3 has some packets to send
but it still finds the L2 link is not up, it will then instruct L2 to
connect.

This way we may be able to both keep the logic simple and still keep
L3 compatible with ISDN.

Re: [PATCH net-next v2 5/6] net/lapb: support netdev events

[Xie He]

On Wed, Nov 18, 2020 at 5:03 AM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Wed, Nov 18, 2020 at 12:49 AM Martin Schiller <ms@dev.tdt.de> wrote:
> >
> > I also have a patch here that implements an "on demand" link feature,
> > which we used for ISDN dialing connections.
> > As ISDN is de facto dead, this is not relevant anymore. But if we want
> > such kind of feature, I think we need to stay with the method to control
> > L2 link state from L3.
>
> I see. Hmm...
>
> I guess for ISDN, the current code (before this patch series) is the
> best. We only establish the connection when L3 has packets to send.
>
> Can we do this? We let L2 handle all device-up / device-down /
> carrier-up / carrier-down events. And when L3 has some packets to send
> but it still finds the L2 link is not up, it will then instruct L2 to
> connect.
>
> This way we may be able to both keep the logic simple and still keep
> L3 compatible with ISDN.

Another solution might be letting ISDN automatically connect when it
receives the first packet from L3. This way we can still free L3 from
all handlings of L2 connections.

Re: [PATCH net-next v2 5/6] net/lapb: support netdev events

[Martin Schiller]

On 2020-11-18 14:46, Xie He wrote:
> On Wed, Nov 18, 2020 at 5:03 AM Xie He <xie.he.0141@gmail.com> wrote:
>> 
>> On Wed, Nov 18, 2020 at 12:49 AM Martin Schiller <ms@dev.tdt.de> 
>> wrote:
>> >
>> > I also have a patch here that implements an "on demand" link feature,
>> > which we used for ISDN dialing connections.
>> > As ISDN is de facto dead, this is not relevant anymore. But if we want
>> > such kind of feature, I think we need to stay with the method to control
>> > L2 link state from L3.
>> 
>> I see. Hmm...
>> 
>> I guess for ISDN, the current code (before this patch series) is the
>> best. We only establish the connection when L3 has packets to send.
>> 
>> Can we do this? We let L2 handle all device-up / device-down /
>> carrier-up / carrier-down events. And when L3 has some packets to send
>> but it still finds the L2 link is not up, it will then instruct L2 to
>> connect.
>> 
>> This way we may be able to both keep the logic simple and still keep
>> L3 compatible with ISDN.
> 
> Another solution might be letting ISDN automatically connect when it
> receives the first packet from L3. This way we can still free L3 from
> all handlings of L2 connections.

ISDN is not important now. Also the I4L subsystem has been removed.

I have now completely reworked the patch-set and it is now much tidier.
For now I left the event handling completely in X.25 (L3).

I will now send the whole thing as v3 and we can discuss it further.